Xen devel - Jun 2013 - [PATCH v2 1/5] hvmloader: Correct bug in low mmio region accounting

When deciding whether to map a device in low MMIO space (<4GiB),
hvmloader compares it with "mmio_left", which is set to the size of
the low MMIO range (pci_mem_end - pci_mem_start).  However, even if it
does map a device in high MMIO space, it still removes the size of its
BAR from mmio_left.

This patch first changes the name of this variable to "low_mmio_left"
to distinguish it from generic MMIO, and corrects the logic to only
subtract the size of the BAR for devices maped in the low MMIO region.

Also make low_mmio_left unsigned, and don''t allow it to go negative.
Since its main use is to be compared to a 64-bit unsigned int, this
may have undefined (and in practice almost certainly incorrect)
results.  Not subtracting is OK because if there''s not enough room, it
won''t actually be mapped.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
---
 tools/firmware/hvmloader/pci.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index c78d4d3..8691a19 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -38,11 +38,10 @@ void pci_setup(void)
 {
     uint8_t is_64bar, using_64bar, bar64_relocate = 0;
     uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
-    uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0;
+    uint64_t base, bar_sz, bar_sz_upper, low_mmio_left, mmio_total = 0;
     uint32_t vga_devfn = 256;
     uint16_t class, vendor_id, device_id;
     unsigned int bar, pin, link, isa_irq;
-    int64_t mmio_left;
 
     /* Resources assignable to PCI devices via BARs. */
     struct resource {
@@ -244,7 +243,7 @@ void pci_setup(void)
     io_resource.base = 0xc000;
     io_resource.max = 0x10000;
 
-    mmio_left = pci_mem_end - pci_mem_start;
+    low_mmio_left = pci_mem_end - pci_mem_start;
 
     /* Assign iomem and ioport resources in descending order of size. */
     for ( i = 0; i < nr_bars; i++ )
@@ -253,7 +252,7 @@ void pci_setup(void)
         bar_reg = bars[i].bar_reg;
         bar_sz  = bars[i].bar_sz;
 
-        using_64bar = bars[i].is_64bar && bar64_relocate &&
(mmio_left < bar_sz);
+        using_64bar = bars[i].is_64bar && bar64_relocate &&
(low_mmio_left < bar_sz);
         bar_data = pci_readl(devfn, bar_reg);
 
         if ( (bar_data & PCI_BASE_ADDRESS_SPACE) =@@ -273,9 +272,10 @@ void
pci_setup(void)
             } 
             else {
                 resource = &mem_resource;
+                if ( bar_sz <= low_mmio_left )
+                    low_mmio_left -= bar_sz;
                 bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
             }
-            mmio_left -= bar_sz;
         }
         else
         {
-- 
1.7.9.5

Keep track of how much mmio space is left total, as well as the amount
of "low" MMIO space (<4GiB), and only load devices into high memory
if there is not enough low memory for the rest of the devices to fit.

Because devices are processed by size in order from large to small,
this should preferentially relocate devices with large BARs to 64-bit
space.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
---
 tools/firmware/hvmloader/pci.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 8691a19..7f306a1 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -38,7 +38,8 @@ void pci_setup(void)
 {
     uint8_t is_64bar, using_64bar, bar64_relocate = 0;
     uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
-    uint64_t base, bar_sz, bar_sz_upper, low_mmio_left, mmio_total = 0;
+    uint64_t base, bar_sz, bar_sz_upper, low_mmio_left, mmio_total = 0,
+        mmio_left;
     uint32_t vga_devfn = 256;
     uint16_t class, vendor_id, device_id;
     unsigned int bar, pin, link, isa_irq;
@@ -244,6 +245,7 @@ void pci_setup(void)
     io_resource.max = 0x10000;
 
     low_mmio_left = pci_mem_end - pci_mem_start;
+    mmio_left = mmio_total;
 
     /* Assign iomem and ioport resources in descending order of size. */
     for ( i = 0; i < nr_bars; i++ )
@@ -252,7 +254,12 @@ void pci_setup(void)
         bar_reg = bars[i].bar_reg;
         bar_sz  = bars[i].bar_sz;
 
-        using_64bar = bars[i].is_64bar && bar64_relocate &&
(low_mmio_left < bar_sz);
+        /* Relocate to high memory if the total amount of MMIO needed
+         * is more than the low MMIO available.  Because devices are
+         * processed in order of bar_sz, this will preferentially
+         * relocate larger devices to high memory first. */
+        using_64bar = bars[i].is_64bar && bar64_relocate
+                       && (mmio_left > low_mmio_left);
         bar_data = pci_readl(devfn, bar_reg);
 
         if ( (bar_data & PCI_BASE_ADDRESS_SPACE) =@@ -276,6 +283,7 @@ void
pci_setup(void)
                     low_mmio_left -= bar_sz;
                 bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
             }
+            mmio_left -= bar_sz;
         }
         else
         {
-- 
1.7.9.5

Allow devices with BARs less than 512MiB to be relocated to high memory.

This will only be invoked if there is not enough low MMIO space to map
the device, and will be done preferentially to large devices first; so
in all likelihood only large devices will be remapped anyway.

This is needed to work-around the issue of qemu-xen not being able to
handle moving guest memory around to resize the MMIO hole.  The default
MMIO hole size is less than 256MiB.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
---
 tools/firmware/hvmloader/config.h |    1 -
 tools/firmware/hvmloader/pci.c    |    5 ++---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/firmware/hvmloader/config.h
b/tools/firmware/hvmloader/config.h
index 8143d6f..6641197 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -55,7 +55,6 @@ extern struct bios_config ovmf_config;
 /* MMIO hole: Hardcoded defaults, which can be dynamically expanded. */
 #define PCI_MEM_START       0xf0000000
 #define PCI_MEM_END         0xfc000000
-#define PCI_MIN_BIG_BAR_SIZE          0x20000000
 
 extern unsigned long pci_mem_start, pci_mem_end;
 
diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 7f306a1..a483b02 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -265,9 +265,8 @@ void pci_setup(void)
         if ( (bar_data & PCI_BASE_ADDRESS_SPACE) =             
PCI_BASE_ADDRESS_SPACE_MEMORY )
         {
-            /* Mapping high memory if PCI deivce is 64 bits bar and the bar
size
-               is larger than 512M */
-            if (using_64bar && (bar_sz > PCI_MIN_BIG_BAR_SIZE)) {
+            /* Mapping high memory if PCI device is 64 bits bar */
+            if (using_64bar) {
                 if ( high_mem_resource.base & (bar_sz - 1) )
                     high_mem_resource.base = high_mem_resource.base - 
                         (high_mem_resource.base & (bar_sz - 1)) + bar_sz;
-- 
1.7.9.5

After attempting to resize the MMIO hole, the check to determine
whether there is a need to relocate BARs into 64-bit space checks the
specific thing that caused the loop to exit (MMIO hole == 2GiB) rather
than checking whether the required MMIO will fit in the hole.

This should have no functional change now, but will make the next patch
(when we add more conditions for exiting the loop) more clean.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
---
 tools/firmware/hvmloader/pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index a483b02..63d79a2 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -213,7 +213,7 @@ void pci_setup(void)
             ((pci_mem_start << 1) != 0) )
         pci_mem_start <<= 1;
 
-    if ( (pci_mem_start << 1) != 0 )
+    if ( mmio_total > (pci_mem_end - pci_mem_start) )
         bar64_relocate = 1;
 
     /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
-- 
1.7.9.5

At the moment, qemu-xen can''t handle memory being relocated by
hvmloader.  This may happen if a device with a large enough memory
region is passed through to the guest.  At the moment, if this
happens, then at some point in the future qemu will crash and the
domain will hang.  (qemu-traditional is fine.)

It''s too late in the release to do a proper fix, so we try to do
damage control.

hvmloader already has mechanisms to relocate memory to 64-bit space
if it can''t make a big enough MMIO hole.  By default this is 2GiB; if
we just refuse to make the hole bigger if it will overlap with guest
memory, then the relocation will happen by default.

v2:
 - style fixes
 - fix and expand comment on the MMIO hole loop
 - use "%d" rather than "%s" ->
(...)?"1":"0"
 - use bool instead of uint8_t
 - Move 64-bit bar relocate detection to another patch
 - Add more diagnostic messages

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
---
 tools/firmware/hvmloader/pci.c          |   41 ++++++++++++++++++++++++++++---
 tools/libxl/libxl_dm.c                  |    6 +++++
 xen/include/public/hvm/hvm_xs_strings.h |    1 +
 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 63d79a2..1ab5124 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -27,6 +27,8 @@
 
 #include <xen/memory.h>
 #include <xen/hvm/ioreq.h>
+#include <xen/hvm/hvm_xs_strings.h>
+#include <stdbool.h>
 
 unsigned long pci_mem_start = PCI_MEM_START;
 unsigned long pci_mem_end = PCI_MEM_END;
@@ -58,6 +60,15 @@ void pci_setup(void)
     } *bars = (struct bars *)scratch_start;
     unsigned int i, nr_bars = 0;
 
+    const char *s;
+    bool allow_memory_relocate = 1;
+
+    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
+    if ( s )
+        allow_memory_relocate = (bool)strtoll(s, NULL, 0);
+    printf("Relocating guest memory for lowmem MMIO space %s\n",
+           allow_memory_relocate?"enabled":"disabled");
+
     /* Program PCI-ISA bridge with appropriate link routes. */
     isa_irq = 0;
     for ( link = 0; link < 4; link++ )
@@ -209,14 +220,38 @@ void pci_setup(void)
         pci_writew(devfn, PCI_COMMAND, cmd);
     }
 
-    while ( (mmio_total > (pci_mem_end - pci_mem_start)) &&
-            ((pci_mem_start << 1) != 0) )
+    /*
+     * At the moment qemu-xen can''t deal with relocated memory
regions.
+     * It''s too close to the release to make a proper fix; for now,
+     * only allow the MMIO hole to grow large enough to move guest memory
+     * if we''re running qemu-traditional.  Items that don''t
fit will be
+     * relocated into the 64-bit address space.
+     *
+     * This loop now does the following:
+     * - If allow_memory_relocate, increase the MMIO hole until it''s
+     *   big enough, or until it''s 2GiB
+     * - If !allow_memory_relocate, increase the MMIO hole until it''s
+     *   big enough, or until it''s 2GiB, or until it overlaps guest
+     *   memory
+     */
+    while ( (mmio_total > (pci_mem_end - pci_mem_start)) 
+            && ((pci_mem_start << 1) != 0)
+            && (allow_memory_relocate
+                || (((pci_mem_start << 1) >> PAGE_SHIFT)
+                    < hvm_info->low_mem_pgend)) )
         pci_mem_start <<= 1;
 
-    if ( mmio_total > (pci_mem_end - pci_mem_start) )
+    if ( mmio_total > (pci_mem_end - pci_mem_start) ) {
+        printf("<4GiB MMIO hole not large enough,"
+               " relocating some BARs to 64-bit\n");
         bar64_relocate = 1;
+    }
 
     /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
+    if ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
+        printf("Relocating 0x%lx pages to highmem for lowmem MMIO
hole\n",
+               hvm_info->low_mem_pgend - (pci_mem_start >>
PAGE_SHIFT));
+
     while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend
)
     {
         struct xen_add_to_physmap xatp;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index ac1f90e..342d2ce 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1154,6 +1154,12 @@ void libxl__spawn_local_dm(libxl__egc *egc,
libxl__dm_spawn_state *dmss)
         libxl__xs_write(gc, XBT_NULL,
                         libxl__sprintf(gc, "%s/hvmloader/bios",
path),
                         "%s",
libxl_bios_type_to_string(b_info->u.hvm.bios));
+        /* Disable relocating memory to make the MMIO hole larger
+         * unless we''re running qemu-traditional */
+        libxl__xs_write(gc, XBT_NULL,
+                        libxl__sprintf(gc,
"%s/hvmloader/allow-memory-relocate", path),
+                        "%d",
+                       
b_info->device_model_version==LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL);
         free(path);
     }
 
diff --git a/xen/include/public/hvm/hvm_xs_strings.h
b/xen/include/public/hvm/hvm_xs_strings.h
index 9042303..4de5881 100644
--- a/xen/include/public/hvm/hvm_xs_strings.h
+++ b/xen/include/public/hvm/hvm_xs_strings.h
@@ -28,6 +28,7 @@
 #define HVM_XS_HVMLOADER               "hvmloader"
 #define HVM_XS_BIOS                    "hvmloader/bios"
 #define HVM_XS_GENERATION_ID_ADDRESS  
"hvmloader/generation-id-address"
+#define HVM_XS_ALLOW_MEMORY_RELOCATE  
"hvmloader/allow-memory-relocate"
 
 /* The following values allow additional ACPI tables to be added to the
  * virtual ACPI BIOS that hvmloader constructs. The values specify the guest
-- 
1.7.9.5

On 06/18/2013 05:46 PM, George Dunlap wrote:
> When deciding whether to map a device in low MMIO space (<4GiB),
> hvmloader compares it with "mmio_left", which is set to the size
of
> the low MMIO range (pci_mem_end - pci_mem_start).  However, even if it
> does map a device in high MMIO space, it still removes the size of its
> BAR from mmio_left.
>
> This patch first changes the name of this variable to
"low_mmio_left"
> to distinguish it from generic MMIO, and corrects the logic to only
> subtract the size of the BAR for devices maped in the low MMIO region.
>
> Also make low_mmio_left unsigned, and don''t allow it to go
negative.
> Since its main use is to be compared to a 64-bit unsigned int, this
> may have undefined (and in practice almost certainly incorrect)
> results.  Not subtracting is OK because if there''s not enough
room, it
> won''t actually be mapped.
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
This patch series is a lot longer and more intrusive than I was hoping 
it would be. :-(

However, it still seems to me that this will be a smaller impact on 
people than the alternatives.

If the code is done properly, then none of these changes should have any 
effect on people who are not passing through devices with large BARs: 
the vast majority of our users, who are simply booting plain VMs in HVM 
mode, should see zero effect.

Similarly, the vast majority of qemu-traditional users should also see 
no effect; the 64-bit code should only be invoked when the MMIO hole 
cannot be resized large enough, and by default, qemu-traditional will be 
able to resize the hole up to 2GiB -- far larger than almost any guest 
should need.

qemu-xen users will be affected, but they would have been affected 
anyway.  This changes a certain "will 100% crash at some point" to
"will
probably work most of the time, with a small unforeseen chance of 
something crashing randomly".  I think that''s still a step
forward.

  -George

On 06/18/2013 05:46 PM, George Dunlap wrote:
> At the moment, qemu-xen can''t handle memory being relocated by
> hvmloader.  This may happen if a device with a large enough memory
> region is passed through to the guest.  At the moment, if this
> happens, then at some point in the future qemu will crash and the
> domain will hang.  (qemu-traditional is fine.)
>
> It''s too late in the release to do a proper fix, so we try to do
> damage control.
>
> hvmloader already has mechanisms to relocate memory to 64-bit space
> if it can''t make a big enough MMIO hole.  By default this is 2GiB;
if
> we just refuse to make the hole bigger if it will overlap with guest
> memory, then the relocation will happen by default.
>
> v2:
>   - style fixes
>   - fix and expand comment on the MMIO hole loop
>   - use "%d" rather than "%s" ->
(...)?"1":"0"
>   - use bool instead of uint8_t
>   - Move 64-bit bar relocate detection to another patch
>   - Add more diagnostic messages
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Hanweidong <hanweidong@huawei.com>
> ---
>   tools/firmware/hvmloader/pci.c          |   41
++++++++++++++++++++++++++++---
>   tools/libxl/libxl_dm.c                  |    6 +++++
>   xen/include/public/hvm/hvm_xs_strings.h |    1 +
>   3 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/tools/firmware/hvmloader/pci.c
b/tools/firmware/hvmloader/pci.c
> index 63d79a2..1ab5124 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -27,6 +27,8 @@
>
>   #include <xen/memory.h>
>   #include <xen/hvm/ioreq.h>
> +#include <xen/hvm/hvm_xs_strings.h>
> +#include <stdbool.h>
>
>   unsigned long pci_mem_start = PCI_MEM_START;
>   unsigned long pci_mem_end = PCI_MEM_END;
> @@ -58,6 +60,15 @@ void pci_setup(void)
>       } *bars = (struct bars *)scratch_start;
>       unsigned int i, nr_bars = 0;
>
> +    const char *s;
> +    bool allow_memory_relocate = 1;
> +
> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
> +    if ( s )
> +        allow_memory_relocate = (bool)strtoll(s, NULL, 0);
> +    printf("Relocating guest memory for lowmem MMIO space %s\n",
> +          
allow_memory_relocate?"enabled":"disabled");
> +
>       /* Program PCI-ISA bridge with appropriate link routes. */
>       isa_irq = 0;
>       for ( link = 0; link < 4; link++ )
> @@ -209,14 +220,38 @@ void pci_setup(void)
>           pci_writew(devfn, PCI_COMMAND, cmd);
>       }
>
> -    while ( (mmio_total > (pci_mem_end - pci_mem_start)) &&
> -            ((pci_mem_start << 1) != 0) )
> +    /*
> +     * At the moment qemu-xen can''t deal with relocated memory
regions.
> +     * It''s too close to the release to make a proper fix; for
now,
> +     * only allow the MMIO hole to grow large enough to move guest memory
> +     * if we''re running qemu-traditional.  Items that
don''t fit will be
> +     * relocated into the 64-bit address space.
> +     *
> +     * This loop now does the following:
> +     * - If allow_memory_relocate, increase the MMIO hole until
it''s
> +     *   big enough, or until it''s 2GiB
> +     * - If !allow_memory_relocate, increase the MMIO hole until
it''s
> +     *   big enough, or until it''s 2GiB, or until it overlaps
guest
> +     *   memory
> +     */
> +    while ( (mmio_total > (pci_mem_end - pci_mem_start))
> +            && ((pci_mem_start << 1) != 0)
> +            && (allow_memory_relocate
> +                || (((pci_mem_start << 1) >> PAGE_SHIFT)
> +                    < hvm_info->low_mem_pgend)) )
Hmm, the polarity on the comparison here is wrong -- it should be >=, not
<.

I''ve hacked up qemu so that it presents a 256MiB xen platform pci 
device.  I''ll re-send when I''ve done some more testing.

But I have, I think, reproduced the original problem; the above error 
means that for qemu-xen, it will resize the MMIO hole only if it *can* 
move guest memory. :-)

  -George

On Tue, 18 Jun 2013, George Dunlap wrote:
> When deciding whether to map a device in low MMIO space (<4GiB),
> hvmloader compares it with "mmio_left", which is set to the size
of
> the low MMIO range (pci_mem_end - pci_mem_start).  However, even if it
> does map a device in high MMIO space, it still removes the size of its
> BAR from mmio_left.
> 
> This patch first changes the name of this variable to
"low_mmio_left"
> to distinguish it from generic MMIO, and corrects the logic to only
> subtract the size of the BAR for devices maped in the low MMIO region.
> 
> Also make low_mmio_left unsigned, and don''t allow it to go
negative.
> Since its main use is to be compared to a 64-bit unsigned int, this
> may have undefined (and in practice almost certainly incorrect)
> results.  Not subtracting is OK because if there''s not enough
room, it
> won''t actually be mapped.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Hanweidong <hanweidong@huawei.com>
> ---
>  tools/firmware/hvmloader/pci.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/pci.c
b/tools/firmware/hvmloader/pci.c
> index c78d4d3..8691a19 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -38,11 +38,10 @@ void pci_setup(void)
>  {
>      uint8_t is_64bar, using_64bar, bar64_relocate = 0;
>      uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
> -    uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0;
> +    uint64_t base, bar_sz, bar_sz_upper, low_mmio_left, mmio_total = 0;
>      uint32_t vga_devfn = 256;
>      uint16_t class, vendor_id, device_id;
>      unsigned int bar, pin, link, isa_irq;
> -    int64_t mmio_left;
>  
>      /* Resources assignable to PCI devices via BARs. */
>      struct resource {
> @@ -244,7 +243,7 @@ void pci_setup(void)
>      io_resource.base = 0xc000;
>      io_resource.max = 0x10000;
>  
> -    mmio_left = pci_mem_end - pci_mem_start;
> +    low_mmio_left = pci_mem_end - pci_mem_start;
>  
>      /* Assign iomem and ioport resources in descending order of size. */
>      for ( i = 0; i < nr_bars; i++ )
> @@ -253,7 +252,7 @@ void pci_setup(void)
>          bar_reg = bars[i].bar_reg;
>          bar_sz  = bars[i].bar_sz;
>  
> -        using_64bar = bars[i].is_64bar && bar64_relocate
&& (mmio_left < bar_sz);
> +        using_64bar = bars[i].is_64bar && bar64_relocate
&& (low_mmio_left < bar_sz);
>          bar_data = pci_readl(devfn, bar_reg);
>  
>          if ( (bar_data & PCI_BASE_ADDRESS_SPACE) => @@ -273,9
+272,10 @@ void pci_setup(void)
>              } 
>              else {
>                  resource = &mem_resource;
> +                if ( bar_sz <= low_mmio_left )
> +                    low_mmio_left -= bar_sz;
Why do you need this check? Isn''t the above if(using_64bar &&
(bar_sz >
PCI_MIN_BIG_BAR_SIZE)) enough?

>                  bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
>              }
> -            mmio_left -= bar_sz;
>          }
>          else
>          {
> -- 
> 1.7.9.5
>

On Tue, 18 Jun 2013, George Dunlap wrote:
> Keep track of how much mmio space is left total, as well as the amount
> of "low" MMIO space (<4GiB), and only load devices into high
memory
> if there is not enough low memory for the rest of the devices to fit.
> 
> Because devices are processed by size in order from large to small,
> this should preferentially relocate devices with large BARs to 64-bit
> space.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Hanweidong <hanweidong@huawei.com>
> ---
>  tools/firmware/hvmloader/pci.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/pci.c
b/tools/firmware/hvmloader/pci.c
> index 8691a19..7f306a1 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -38,7 +38,8 @@ void pci_setup(void)
>  {
>      uint8_t is_64bar, using_64bar, bar64_relocate = 0;
>      uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
> -    uint64_t base, bar_sz, bar_sz_upper, low_mmio_left, mmio_total = 0;
> +    uint64_t base, bar_sz, bar_sz_upper, low_mmio_left, mmio_total = 0,
> +        mmio_left;
It think you can just use mmio_total instead of reintroducing mmio_left

>      uint32_t vga_devfn = 256;
>      uint16_t class, vendor_id, device_id;
>      unsigned int bar, pin, link, isa_irq;
> @@ -244,6 +245,7 @@ void pci_setup(void)
>      io_resource.max = 0x10000;
>  
>      low_mmio_left = pci_mem_end - pci_mem_start;
> +    mmio_left = mmio_total;
>  
>      /* Assign iomem and ioport resources in descending order of size. */
>      for ( i = 0; i < nr_bars; i++ )
> @@ -252,7 +254,12 @@ void pci_setup(void)
>          bar_reg = bars[i].bar_reg;
>          bar_sz  = bars[i].bar_sz;
>  
> -        using_64bar = bars[i].is_64bar && bar64_relocate
&& (low_mmio_left < bar_sz);
> +        /* Relocate to high memory if the total amount of MMIO needed
> +         * is more than the low MMIO available.  Because devices are
> +         * processed in order of bar_sz, this will preferentially
> +         * relocate larger devices to high memory first. */
> +        using_64bar = bars[i].is_64bar && bar64_relocate
> +                       && (mmio_left > low_mmio_left);
>          bar_data = pci_readl(devfn, bar_reg);
>  
>          if ( (bar_data & PCI_BASE_ADDRESS_SPACE) => @@ -276,6
+283,7 @@ void pci_setup(void)
>                      low_mmio_left -= bar_sz;
>                  bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
>              }
> +            mmio_left -= bar_sz;
As I wrote above, I would just decrement mmio_total

On Tue, 18 Jun 2013, George Dunlap wrote:
> Allow devices with BARs less than 512MiB to be relocated to high memory.
> 
> This will only be invoked if there is not enough low MMIO space to map
> the device, and will be done preferentially to large devices first; so
> in all likelihood only large devices will be remapped anyway.
> 
> This is needed to work-around the issue of qemu-xen not being able to
> handle moving guest memory around to resize the MMIO hole.  The default
> MMIO hole size is less than 256MiB.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Hanweidong <hanweidong@huawei.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  tools/firmware/hvmloader/config.h |    1 -
>  tools/firmware/hvmloader/pci.c    |    5 ++---
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/config.h
b/tools/firmware/hvmloader/config.h
> index 8143d6f..6641197 100644
> --- a/tools/firmware/hvmloader/config.h
> +++ b/tools/firmware/hvmloader/config.h
> @@ -55,7 +55,6 @@ extern struct bios_config ovmf_config;
>  /* MMIO hole: Hardcoded defaults, which can be dynamically expanded. */
>  #define PCI_MEM_START       0xf0000000
>  #define PCI_MEM_END         0xfc000000
> -#define PCI_MIN_BIG_BAR_SIZE          0x20000000
>  
>  extern unsigned long pci_mem_start, pci_mem_end;
>  
> diff --git a/tools/firmware/hvmloader/pci.c
b/tools/firmware/hvmloader/pci.c
> index 7f306a1..a483b02 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -265,9 +265,8 @@ void pci_setup(void)
>          if ( (bar_data & PCI_BASE_ADDRESS_SPACE) =>              
PCI_BASE_ADDRESS_SPACE_MEMORY )
>          {
> -            /* Mapping high memory if PCI deivce is 64 bits bar and the
bar size
> -               is larger than 512M */
> -            if (using_64bar && (bar_sz > PCI_MIN_BIG_BAR_SIZE))
{
> +            /* Mapping high memory if PCI device is 64 bits bar */
> +            if (using_64bar) {
>                  if ( high_mem_resource.base & (bar_sz - 1) )
>                      high_mem_resource.base = high_mem_resource.base - 
>                          (high_mem_resource.base & (bar_sz - 1)) +
bar_sz;
> -- 
> 1.7.9.5
>

On Tue, 18 Jun 2013, George Dunlap wrote:
> After attempting to resize the MMIO hole, the check to determine
> whether there is a need to relocate BARs into 64-bit space checks the
> specific thing that caused the loop to exit (MMIO hole == 2GiB) rather
> than checking whether the required MMIO will fit in the hole.
> 
> This should have no functional change now, but will make the next patch
> (when we add more conditions for exiting the loop) more clean.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Hanweidong <hanweidong@huawei.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  tools/firmware/hvmloader/pci.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/firmware/hvmloader/pci.c
b/tools/firmware/hvmloader/pci.c
> index a483b02..63d79a2 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -213,7 +213,7 @@ void pci_setup(void)
>              ((pci_mem_start << 1) != 0) )
>          pci_mem_start <<= 1;
>  
> -    if ( (pci_mem_start << 1) != 0 )
> +    if ( mmio_total > (pci_mem_end - pci_mem_start) )
>          bar64_relocate = 1;
>  
>      /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
> -- 
> 1.7.9.5
>

On Tue, 18 Jun 2013, George Dunlap wrote:
> At the moment, qemu-xen can''t handle memory being relocated by
> hvmloader.  This may happen if a device with a large enough memory
> region is passed through to the guest.  At the moment, if this
> happens, then at some point in the future qemu will crash and the
> domain will hang.  (qemu-traditional is fine.)
> 
> It''s too late in the release to do a proper fix, so we try to do
> damage control.
> 
> hvmloader already has mechanisms to relocate memory to 64-bit space
> if it can''t make a big enough MMIO hole.  By default this is 2GiB;
if
> we just refuse to make the hole bigger if it will overlap with guest
> memory, then the relocation will happen by default.
> 
> v2:
>  - style fixes
>  - fix and expand comment on the MMIO hole loop
>  - use "%d" rather than "%s" ->
(...)?"1":"0"
>  - use bool instead of uint8_t
>  - Move 64-bit bar relocate detection to another patch
>  - Add more diagnostic messages
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Hanweidong <hanweidong@huawei.com>
> ---
>  tools/firmware/hvmloader/pci.c          |   41
++++++++++++++++++++++++++++---
>  tools/libxl/libxl_dm.c                  |    6 +++++
>  xen/include/public/hvm/hvm_xs_strings.h |    1 +
>  3 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/pci.c
b/tools/firmware/hvmloader/pci.c
> index 63d79a2..1ab5124 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -27,6 +27,8 @@
>  
>  #include <xen/memory.h>
>  #include <xen/hvm/ioreq.h>
> +#include <xen/hvm/hvm_xs_strings.h>
> +#include <stdbool.h>
>  
>  unsigned long pci_mem_start = PCI_MEM_START;
>  unsigned long pci_mem_end = PCI_MEM_END;
> @@ -58,6 +60,15 @@ void pci_setup(void)
>      } *bars = (struct bars *)scratch_start;
>      unsigned int i, nr_bars = 0;
>  
> +    const char *s;
> +    bool allow_memory_relocate = 1;
Arguably the default should be 0, given that the default device model is
qemu-xen that cannot cope with memory relocation.

> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
> +    if ( s )
> +        allow_memory_relocate = (bool)strtoll(s, NULL, 0);
> +    printf("Relocating guest memory for lowmem MMIO space %s\n",
> +          
allow_memory_relocate?"enabled":"disabled");
It doesn''t take a strtoll to parse a boolean.


>      /* Program PCI-ISA bridge with appropriate link routes. */
>      isa_irq = 0;
>      for ( link = 0; link < 4; link++ )
> @@ -209,14 +220,38 @@ void pci_setup(void)
>          pci_writew(devfn, PCI_COMMAND, cmd);
>      }
>  
> -    while ( (mmio_total > (pci_mem_end - pci_mem_start)) &&
> -            ((pci_mem_start << 1) != 0) )
> +    /*
> +     * At the moment qemu-xen can''t deal with relocated memory
regions.
> +     * It''s too close to the release to make a proper fix; for
now,
> +     * only allow the MMIO hole to grow large enough to move guest memory
> +     * if we''re running qemu-traditional.  Items that
don''t fit will be
> +     * relocated into the 64-bit address space.
I would avoid mentioning release issues in a comment within the code.

> +     * This loop now does the following:
> +     * - If allow_memory_relocate, increase the MMIO hole until
it''s
> +     *   big enough, or until it''s 2GiB
> +     * - If !allow_memory_relocate, increase the MMIO hole until
it''s
> +     *   big enough, or until it''s 2GiB, or until it overlaps
guest
> +     *   memory
> +     */
> +    while ( (mmio_total > (pci_mem_end - pci_mem_start)) 
> +            && ((pci_mem_start << 1) != 0)
> +            && (allow_memory_relocate
> +                || (((pci_mem_start << 1) >> PAGE_SHIFT)
> +                    < hvm_info->low_mem_pgend)) )
Isn''t this last condition inverted? It should be
''>='' ?
>          pci_mem_start <<= 1;
>  
> -    if ( mmio_total > (pci_mem_end - pci_mem_start) )
> +    if ( mmio_total > (pci_mem_end - pci_mem_start) ) {
> +        printf("<4GiB MMIO hole not large enough,"
> +               " relocating some BARs to 64-bit\n");
>          bar64_relocate = 1;
> +    }
>  
>      /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
> +    if ( (pci_mem_start >> PAGE_SHIFT) <
hvm_info->low_mem_pgend )
> +        printf("Relocating 0x%lx pages to highmem for lowmem MMIO
hole\n",
> +               hvm_info->low_mem_pgend - (pci_mem_start >>
PAGE_SHIFT));
> +
>      while ( (pci_mem_start >> PAGE_SHIFT) <
hvm_info->low_mem_pgend )
>      {
>          struct xen_add_to_physmap xatp;
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index ac1f90e..342d2ce 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1154,6 +1154,12 @@ void libxl__spawn_local_dm(libxl__egc *egc,
libxl__dm_spawn_state *dmss)
>          libxl__xs_write(gc, XBT_NULL,
>                          libxl__sprintf(gc, "%s/hvmloader/bios",
path),
>                          "%s",
libxl_bios_type_to_string(b_info->u.hvm.bios));
> +        /* Disable relocating memory to make the MMIO hole larger
> +         * unless we''re running qemu-traditional */
> +        libxl__xs_write(gc, XBT_NULL,
> +                        libxl__sprintf(gc,
"%s/hvmloader/allow-memory-relocate", path),
> +                        "%d",
> +                       
b_info->device_model_version==LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL);
>          free(path);
>      }
>  
> diff --git a/xen/include/public/hvm/hvm_xs_strings.h
b/xen/include/public/hvm/hvm_xs_strings.h
> index 9042303..4de5881 100644
> --- a/xen/include/public/hvm/hvm_xs_strings.h
> +++ b/xen/include/public/hvm/hvm_xs_strings.h
> @@ -28,6 +28,7 @@
>  #define HVM_XS_HVMLOADER               "hvmloader"
>  #define HVM_XS_BIOS                    "hvmloader/bios"
>  #define HVM_XS_GENERATION_ID_ADDRESS  
"hvmloader/generation-id-address"
> +#define HVM_XS_ALLOW_MEMORY_RELOCATE  
"hvmloader/allow-memory-relocate"
>  
>  /* The following values allow additional ACPI tables to be added to the
>   * virtual ACPI BIOS that hvmloader constructs. The values specify the
guest
> -- 
> 1.7.9.5
>

On Tue, Jun 18, 2013 at 05:46:22PM +0100, George Dunlap
wrote:
> Allow devices with BARs less than 512MiB to be relocated to high memory.
> 
> This will only be invoked if there is not enough low MMIO space to map
> the device, and will be done preferentially to large devices first; so
> in all likelihood only large devices will be remapped anyway.
> 
> This is needed to work-around the issue of qemu-xen not being able to
> handle moving guest memory around to resize the MMIO hole.  The default
> MMIO hole size is less than 256MiB.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Hanweidong <hanweidong@huawei.com>
> ---
>  tools/firmware/hvmloader/config.h |    1 -
>  tools/firmware/hvmloader/pci.c    |    5 ++---
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/config.h
b/tools/firmware/hvmloader/config.h
> index 8143d6f..6641197 100644
> --- a/tools/firmware/hvmloader/config.h
> +++ b/tools/firmware/hvmloader/config.h
> @@ -55,7 +55,6 @@ extern struct bios_config ovmf_config;
>  /* MMIO hole: Hardcoded defaults, which can be dynamically expanded. */
>  #define PCI_MEM_START       0xf0000000
>  #define PCI_MEM_END         0xfc000000
> -#define PCI_MIN_BIG_BAR_SIZE          0x20000000
>  
>  extern unsigned long pci_mem_start, pci_mem_end;
>  
> diff --git a/tools/firmware/hvmloader/pci.c
b/tools/firmware/hvmloader/pci.c
> index 7f306a1..a483b02 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -265,9 +265,8 @@ void pci_setup(void)
>          if ( (bar_data & PCI_BASE_ADDRESS_SPACE) =>              
PCI_BASE_ADDRESS_SPACE_MEMORY )
>          {
> -            /* Mapping high memory if PCI deivce is 64 bits bar and the
bar size
> -               is larger than 512M */
> -            if (using_64bar && (bar_sz > PCI_MIN_BIG_BAR_SIZE))
{
> +            /* Mapping high memory if PCI device is 64 bits bar */
> +            if (using_64bar) {
I think the original had style problem, if you look at other occurrences
for ''if'', it should be

  if ( condition )
      statement;


Wei.
>                  if ( high_mem_resource.base & (bar_sz - 1) )
>                      high_mem_resource.base = high_mem_resource.base - 
>                          (high_mem_resource.base & (bar_sz - 1)) +
bar_sz;
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On 19/06/13 18:18, Stefano Stabellini wrote:
> On Tue, 18 Jun 2013, George Dunlap wrote:
>> When deciding whether to map a device in low MMIO space (<4GiB),
>> hvmloader compares it with "mmio_left", which is set to the
size of
>> the low MMIO range (pci_mem_end - pci_mem_start).  However, even if it
>> does map a device in high MMIO space, it still removes the size of its
>> BAR from mmio_left.
>>
>> This patch first changes the name of this variable to
"low_mmio_left"
>> to distinguish it from generic MMIO, and corrects the logic to only
>> subtract the size of the BAR for devices maped in the low MMIO region.
>>
>> Also make low_mmio_left unsigned, and don''t allow it to go
negative.
>> Since its main use is to be compared to a 64-bit unsigned int, this
>> may have undefined (and in practice almost certainly incorrect)
>> results.  Not subtracting is OK because if there''s not enough
room, it
>> won''t actually be mapped.
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>> CC: Hanweidong <hanweidong@huawei.com>
>> ---
>>   tools/firmware/hvmloader/pci.c |   10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/firmware/hvmloader/pci.c
b/tools/firmware/hvmloader/pci.c
>> index c78d4d3..8691a19 100644
>> --- a/tools/firmware/hvmloader/pci.c
>> +++ b/tools/firmware/hvmloader/pci.c
>> @@ -38,11 +38,10 @@ void pci_setup(void)
>>   {
>>       uint8_t is_64bar, using_64bar, bar64_relocate = 0;
>>       uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
>> -    uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0;
>> +    uint64_t base, bar_sz, bar_sz_upper, low_mmio_left, mmio_total =
0;
>>       uint32_t vga_devfn = 256;
>>       uint16_t class, vendor_id, device_id;
>>       unsigned int bar, pin, link, isa_irq;
>> -    int64_t mmio_left;
>>   
>>       /* Resources assignable to PCI devices via BARs. */
>>       struct resource {
>> @@ -244,7 +243,7 @@ void pci_setup(void)
>>       io_resource.base = 0xc000;
>>       io_resource.max = 0x10000;
>>   
>> -    mmio_left = pci_mem_end - pci_mem_start;
>> +    low_mmio_left = pci_mem_end - pci_mem_start;
>>   
>>       /* Assign iomem and ioport resources in descending order of size.
*/
>>       for ( i = 0; i < nr_bars; i++ )
>> @@ -253,7 +252,7 @@ void pci_setup(void)
>>           bar_reg = bars[i].bar_reg;
>>           bar_sz  = bars[i].bar_sz;
>>   
>> -        using_64bar = bars[i].is_64bar && bar64_relocate
&& (mmio_left < bar_sz);
>> +        using_64bar = bars[i].is_64bar && bar64_relocate
&& (low_mmio_left < bar_sz);
>>           bar_data = pci_readl(devfn, bar_reg);
>>   
>>           if ( (bar_data & PCI_BASE_ADDRESS_SPACE) =>> @@
-273,9 +272,10 @@ void pci_setup(void)
>>               }
>>               else {
>>                   resource = &mem_resource;
>> +                if ( bar_sz <= low_mmio_left )
>> +                    low_mmio_left -= bar_sz;
> Why do you need this check? Isn''t the above if(using_64bar
&& (bar_sz >
> PCI_MIN_BIG_BAR_SIZE)) enough?
This is in the lowmem region.  There may be regions which can''t be 
relocated to the high PCI region that nevertheless don''t fit in the low
PCI region.  If it doesn''t fit, it will hit the "no space for
resource"
conditional below and not be mapped; we need to make sure not to 
subtract it off.

I suppose a more robust method might be to use resource->max - 
resource->base instead of keeping a separate accounting... I had 
originally thought that would be too invasive a change, but I''m not so 
sure now... any thoughts?

  -George

On 19/06/13 22:14, Wei Liu wrote:
> On Tue, Jun 18, 2013 at 05:46:22PM +0100, George Dunlap wrote:
>> Allow devices with BARs less than 512MiB to be relocated to high
memory.
>>
>> This will only be invoked if there is not enough low MMIO space to map
>> the device, and will be done preferentially to large devices first; so
>> in all likelihood only large devices will be remapped anyway.
>>
>> This is needed to work-around the issue of qemu-xen not being able to
>> handle moving guest memory around to resize the MMIO hole.  The default
>> MMIO hole size is less than 256MiB.
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>> CC: Hanweidong <hanweidong@huawei.com>
>> ---
>>   tools/firmware/hvmloader/config.h |    1 -
>>   tools/firmware/hvmloader/pci.c    |    5 ++---
>>   2 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/firmware/hvmloader/config.h
b/tools/firmware/hvmloader/config.h
>> index 8143d6f..6641197 100644
>> --- a/tools/firmware/hvmloader/config.h
>> +++ b/tools/firmware/hvmloader/config.h
>> @@ -55,7 +55,6 @@ extern struct bios_config ovmf_config;
>>   /* MMIO hole: Hardcoded defaults, which can be dynamically expanded.
*/
>>   #define PCI_MEM_START       0xf0000000
>>   #define PCI_MEM_END         0xfc000000
>> -#define PCI_MIN_BIG_BAR_SIZE          0x20000000
>>   
>>   extern unsigned long pci_mem_start, pci_mem_end;
>>   
>> diff --git a/tools/firmware/hvmloader/pci.c
b/tools/firmware/hvmloader/pci.c
>> index 7f306a1..a483b02 100644
>> --- a/tools/firmware/hvmloader/pci.c
>> +++ b/tools/firmware/hvmloader/pci.c
>> @@ -265,9 +265,8 @@ void pci_setup(void)
>>           if ( (bar_data & PCI_BASE_ADDRESS_SPACE) =>>       
PCI_BASE_ADDRESS_SPACE_MEMORY )
>>           {
>> -            /* Mapping high memory if PCI deivce is 64 bits bar and
the bar size
>> -               is larger than 512M */
>> -            if (using_64bar && (bar_sz >
PCI_MIN_BIG_BAR_SIZE)) {
>> +            /* Mapping high memory if PCI device is 64 bits bar */
>> +            if (using_64bar) {
> I think the original had style problem, if you look at other occurrences
> for ''if'', it should be
>
>    if ( condition )
>        statement;
Ack.

  -George

On 19/06/13 18:18, Stefano Stabellini wrote:
> On Tue, 18 Jun 2013, George Dunlap wrote:
>> +    const char *s;
>> +    bool allow_memory_relocate = 1;
> Arguably the default should be 0, given that the default device model is
> qemu-xen that cannot cope with memory relocation.
OK, so time to think a bit harder about this. This will only matter if 
someone is using this hvmloader with a non-libxl toolstack which 
includes xend, or a home-grown one.

* If we default to 1, then:
  - VMs running qemu-traditional will be have exactly as before
  - VMs running qemu-xen will have the risk of crashing mysteriously.
  - If qemu-xen is the default, then there is a work-around: run 
qemu-traditional

* If we default to 0, then:
  - VMs running qemu-xen will be fine
  - VMs running qemu-traditional may have strange problems; we haven''t 
tested relocating things into 64-bit with qemu-tradiational.
  - There is no work-around available; if the device either can''t be 
relocated, or the OS / qemu can''t handle the relocation, then the user 
is just hosed.

Furthermore, I think xm defaults to qemu-traditional, right?  Does xm 
even know how to drive qemu-xen?  If it does default to 
qemu-traditional, defaulting to 0 will pretty much guarantee a whole 
slew of currently-working configurations will be affected (perhaps 
break, perhaps not).

Overall I think defaulting to 1 is probably the lowest-risk option.
>
>
>> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
>> +    if ( s )
>> +        allow_memory_relocate = (bool)strtoll(s, NULL, 0);
>> +    printf("Relocating guest memory for lowmem MMIO space
%s\n",
>> +          
allow_memory_relocate?"enabled":"disabled");
> It doesn''t take a strtoll to parse a boolean.
As discussed in v1, strtoll is the only "XtoY" function available in 
hvmloader. :-)  The only other option would be to explicitly compare for 
"1" or "0" (or do some half-baked *s-''0''
thing).

This does make me think though -- what is the semantics of casting to a 
bool?  Is it !!, or will it essentially clip off the high bits?  (e.g., 
would "2" become "1", or "0"?)
>>       /* Program PCI-ISA bridge with appropriate link routes. */
>>       isa_irq = 0;
>>       for ( link = 0; link < 4; link++ )
>> @@ -209,14 +220,38 @@ void pci_setup(void)
>>           pci_writew(devfn, PCI_COMMAND, cmd);
>>       }
>>   
>> -    while ( (mmio_total > (pci_mem_end - pci_mem_start)) &&
>> -            ((pci_mem_start << 1) != 0) )
>> +    /*
>> +     * At the moment qemu-xen can''t deal with relocated
memory regions.
>> +     * It''s too close to the release to make a proper fix;
for now,
>> +     * only allow the MMIO hole to grow large enough to move guest
memory
>> +     * if we''re running qemu-traditional.  Items that
don''t fit will be
>> +     * relocated into the 64-bit address space.
> I would avoid mentioning release issues in a comment within the code.
I guess it depends on whether we intend to keep this change or to get 
rid of it once the 4.4. window opens. If we intend to get rid of it, 
then I think the comment should stay; if for some reason someone comes 
along later and and wants to know, "Can I change this?" Knowing that
it
was only meant to be a temporary measure is important information.

Really, I''m of the opinion that if KVM is using SeaBIOS''s pci
routines,
we should just move do the same.  No sense in duplicating the effort for 
something like this.
>> +     * This loop now does the following:
>> +     * - If allow_memory_relocate, increase the MMIO hole until
it''s
>> +     *   big enough, or until it''s 2GiB
>> +     * - If !allow_memory_relocate, increase the MMIO hole until
it''s
>> +     *   big enough, or until it''s 2GiB, or until it overlaps
guest
>> +     *   memory
>> +     */
>> +    while ( (mmio_total > (pci_mem_end - pci_mem_start))
>> +            && ((pci_mem_start << 1) != 0)
>> +            && (allow_memory_relocate
>> +                || (((pci_mem_start << 1) >> PAGE_SHIFT)
>> +                    < hvm_info->low_mem_pgend)) )
> Isn''t this last condition inverted? It should be
''>='' ?
Yep -- replied to myself Tuesday saying as much. :-)

Good catch though -- thanks for the close review.

  -George

On 19/06/13 18:18, Stefano Stabellini wrote:
> On Tue, 18 Jun 2013, George Dunlap wrote:
>> Keep track of how much mmio space is left total, as well as the amount
>> of "low" MMIO space (<4GiB), and only load devices into
high memory
>> if there is not enough low memory for the rest of the devices to fit.
>>
>> Because devices are processed by size in order from large to small,
>> this should preferentially relocate devices with large BARs to 64-bit
>> space.
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>> CC: Hanweidong <hanweidong@huawei.com>
>> ---
>>   tools/firmware/hvmloader/pci.c |   12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/firmware/hvmloader/pci.c
b/tools/firmware/hvmloader/pci.c
>> index 8691a19..7f306a1 100644
>> --- a/tools/firmware/hvmloader/pci.c
>> +++ b/tools/firmware/hvmloader/pci.c
>> @@ -38,7 +38,8 @@ void pci_setup(void)
>>   {
>>       uint8_t is_64bar, using_64bar, bar64_relocate = 0;
>>       uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
>> -    uint64_t base, bar_sz, bar_sz_upper, low_mmio_left, mmio_total =
0;
>> +    uint64_t base, bar_sz, bar_sz_upper, low_mmio_left, mmio_total =
0,
>> +        mmio_left;
> It think you can just use mmio_total instead of reintroducing mmio_left
Ack.

  -George

>>> On 18.06.13 at 18:46, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> When deciding whether to map a device in low MMIO space (<4GiB),
> hvmloader compares it with "mmio_left", which is set to the size
of
> the low MMIO range (pci_mem_end - pci_mem_start).  However, even if it
> does map a device in high MMIO space, it still removes the size of its
> BAR from mmio_left.
> 
> This patch first changes the name of this variable to
"low_mmio_left"
> to distinguish it from generic MMIO, and corrects the logic to only
> subtract the size of the BAR for devices maped in the low MMIO region.
> 
> Also make low_mmio_left unsigned, and don''t allow it to go
negative.
> Since its main use is to be compared to a 64-bit unsigned int, this
> may have undefined (and in practice almost certainly incorrect)
> results.  Not subtracting is OK because if there''s not enough
room, it
> won''t actually be mapped.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Hanweidong <hanweidong@huawei.com>
> ---
>  tools/firmware/hvmloader/pci.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/pci.c
b/tools/firmware/hvmloader/pci.c
> index c78d4d3..8691a19 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -38,11 +38,10 @@ void pci_setup(void)
>  {
>      uint8_t is_64bar, using_64bar, bar64_relocate = 0;
>      uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
> -    uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0;
> +    uint64_t base, bar_sz, bar_sz_upper, low_mmio_left, mmio_total = 0;
>      uint32_t vga_devfn = 256;
>      uint16_t class, vendor_id, device_id;
>      unsigned int bar, pin, link, isa_irq;
> -    int64_t mmio_left;
>  
>      /* Resources assignable to PCI devices via BARs. */
>      struct resource {
> @@ -244,7 +243,7 @@ void pci_setup(void)
>      io_resource.base = 0xc000;
>      io_resource.max = 0x10000;
>  
> -    mmio_left = pci_mem_end - pci_mem_start;
> +    low_mmio_left = pci_mem_end - pci_mem_start;
>  
>      /* Assign iomem and ioport resources in descending order of size. */
>      for ( i = 0; i < nr_bars; i++ )
> @@ -253,7 +252,7 @@ void pci_setup(void)
>          bar_reg = bars[i].bar_reg;
>          bar_sz  = bars[i].bar_sz;
>  
> -        using_64bar = bars[i].is_64bar && bar64_relocate
&& (mmio_left < bar_sz);
> +        using_64bar = bars[i].is_64bar && bar64_relocate
&& (low_mmio_left <
> bar_sz);
>          bar_data = pci_readl(devfn, bar_reg);
>  
>          if ( (bar_data & PCI_BASE_ADDRESS_SPACE) => @@ -273,9
+272,10 @@ void pci_setup(void)
>              } 
>              else {
>                  resource = &mem_resource;
> +                if ( bar_sz <= low_mmio_left )
> +                    low_mmio_left -= bar_sz;
>                  bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
>              }
> -            mmio_left -= bar_sz;
>          }
>          else
>          {
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

>>> On 18.06.13 at 18:46, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> Keep track of how much mmio space is left total, as well as the amount
> of "low" MMIO space (<4GiB), and only load devices into high
memory
> if there is not enough low memory for the rest of the devices to fit.
> 
> Because devices are processed by size in order from large to small,
> this should preferentially relocate devices with large BARs to 64-bit
> space.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Hanweidong <hanweidong@huawei.com>
> ---
>  tools/firmware/hvmloader/pci.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/pci.c
b/tools/firmware/hvmloader/pci.c
> index 8691a19..7f306a1 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -38,7 +38,8 @@ void pci_setup(void)
>  {
>      uint8_t is_64bar, using_64bar, bar64_relocate = 0;
>      uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
> -    uint64_t base, bar_sz, bar_sz_upper, low_mmio_left, mmio_total = 0;
> +    uint64_t base, bar_sz, bar_sz_upper, low_mmio_left, mmio_total = 0,
> +        mmio_left;
>      uint32_t vga_devfn = 256;
>      uint16_t class, vendor_id, device_id;
>      unsigned int bar, pin, link, isa_irq;
> @@ -244,6 +245,7 @@ void pci_setup(void)
>      io_resource.max = 0x10000;
>  
>      low_mmio_left = pci_mem_end - pci_mem_start;
> +    mmio_left = mmio_total;
>  
>      /* Assign iomem and ioport resources in descending order of size. */
>      for ( i = 0; i < nr_bars; i++ )
> @@ -252,7 +254,12 @@ void pci_setup(void)
>          bar_reg = bars[i].bar_reg;
>          bar_sz  = bars[i].bar_sz;
>  
> -        using_64bar = bars[i].is_64bar && bar64_relocate
&& (low_mmio_left <
> bar_sz);
> +        /* Relocate to high memory if the total amount of MMIO needed
> +         * is more than the low MMIO available.  Because devices are
> +         * processed in order of bar_sz, this will preferentially
> +         * relocate larger devices to high memory first. */
> +        using_64bar = bars[i].is_64bar && bar64_relocate
> +                       && (mmio_left > low_mmio_left);
>          bar_data = pci_readl(devfn, bar_reg);
>  
>          if ( (bar_data & PCI_BASE_ADDRESS_SPACE) => @@ -276,6
+283,7 @@ void pci_setup(void)
>                      low_mmio_left -= bar_sz;
>                  bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
>              }
> +            mmio_left -= bar_sz;
>          }
>          else
>          {
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

>>> On 18.06.13 at 18:46, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> Allow devices with BARs less than 512MiB to be relocated to high memory.
> 
> This will only be invoked if there is not enough low MMIO space to map
> the device, and will be done preferentially to large devices first; so
> in all likelihood only large devices will be remapped anyway.
> 
> This is needed to work-around the issue of qemu-xen not being able to
> handle moving guest memory around to resize the MMIO hole.  The default
> MMIO hole size is less than 256MiB.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Hanweidong <hanweidong@huawei.com>
> ---
>  tools/firmware/hvmloader/config.h |    1 -
>  tools/firmware/hvmloader/pci.c    |    5 ++---
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/config.h 
> b/tools/firmware/hvmloader/config.h
> index 8143d6f..6641197 100644
> --- a/tools/firmware/hvmloader/config.h
> +++ b/tools/firmware/hvmloader/config.h
> @@ -55,7 +55,6 @@ extern struct bios_config ovmf_config;
>  /* MMIO hole: Hardcoded defaults, which can be dynamically expanded. */
>  #define PCI_MEM_START       0xf0000000
>  #define PCI_MEM_END         0xfc000000
> -#define PCI_MIN_BIG_BAR_SIZE          0x20000000
>  
>  extern unsigned long pci_mem_start, pci_mem_end;
>  
> diff --git a/tools/firmware/hvmloader/pci.c
b/tools/firmware/hvmloader/pci.c
> index 7f306a1..a483b02 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -265,9 +265,8 @@ void pci_setup(void)
>          if ( (bar_data & PCI_BASE_ADDRESS_SPACE) =>              
PCI_BASE_ADDRESS_SPACE_MEMORY )
>          {
> -            /* Mapping high memory if PCI deivce is 64 bits bar and the
bar
> size
> -               is larger than 512M */
> -            if (using_64bar && (bar_sz > PCI_MIN_BIG_BAR_SIZE))
{
> +            /* Mapping high memory if PCI device is 64 bits bar */
> +            if (using_64bar) {
>                  if ( high_mem_resource.base & (bar_sz - 1) )
>                      high_mem_resource.base = high_mem_resource.base - 
>                          (high_mem_resource.base & (bar_sz - 1)) +
bar_sz;
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

>>> On 18.06.13 at 18:46, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> After attempting to resize the MMIO hole, the check to determine
> whether there is a need to relocate BARs into 64-bit space checks the
> specific thing that caused the loop to exit (MMIO hole == 2GiB) rather
> than checking whether the required MMIO will fit in the hole.
> 
> This should have no functional change now, but will make the next patch
> (when we add more conditions for exiting the loop) more clean.
This does change functionality, but in a desirable way: With what
the code did before, even if pci_mem_start remained at its start
value, bar64_relocate would end up getting set to 1, while it would
be left at 0 when pci_mem_start got lowered down to 2Gb. I.e. it
seems to me that the original condition was inverted.
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

With the above I even think this is a backporting candidate
(irrespective of eventual considerations of this kind for the full
series).

Jan
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Hanweidong <hanweidong@huawei.com>
> ---
>  tools/firmware/hvmloader/pci.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/firmware/hvmloader/pci.c
b/tools/firmware/hvmloader/pci.c
> index a483b02..63d79a2 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -213,7 +213,7 @@ void pci_setup(void)
>              ((pci_mem_start << 1) != 0) )
>          pci_mem_start <<= 1;
>  
> -    if ( (pci_mem_start << 1) != 0 )
> +    if ( mmio_total > (pci_mem_end - pci_mem_start) )
>          bar64_relocate = 1;
>  
>      /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

>>> On 20.06.13 at 11:22, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> On 19/06/13 18:18, Stefano Stabellini wrote:
>> On Tue, 18 Jun 2013, George Dunlap wrote:
>>> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
>>> +    if ( s )
>>> +        allow_memory_relocate = (bool)strtoll(s, NULL, 0);
>>> +    printf("Relocating guest memory for lowmem MMIO space
%s\n",
>>> +          
allow_memory_relocate?"enabled":"disabled");
>> It doesn''t take a strtoll to parse a boolean.
> 
> As discussed in v1, strtoll is the only "XtoY" function available
in
> hvmloader. :-)  The only other option would be to explicitly compare for 
> "1" or "0" (or do some half-baked
*s-''0'' thing).
> 
> This does make me think though -- what is the semantics of casting to a 
> bool?  Is it !!, or will it essentially clip off the high bits?  (e.g., 
> would "2" become "1", or "0"?)
If bool is a typedef or #define of _Bool, and _Bool is a complier
supplied type, then the cast will do the right thing. But doing the
assignment without the cast would too, i.e. the cast is pointless
(as I think IanJ had already pointed out).

However, if we want to be on the safe side and also make the
code work with a compiler that doesn''t have a built-in _Bool,
I''d
think

    allow_memory_relocate = !s || strtoll(s, NULL, 0);

would be the better statement (without any if() surrounding it,
and without the variable declaration having an initializer.

Jan

On 20/06/13 11:12, Jan Beulich wrote:
>>>> On 20.06.13 at 11:22, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
>> On 19/06/13 18:18, Stefano Stabellini wrote:
>>> On Tue, 18 Jun 2013, George Dunlap wrote:
>>>> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
>>>> +    if ( s )
>>>> +        allow_memory_relocate = (bool)strtoll(s, NULL, 0);
>>>> +    printf("Relocating guest memory for lowmem MMIO space
%s\n",
>>>> +          
allow_memory_relocate?"enabled":"disabled");
>>> It doesn''t take a strtoll to parse a boolean.
>> As discussed in v1, strtoll is the only "XtoY" function
available in
>> hvmloader. :-)  The only other option would be to explicitly compare
for
>> "1" or "0" (or do some half-baked
*s-''0'' thing).
>>
>> This does make me think though -- what is the semantics of casting to a
>> bool?  Is it !!, or will it essentially clip off the high bits?  (e.g.,
>> would "2" become "1", or "0"?)
> If bool is a typedef or #define of _Bool, and _Bool is a complier
> supplied type, then the cast will do the right thing. But doing the
> assignment without the cast would too, i.e. the cast is pointless
> (as I think IanJ had already pointed out).
Thanks for the info.

It may be pointless from a functionality perspective, but it''s also 
harmless.  It won''t add a single byte to the compiled code, but the 6 
characters will remind a developer reading the source that there is a 
cast being done here, just in case it should ever become important.  Not 
super important, but I''d rather leave it in.
> However, if we want to be on the safe side and also make the
> code work with a compiler that doesn''t have a built-in _Bool,
I''d
> think
>
>      allow_memory_relocate = !s || strtoll(s, NULL, 0);
>
> would be the better statement (without any if() surrounding it,
> and without the variable declaration having an initializer.
Doing this would effectively hide the "default" value.  This is bad 
because 1) it''s not clear what the default is to someone just scanning 
the code, 2) it''s hard to change.  (Consider how you''d modify
the above
statement if you wanted to default to 0 instead.)

  -George

On 20/06/13 11:01, Jan Beulich wrote:
>>>> On 18.06.13 at 18:46, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
>> After attempting to resize the MMIO hole, the check to determine
>> whether there is a need to relocate BARs into 64-bit space checks the
>> specific thing that caused the loop to exit (MMIO hole == 2GiB) rather
>> than checking whether the required MMIO will fit in the hole.
>>
>> This should have no functional change now, but will make the next patch
>> (when we add more conditions for exiting the loop) more clean.
> This does change functionality, but in a desirable way: With what
> the code did before, even if pci_mem_start remained at its start
> value, bar64_relocate would end up getting set to 1, while it would
> be left at 0 when pci_mem_start got lowered down to 2Gb. I.e. it
> seems to me that the original condition was inverted.
Oh -- good point.  But the relocation was never triggered before because 
bar_sz was always < mmio_left.

In that case, this patch should go earlier in the series, before we 
actually start doing functional changes.

  -George

On Thu, 20 Jun 2013, George Dunlap wrote:
> On 20/06/13 11:12, Jan Beulich wrote:
> > > > > On 20.06.13 at 11:22, George Dunlap
<george.dunlap@eu.citrix.com>
> > > > > wrote:
> > > On 19/06/13 18:18, Stefano Stabellini wrote:
> > > > On Tue, 18 Jun 2013, George Dunlap wrote:
> > > > > +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE,
NULL);
> > > > > +    if ( s )
> > > > > +        allow_memory_relocate = (bool)strtoll(s, NULL,
0);
> > > > > +    printf("Relocating guest memory for lowmem
MMIO space %s\n",
> > > > > +          
allow_memory_relocate?"enabled":"disabled");
> > > > It doesn''t take a strtoll to parse a boolean.
> > > As discussed in v1, strtoll is the only "XtoY" function
available in
> > > hvmloader. :-)  The only other option would be to explicitly
compare for
> > > "1" or "0" (or do some half-baked
*s-''0'' thing).
> > > 
> > > This does make me think though -- what is the semantics of
casting to a
> > > bool?  Is it !!, or will it essentially clip off the high bits? 
(e.g.,
> > > would "2" become "1", or "0"?)
> > If bool is a typedef or #define of _Bool, and _Bool is a complier
> > supplied type, then the cast will do the right thing. But doing the
> > assignment without the cast would too, i.e. the cast is pointless
> > (as I think IanJ had already pointed out).
> 
> Thanks for the info.
> 
> It may be pointless from a functionality perspective, but it''s
also harmless.
> It won''t add a single byte to the compiled code, but the 6
characters will
> remind a developer reading the source that there is a cast being done here,
> just in case it should ever become important.  Not super important, but
I''d
> rather leave it in.
> 
> > However, if we want to be on the safe side and also make the
> > code work with a compiler that doesn''t have a built-in _Bool,
I''d
> > think
> > 
> >      allow_memory_relocate = !s || strtoll(s, NULL, 0);
> > 
> > would be the better statement (without any if() surrounding it,
> > and without the variable declaration having an initializer.
> 
> Doing this would effectively hide the "default" value.  This is
bad because 1)
> it''s not clear what the default is to someone just scanning the
code, 2) it''s
> hard to change.  (Consider how you''d modify the above statement if
you wanted
> to default to 0 instead.)
I would avoid the strtoll altogether:

if (s != NULL && s[0] != ''0'')
    allow_memory_relocate = 1;
else
    allow_memory_relocate = 0;

George Dunlap writes ("Re: [Xen-devel] [PATCH v2 5/5] libxl, hvmloader:
Don''t relocate memory for MMIO hole"):
> It may be pointless from a functionality perspective, but it''s
also
> harmless.  It won''t add a single byte to the compiled code, but
the 6
> characters will remind a developer reading the source that there is a 
> cast being done here, just in case it should ever become important.  Not 
> super important, but I''d rather leave it in.
IMO this is a terrible reason for a cast.  Casts are dangerous things
to have in code - they can override the compiler''s normal
typechecking.  They should be used only when actually necessary, and
code should normally be constructed so that they aren''t.
> Doing this would effectively hide the "default" value.  This is
bad
> because 1) it''s not clear what the default is to someone just
scanning
> the code, 2) it''s hard to change.  (Consider how you''d
modify the above
> statement if you wanted to default to 0 instead.)
I agree with this complaint.

A straight assignment to _Bool is guaranteed to DTRT.  If that''s too
opaque because the type of the variable is hidden elsewhere, you can
use the boolean canonicalisation operator, !!.

Ian.

On Thu, 20 Jun 2013, George Dunlap wrote:
> On 19/06/13 18:18, Stefano Stabellini wrote:
> > On Tue, 18 Jun 2013, George Dunlap wrote:
> > > When deciding whether to map a device in low MMIO space
(<4GiB),
> > > hvmloader compares it with "mmio_left", which is set to
the size of
> > > the low MMIO range (pci_mem_end - pci_mem_start).  However, even
if it
> > > does map a device in high MMIO space, it still removes the size
of its
> > > BAR from mmio_left.
> > > 
> > > This patch first changes the name of this variable to
"low_mmio_left"
> > > to distinguish it from generic MMIO, and corrects the logic to
only
> > > subtract the size of the BAR for devices maped in the low MMIO
region.
> > > 
> > > Also make low_mmio_left unsigned, and don''t allow it to
go negative.
> > > Since its main use is to be compared to a 64-bit unsigned int,
this
> > > may have undefined (and in practice almost certainly incorrect)
> > > results.  Not subtracting is OK because if there''s not
enough room, it
> > > won''t actually be mapped.
> > > 
> > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > > CC: Ian Jackson <ian.jackson@citrix.com>
> > > CC: Ian Campbell <ian.campbell@citrix.com>
> > > CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> > > CC: Hanweidong <hanweidong@huawei.com>
> > > ---
> > >   tools/firmware/hvmloader/pci.c |   10 +++++-----
> > >   1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/tools/firmware/hvmloader/pci.c
> > > b/tools/firmware/hvmloader/pci.c
> > > index c78d4d3..8691a19 100644
> > > --- a/tools/firmware/hvmloader/pci.c
> > > +++ b/tools/firmware/hvmloader/pci.c
> > > @@ -38,11 +38,10 @@ void pci_setup(void)
> > >   {
> > >       uint8_t is_64bar, using_64bar, bar64_relocate = 0;
> > >       uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
> > > -    uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0;
> > > +    uint64_t base, bar_sz, bar_sz_upper, low_mmio_left,
mmio_total = 0;
> > >       uint32_t vga_devfn = 256;
> > >       uint16_t class, vendor_id, device_id;
> > >       unsigned int bar, pin, link, isa_irq;
> > > -    int64_t mmio_left;
> > >         /* Resources assignable to PCI devices via BARs. */
> > >       struct resource {
> > > @@ -244,7 +243,7 @@ void pci_setup(void)
> > >       io_resource.base = 0xc000;
> > >       io_resource.max = 0x10000;
> > >   -    mmio_left = pci_mem_end - pci_mem_start;
> > > +    low_mmio_left = pci_mem_end - pci_mem_start;
> > >         /* Assign iomem and ioport resources in descending order
of size.
> > > */
> > >       for ( i = 0; i < nr_bars; i++ )
> > > @@ -253,7 +252,7 @@ void pci_setup(void)
> > >           bar_reg = bars[i].bar_reg;
> > >           bar_sz  = bars[i].bar_sz;
> > >   -        using_64bar = bars[i].is_64bar &&
bar64_relocate && (mmio_left
> > > < bar_sz);
> > > +        using_64bar = bars[i].is_64bar && bar64_relocate
&&
> > > (low_mmio_left < bar_sz);
> > >           bar_data = pci_readl(devfn, bar_reg);
> > >             if ( (bar_data & PCI_BASE_ADDRESS_SPACE) =>
> > @@ -273,9 +272,10 @@ void pci_setup(void)
> > >               }
> > >               else {
> > >                   resource = &mem_resource;
> > > +                if ( bar_sz <= low_mmio_left )
> > > +                    low_mmio_left -= bar_sz;
> > Why do you need this check? Isn''t the above if(using_64bar
&& (bar_sz >
> > PCI_MIN_BIG_BAR_SIZE)) enough?
> 
> This is in the lowmem region.  There may be regions which can''t be
relocated
> to the high PCI region that nevertheless don''t fit in the low PCI
region.  If
> it doesn''t fit, it will hit the "no space for resource"
conditional below and
> not be mapped; we need to make sure not to subtract it off.
> 
> I suppose a more robust method might be to use resource->max -
resource->base
> instead of keeping a separate accounting... I had originally thought that
> would be too invasive a change, but I''m not so sure now... any
thoughts?
You could just add:

if (resource == &mem_resource)
    low_mmio_left -= bar_sz;

right below the resource size check. This way we would have only one
check to see if the bar fits.

On 20/06/13 11:40, Stefano Stabellini wrote:
> On Thu, 20 Jun 2013, George Dunlap wrote:
>> On 19/06/13 18:18, Stefano Stabellini wrote:
>>> On Tue, 18 Jun 2013, George Dunlap wrote:
>>>> When deciding whether to map a device in low MMIO space
(<4GiB),
>>>> hvmloader compares it with "mmio_left", which is set
to the size of
>>>> the low MMIO range (pci_mem_end - pci_mem_start).  However,
even if it
>>>> does map a device in high MMIO space, it still removes the size
of its
>>>> BAR from mmio_left.
>>>>
>>>> This patch first changes the name of this variable to
"low_mmio_left"
>>>> to distinguish it from generic MMIO, and corrects the logic to
only
>>>> subtract the size of the BAR for devices maped in the low MMIO
region.
>>>>
>>>> Also make low_mmio_left unsigned, and don''t allow it
to go negative.
>>>> Since its main use is to be compared to a 64-bit unsigned int,
this
>>>> may have undefined (and in practice almost certainly incorrect)
>>>> results.  Not subtracting is OK because if there''s not
enough room, it
>>>> won''t actually be mapped.
>>>>
>>>> Signed-off-by: George Dunlap
<george.dunlap@eu.citrix.com>
>>>> CC: Ian Jackson <ian.jackson@citrix.com>
>>>> CC: Ian Campbell <ian.campbell@citrix.com>
>>>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>>>> CC: Hanweidong <hanweidong@huawei.com>
>>>> ---
>>>>    tools/firmware/hvmloader/pci.c |   10 +++++-----
>>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/tools/firmware/hvmloader/pci.c
>>>> b/tools/firmware/hvmloader/pci.c
>>>> index c78d4d3..8691a19 100644
>>>> --- a/tools/firmware/hvmloader/pci.c
>>>> +++ b/tools/firmware/hvmloader/pci.c
>>>> @@ -38,11 +38,10 @@ void pci_setup(void)
>>>>    {
>>>>        uint8_t is_64bar, using_64bar, bar64_relocate = 0;
>>>>        uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
>>>> -    uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0;
>>>> +    uint64_t base, bar_sz, bar_sz_upper, low_mmio_left,
mmio_total = 0;
>>>>        uint32_t vga_devfn = 256;
>>>>        uint16_t class, vendor_id, device_id;
>>>>        unsigned int bar, pin, link, isa_irq;
>>>> -    int64_t mmio_left;
>>>>          /* Resources assignable to PCI devices via BARs. */
>>>>        struct resource {
>>>> @@ -244,7 +243,7 @@ void pci_setup(void)
>>>>        io_resource.base = 0xc000;
>>>>        io_resource.max = 0x10000;
>>>>    -    mmio_left = pci_mem_end - pci_mem_start;
>>>> +    low_mmio_left = pci_mem_end - pci_mem_start;
>>>>          /* Assign iomem and ioport resources in descending
order of size.
>>>> */
>>>>        for ( i = 0; i < nr_bars; i++ )
>>>> @@ -253,7 +252,7 @@ void pci_setup(void)
>>>>            bar_reg = bars[i].bar_reg;
>>>>            bar_sz  = bars[i].bar_sz;
>>>>    -        using_64bar = bars[i].is_64bar &&
bar64_relocate && (mmio_left
>>>> < bar_sz);
>>>> +        using_64bar = bars[i].is_64bar &&
bar64_relocate &&
>>>> (low_mmio_left < bar_sz);
>>>>            bar_data = pci_readl(devfn, bar_reg);
>>>>              if ( (bar_data & PCI_BASE_ADDRESS_SPACE)
=>>>> @@ -273,9 +272,10 @@ void pci_setup(void)
>>>>                }
>>>>                else {
>>>>                    resource = &mem_resource;
>>>> +                if ( bar_sz <= low_mmio_left )
>>>> +                    low_mmio_left -= bar_sz;
>>> Why do you need this check? Isn''t the above if(using_64bar
&& (bar_sz >
>>> PCI_MIN_BIG_BAR_SIZE)) enough?
>> This is in the lowmem region.  There may be regions which
can''t be relocated
>> to the high PCI region that nevertheless don''t fit in the low
PCI region.  If
>> it doesn''t fit, it will hit the "no space for
resource" conditional below and
>> not be mapped; we need to make sure not to subtract it off.
>>
>> I suppose a more robust method might be to use resource->max -
resource->base
>> instead of keeping a separate accounting... I had originally thought
that
>> would be too invasive a change, but I''m not so sure now... any
thoughts?
> You could just add:
>
> if (resource == &mem_resource)
>      low_mmio_left -= bar_sz;
>
> right below the resource size check. This way we would have only one
> check to see if the bar fits.
Actually I just changed v3 to rid of low_mmio_left altogether, and just 
use "mem_resource.max - mem_resource.base" for the one and only time
the
value is needed.

  -George

On 20/06/13 11:37, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] [PATCH v2 5/5] libxl,
hvmloader: Don''t relocate memory for MMIO hole"):
>> It may be pointless from a functionality perspective, but it''s
also
>> harmless.  It won''t add a single byte to the compiled code,
but the 6
>> characters will remind a developer reading the source that there is a
>> cast being done here, just in case it should ever become important. 
Not
>> super important, but I''d rather leave it in.
> IMO this is a terrible reason for a cast.  Casts are dangerous things
> to have in code - they can override the compiler''s normal
> typechecking.  They should be used only when actually necessary, and
> code should normally be constructed so that they aren''t.
>
>> Doing this would effectively hide the "default" value.  This
is bad
>> because 1) it''s not clear what the default is to someone just
scanning
>> the code, 2) it''s hard to change.  (Consider how
you''d modify the above
>> statement if you wanted to default to 0 instead.)
> I agree with this complaint.
Sorry, which complaint?  Jan''s complaint that we have an
"unnecessary"
if() statment, or my complaint that not having an if() statement is 
confusing?  (That''s the topic of this paragraph.)

Or did you mean the complaint above, i.e., casting the result of strtoll()?

I don''t care terribly much about the casting; I''ll change it
rather than
argue about it. :-)

Does anyone has any opinions on getting rid of strtoll() altogether, and 
using direct comparisons, as Stefano suggests?

  -George

On Thu, 20 Jun 2013, George Dunlap wrote:
> On 19/06/13 18:18, Stefano Stabellini wrote:
> > On Tue, 18 Jun 2013, George Dunlap wrote:
> > > +    const char *s;
> > > +    bool allow_memory_relocate = 1;
> > Arguably the default should be 0, given that the default device model
is
> > qemu-xen that cannot cope with memory relocation.
> 
> OK, so time to think a bit harder about this. This will only matter if
someone
> is using this hvmloader with a non-libxl toolstack which includes xend, or
a
> home-grown one.
> 
> * If we default to 1, then:
>  - VMs running qemu-traditional will be have exactly as before
>  - VMs running qemu-xen will have the risk of crashing mysteriously.
>  - If qemu-xen is the default, then there is a work-around: run
> qemu-traditional
If we are talking about xend, then there is no point in thinking about
qemu-xen, given that xend cannot use it.

> * If we default to 0, then:
>  - VMs running qemu-xen will be fine
>  - VMs running qemu-traditional may have strange problems; we
haven''t tested
> relocating things into 64-bit with qemu-tradiational.
This is not true, hvmloader even before your changes relocates bar above
64-bit. It just does it in a more limited way.

>  - There is no work-around available; if the device either can''t
be relocated,
> or the OS / qemu can''t handle the relocation, then the user is
just hosed.
Given that we are talking about xend, we are forced to run
qemu-xen-traditional, so there is no workaround.

> Furthermore, I think xm defaults to qemu-traditional, right?  Does xm even
> know how to drive qemu-xen?  If it does default to qemu-traditional,
> defaulting to 0 will pretty much guarantee a whole slew of
currently-working
> configurations will be affected (perhaps break, perhaps not).
> 
> Overall I think defaulting to 1 is probably the lowest-risk option.
Defaulting to 1 makes sure that the behaviour of xend remains the same.
OK.

> > > +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
> > > +    if ( s )
> > > +        allow_memory_relocate = (bool)strtoll(s, NULL, 0);
> > > +    printf("Relocating guest memory for lowmem MMIO space
%s\n",
> > > +          
allow_memory_relocate?"enabled":"disabled");
> > It doesn''t take a strtoll to parse a boolean.
> 
> As discussed in v1, strtoll is the only "XtoY" function available
in
> hvmloader. :-)  The only other option would be to explicitly compare for
"1"
> or "0" (or do some half-baked *s-''0'' thing).
What''s wrong with testing for ''0'' or
''1''?

> This does make me think though -- what is the semantics of casting to a
bool?
> Is it !!, or will it essentially clip off the high bits?  (e.g., would
"2"
> become "1", or "0"?)
I think that is !!

> > >       /* Program PCI-ISA bridge with appropriate link routes. */
> > >       isa_irq = 0;
> > >       for ( link = 0; link < 4; link++ )
> > > @@ -209,14 +220,38 @@ void pci_setup(void)
> > >           pci_writew(devfn, PCI_COMMAND, cmd);
> > >       }
> > >   -    while ( (mmio_total > (pci_mem_end - pci_mem_start))
&&
> > > -            ((pci_mem_start << 1) != 0) )
> > > +    /*
> > > +     * At the moment qemu-xen can''t deal with relocated
memory regions.
> > > +     * It''s too close to the release to make a proper
fix; for now,
> > > +     * only allow the MMIO hole to grow large enough to move
guest memory
> > > +     * if we''re running qemu-traditional.  Items that
don''t fit will be
> > > +     * relocated into the 64-bit address space.
> > I would avoid mentioning release issues in a comment within the code.
> 
> I guess it depends on whether we intend to keep this change or to get rid
of
> it once the 4.4. window opens. If we intend to get rid of it, then I think
the
> comment should stay; if for some reason someone comes along later and and
> wants to know, "Can I change this?" Knowing that it was only
meant to be a
> temporary measure is important information.
> 
> Really, I''m of the opinion that if KVM is using SeaBIOS''s
pci routines, we
> should just move do the same.  No sense in duplicating the effort for
> something like this.
I would never make any changes with the assumption that they are going
to be reverted. There is nothing more lasting than a temporary
workaround. The comment should stay and explain why we are using it, but
mentioning the release (what release? when was it?) is pointless.

>>> On 20.06.13 at 12:20, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> On 20/06/13 11:12, Jan Beulich wrote:
>>>>> On 20.06.13 at 11:22, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
>>> On 19/06/13 18:18, Stefano Stabellini wrote:
>>>> On Tue, 18 Jun 2013, George Dunlap wrote:
>>>>> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
>>>>> +    if ( s )
>>>>> +        allow_memory_relocate = (bool)strtoll(s, NULL, 0);
>>>>> +    printf("Relocating guest memory for lowmem MMIO
space %s\n",
>>>>> +          
allow_memory_relocate?"enabled":"disabled");
>>>> It doesn''t take a strtoll to parse a boolean.
>>> As discussed in v1, strtoll is the only "XtoY" function
available in
>>> hvmloader. :-)  The only other option would be to explicitly
compare for
>>> "1" or "0" (or do some half-baked
*s-''0'' thing).
>>>
>>> This does make me think though -- what is the semantics of casting
to a
>>> bool?  Is it !!, or will it essentially clip off the high bits? 
(e.g.,
>>> would "2" become "1", or "0"?)
>> If bool is a typedef or #define of _Bool, and _Bool is a complier
>> supplied type, then the cast will do the right thing. But doing the
>> assignment without the cast would too, i.e. the cast is pointless
>> (as I think IanJ had already pointed out).
> 
> Thanks for the info.
> 
> It may be pointless from a functionality perspective, but it''s
also
> harmless.  It won''t add a single byte to the compiled code, but
the 6
> characters will remind a developer reading the source that there is a 
> cast being done here, just in case it should ever become important.  Not 
> super important, but I''d rather leave it in.
To a degree this is certainly a matter of taste, but since casts
frequently are hiding problems, I''m generally advocating to have
as few casts as possible.
>> However, if we want to be on the safe side and also make the
>> code work with a compiler that doesn''t have a built-in _Bool,
I''d
>> think
>>
>>      allow_memory_relocate = !s || strtoll(s, NULL, 0);
>>
>> would be the better statement (without any if() surrounding it,
>> and without the variable declaration having an initializer.
> 
> Doing this would effectively hide the "default" value.  This is
bad
> because 1) it''s not clear what the default is to someone just
scanning
> the code, 2) it''s hard to change.  (Consider how you''d
modify the above
> statement if you wanted to default to 0 instead.)
Again a matter of taste perhaps, or what people are used to.
To me, the suggested statement is perfectly obvious in terms
of the resulting default, and wanting the opposite default also
wouldn''t be a hard to understand change:

      allow_memory_relocate = s && strtoll(s, NULL, 0);

Jan

>>> On 20.06.13 at 12:29, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 20 Jun 2013, George Dunlap wrote:
>> On 20/06/13 11:12, Jan Beulich wrote:
>> > However, if we want to be on the safe side and also make the
>> > code work with a compiler that doesn''t have a built-in
_Bool, I''d
>> > think
>> > 
>> >      allow_memory_relocate = !s || strtoll(s, NULL, 0);
>> > 
>> > would be the better statement (without any if() surrounding it,
>> > and without the variable declaration having an initializer.
>> 
>> Doing this would effectively hide the "default" value.  This
is bad because 1)
>> it''s not clear what the default is to someone just scanning
the code, 2) it''s
>> hard to change.  (Consider how you''d modify the above
statement if you wanted
>> to default to 0 instead.)
> 
> I would avoid the strtoll altogether:
> 
> if (s != NULL && s[0] != ''0'')
>     allow_memory_relocate = 1;
> else
>     allow_memory_relocate = 0;
Let''s not add hacks like this - a string of "0x1" ought to
not be
mis-interpreted as meaning 0.

Jan

On 20/06/13 11:56, Jan Beulich wrote:
>>>> On 20.06.13 at 12:29, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
>> On Thu, 20 Jun 2013, George Dunlap wrote:
>>> On 20/06/13 11:12, Jan Beulich wrote:
>>>> However, if we want to be on the safe side and also make the
>>>> code work with a compiler that doesn''t have a built-in
_Bool, I''d
>>>> think
>>>>
>>>>       allow_memory_relocate = !s || strtoll(s, NULL, 0);
>>>>
>>>> would be the better statement (without any if() surrounding it,
>>>> and without the variable declaration having an initializer.
>>> Doing this would effectively hide the "default" value. 
This is bad because 1)
>>> it''s not clear what the default is to someone just
scanning the code, 2) it''s
>>> hard to change.  (Consider how you''d modify the above
statement if you wanted
>>> to default to 0 instead.)
>> I would avoid the strtoll altogether:
>>
>> if (s != NULL && s[0] != ''0'')
>>      allow_memory_relocate = 1;
>> else
>>      allow_memory_relocate = 0;
> Let''s not add hacks like this - a string of "0x1" ought
to not be
> mis-interpreted as meaning 0.
I''m pretty sure the xs protocol specifies "0" or
"1" for booleans, so
"0x1" would be undefined anyway.

  -George

On 20/06/13 11:29, Stefano Stabellini wrote:
> On Thu, 20 Jun 2013, George Dunlap wrote:
>> On 20/06/13 11:12, Jan Beulich wrote:
>>>>>> On 20.06.13 at 11:22, George Dunlap
<george.dunlap@eu.citrix.com>
>>>>>> wrote:
>>>> On 19/06/13 18:18, Stefano Stabellini wrote:
>>>>> On Tue, 18 Jun 2013, George Dunlap wrote:
>>>>>> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE,
NULL);
>>>>>> +    if ( s )
>>>>>> +        allow_memory_relocate = (bool)strtoll(s, NULL,
0);
>>>>>> +    printf("Relocating guest memory for lowmem
MMIO space %s\n",
>>>>>> +          
allow_memory_relocate?"enabled":"disabled");
>>>>> It doesn''t take a strtoll to parse a boolean.
>>>> As discussed in v1, strtoll is the only "XtoY"
function available in
>>>> hvmloader. :-)  The only other option would be to explicitly
compare for
>>>> "1" or "0" (or do some half-baked
*s-''0'' thing).
>>>>
>>>> This does make me think though -- what is the semantics of
casting to a
>>>> bool?  Is it !!, or will it essentially clip off the high bits?
(e.g.,
>>>> would "2" become "1", or "0"?)
>>> If bool is a typedef or #define of _Bool, and _Bool is a complier
>>> supplied type, then the cast will do the right thing. But doing the
>>> assignment without the cast would too, i.e. the cast is pointless
>>> (as I think IanJ had already pointed out).
>> Thanks for the info.
>>
>> It may be pointless from a functionality perspective, but it''s
also harmless.
>> It won''t add a single byte to the compiled code, but the 6
characters will
>> remind a developer reading the source that there is a cast being done
here,
>> just in case it should ever become important.  Not super important, but
I''d
>> rather leave it in.
>>
>>> However, if we want to be on the safe side and also make the
>>> code work with a compiler that doesn''t have a built-in
_Bool, I''d
>>> think
>>>
>>>       allow_memory_relocate = !s || strtoll(s, NULL, 0);
>>>
>>> would be the better statement (without any if() surrounding it,
>>> and without the variable declaration having an initializer.
>> Doing this would effectively hide the "default" value.  This
is bad because 1)
>> it''s not clear what the default is to someone just scanning
the code, 2) it''s
>> hard to change.  (Consider how you''d modify the above
statement if you wanted
>> to default to 0 instead.)
> I would avoid the strtoll altogether:
>
> if (s != NULL && s[0] != ''0'')
>      allow_memory_relocate = 1;
> else
>      allow_memory_relocate = 0;
I think I''d be more inclined to do allow_memory_relocate = strcmp(s, 
"0");  That will have more predictable results (e.g., 0 is false, 
anything else at all is true).

  -George

George Dunlap writes ("Re: [Xen-devel] [PATCH v2 5/5] libxl, hvmloader:
Don''t relocate memory for MMIO hole"):
> I think I''d be more inclined to do allow_memory_relocate =
strcmp(s,
> "0");  That will have more predictable results (e.g., 0 is false,
> anything else at all is true).
This is a real bikeshed issue, but:

IMO the strtoll is fine and is superior to strcmp(), direct string
inspection, etc.  If we are representing booleans as integers written
out in ASCII, we should be using an integer parsing function, not
cooking up our own half-arsed not-quite-integer parser.

If someone wants to add xs_read_bool or something which checks for "0"
and "1" and throws an error if it reads something else, then fine.
But not in this patch series and not for 4.3, obviously.

Ian.

On 20/06/13 14:35, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] [PATCH v2 5/5] libxl,
hvmloader: Don''t relocate memory for MMIO hole"):
>> I think I''d be more inclined to do allow_memory_relocate =
strcmp(s,
>> "0");  That will have more predictable results (e.g., 0 is
false,
>> anything else at all is true).
> This is a real bikeshed issue, but:
>
> IMO the strtoll is fine and is superior to strcmp(), direct string
> inspection, etc.  If we are representing booleans as integers written
> out in ASCII, we should be using an integer parsing function, not
> cooking up our own half-arsed not-quite-integer parser.
Right -- so I''ll paint the bike shed "allow_memory_relocate = 
strtoll()", removing the cast to bool.

  -George

On Thu, 2013-06-20 at 10:22 +0100, George Dunlap wrote:
> Really, I''m of the opinion that if KVM is using SeaBIOS''s
pci
> routines, we should just move do the same.  No sense in duplicating
> the effort for something like this.
Perhaps I''m misremembering and/or conflating different issues but I
think I tried this when I initially ported SeaBIOS to Xen and it got
complicated quickly.

My memory is a bit vague but IIRC there were issues with things like the
PCI IRQ routing and with the ACPI tables. In both cases these are
handled in hvmloader because they are counterparts to the implementation
of the underlying virtualised platform which is tied to Xen. Moving
things into SeaBIOS would constrain our ability to change stuff going
forward (e.g. SeaBIOSes decisions about interrupt routing doesn''t not
currently match Xen''s implementation) and would lead to a tricky API
boundary somewhere between hvmloader+xen and SeaBIOS(+-qemu)

Maybe PCI BAR placement isn''t inherently linked to all that though, you
are welcome to try and split it out ;-)

Ian.

On 06/25/2013 10:56 AM, Ian Campbell wrote:
> On Thu, 2013-06-20 at 10:22 +0100, George Dunlap wrote:
>> Really, I''m of the opinion that if KVM is using
SeaBIOS''s pci
>> routines, we should just move do the same.  No sense in duplicating
>> the effort for something like this.
>
> Perhaps I''m misremembering and/or conflating different issues but
I
> think I tried this when I initially ported SeaBIOS to Xen and it got
> complicated quickly.
>
> My memory is a bit vague but IIRC there were issues with things like the
> PCI IRQ routing and with the ACPI tables. In both cases these are
> handled in hvmloader because they are counterparts to the implementation
> of the underlying virtualised platform which is tied to Xen. Moving
> things into SeaBIOS would constrain our ability to change stuff going
> forward (e.g. SeaBIOSes decisions about interrupt routing doesn''t
not
> currently match Xen''s implementation) and would lead to a tricky
API
> boundary somewhere between hvmloader+xen and SeaBIOS(+-qemu)
>
> Maybe PCI BAR placement isn''t inherently linked to all that
though, you
> are welcome to try and split it out ;-)
Thanks for the heads up.  Yeah, I guess the interrupt routing stuff is 
all mixed in there together, isn''t it... anyway I''ll keep that
in mind.

  -George