[PATCH v3 0/1] Add pci_hole_min_size

[PATCH v3 0/1] Add pci_hole_min_size

[Don Slutz]

Open question: Should I switch this to max_ram_below_4g or stay with
pci_hole_min_size?

This may help or fix http://bugs.xenproject.org/xen/bug/28

Changes from v2 to v3:
  Dropped RFC since QEMU change will be in QEMU 2.1.0 release.

  Dropped "if (dom_info.debug)" in parse_config_data()
  since dom_info no long passed in.

  Lots more bounds checking.  Since QEMU is a min(qemu_limit,
  max_ram_below_4g) adjust code the do the same.

  Boris Ostrovsky:
    More in commit message.


Changes from v1 to v2:
  Boris Ostrovsky:
    Need to init pci_hole_min_size

  Changed the qemu name from pci_hole_min_size to
  pc-memory-layout.max-ram-below-4g.

  Did not change the Xen version for now.

  Added quick note to xl.cfg.pod.5

  Added a check for a too big value. (Most likely in the wrong
  place.)

If you add enough PCI devices then all mmio may not fit below 4G
which may not be the layout the user wanted. This allows you to
increase the below 4G address space that PCI devices can use and
therefore in more cases not have any mmio that is above 4G.

There are real PCI cards that do not support mmio over 4G, so if you
want to emulate them precisely, you may also need to increase the
space below 4G for them. There are drivers for these cards that also
do not work if they have their mmio space mapped above 4G.

This is posted as an RFC because you need the upstream version of
qemu with all 4 patches:

http://marc.info/?l=qemu-devel&m=139455360016654&w=2

This allows growing the pci_hole to the size needed.

This may help with using pci passthru and HVM. 


Don Slutz (1):
  Add pci_hole_min_size

 docs/man/xl.cfg.pod.5             | 12 ++++++++++++
 tools/firmware/hvmloader/config.h |  1 -
 tools/firmware/hvmloader/pci.c    | 28 +++++++++++++++++++++++++++-
 tools/libxc/xc_hvm_build_x86.c    | 30 ++++++++++++++++++++++++++++--
 tools/libxc/xenguest.h            | 11 +++++++++++
 tools/libxl/libxl_create.c        | 29 ++++++++++++++++++++++-------
 tools/libxl/libxl_dm.c            | 23 +++++++++++++++++++++--
 tools/libxl/libxl_dom.c           |  3 ++-
 tools/libxl/libxl_types.idl       |  1 +
 tools/libxl/xl_cmdimpl.c          | 13 +++++++++++++
 10 files changed, 137 insertions(+), 14 deletions(-)

-- 
1.8.4

[PATCH v3 1/1] Add pci_hole_min_size

[Don Slutz]

If you add enough PCI devices then all mmio may not fit below 4G
which may not be the layout the user wanted. This allows you to
increase the below 4G address space that PCI devices can use and
therefore in more cases not have any mmio that is above 4G.

There are real PCI cards that do not support mmio over 4G, so if you
want to emulate them precisely, you may also need to increase the
space below 4G for them. There are drivers for these cards that also
do not work if they have their mmio space mapped above 4G.

This allows growing the pci_hole to the size needed.

This may help with using pci passthru and HVM.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
v3:
  Dropped "if (dom_info.debug)" in parse_config_data()
  since dom_info no long passed in.

  Lots more bounds checking.  Since QEMU is a min(qemu_limit,
  max_ram_below_4g) adjust code the do the same.

  More in commit message.

 docs/man/xl.cfg.pod.5             | 12 ++++++++++++
 tools/firmware/hvmloader/config.h |  1 -
 tools/firmware/hvmloader/pci.c    | 28 +++++++++++++++++++++++++++-
 tools/libxc/xc_hvm_build_x86.c    | 30 ++++++++++++++++++++++++++++--
 tools/libxc/xenguest.h            | 11 +++++++++++
 tools/libxl/libxl_create.c        | 29 ++++++++++++++++++++++-------
 tools/libxl/libxl_dm.c            | 23 +++++++++++++++++++++--
 tools/libxl/libxl_dom.c           |  3 ++-
 tools/libxl/libxl_types.idl       |  1 +
 tools/libxl/xl_cmdimpl.c          | 13 +++++++++++++
 10 files changed, 137 insertions(+), 14 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index c087cbc..60b4cb6 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1032,6 +1032,18 @@ wallclock (i.e., real) time.
 
 =back
 
+=head3 Memory layout
+
+=over 4
+
+=item B<pci_hole_min_size=BYTES>
+
+Specifies the minimum size the PCI MMIO hole below 4GiB will be.
+Only valid for device_model_version = "qemu-xen".
+Note: this requires QEMU 2.1.0 or later.
+
+=back
+
 =head3 Support for Paravirtualisation of HVM Guests
 
 The following options allow Paravirtualised features (such as devices)
diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index 80bea46..b838cf9 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -53,7 +53,6 @@ extern struct bios_config ovmf_config;
 #define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI connected */
 
 /* MMIO hole: Hardcoded defaults, which can be dynamically expanded. */
-#define PCI_MEM_START       0xf0000000
 #define PCI_MEM_END         0xfc000000
 
 extern unsigned long pci_mem_start, pci_mem_end;
diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 3712988..dff3576 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -28,9 +28,10 @@
 #include <xen/memory.h>
 #include <xen/hvm/ioreq.h>
 #include <xen/hvm/hvm_xs_strings.h>
+#include <xen/hvm/e820.h>
 #include <stdbool.h>
 
-unsigned long pci_mem_start = PCI_MEM_START;
+unsigned long pci_mem_start = HVM_BELOW_4G_RAM_END;
 unsigned long pci_mem_end = PCI_MEM_END;
 uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
 
@@ -59,6 +60,7 @@ void pci_setup(void)
         uint64_t bar_sz;
     } *bars = (struct bars *)scratch_start;
     unsigned int i, nr_bars = 0;
+    uint64_t pci_hole_min_size = 0;
 
     const char *s;
     /*
@@ -86,6 +88,10 @@ void pci_setup(void)
     printf("Relocating guest memory for lowmem MMIO space %s\n",
            allow_memory_relocate?"enabled":"disabled");
 
+    s = xenstore_read("platform/pci_hole_min_size", NULL);
+    if ( s )
+        pci_hole_min_size = strtoll(s, NULL, 0);
+
     /* Program PCI-ISA bridge with appropriate link routes. */
     isa_irq = 0;
     for ( link = 0; link < 4; link++ )
@@ -237,6 +243,26 @@ void pci_setup(void)
         pci_writew(devfn, PCI_COMMAND, cmd);
     }
 
+    if ( pci_hole_min_size )
+    {
+        uint64_t max_ram_below_4g = (1ULL << 32) - pci_hole_min_size;
+
+        if ( max_ram_below_4g >= HVM_BELOW_4G_RAM_END )
+        {
+            printf("max_ram_below_4g=0x"PRIllx
+                   " too big for pci_hole_min_size=0x"PRIllx"\n",
+                   PRIllx_arg(max_ram_below_4g),
+                   PRIllx_arg(pci_hole_min_size));
+        }
+        else
+        {
+            pci_mem_start = max_ram_below_4g;
+            printf("pci_mem_start=0x%lx (was 0x%x) for pci_hole_min_size=%lu\n",
+                   pci_mem_start, HVM_BELOW_4G_RAM_END,
+                   (long)pci_hole_min_size);
+        }
+    }
+
     /*
      * 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,
diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index ed1069b..f557f87 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -628,14 +628,40 @@ int xc_hvm_build_target_mem(xc_interface *xch,
                            int target,
                            const char *image_name)
 {
+    return xc_hvm_build_target_mem_with_hole(xch, domid, memsize, target,
+                                             image_name, 0);
+}
+
+int xc_hvm_build_with_hole(xc_interface *xch, uint32_t domid,
+                           struct xc_hvm_build_args *args,
+                           uint64_t pci_hole_min_size)
+{
+    if (pci_hole_min_size)
+    {
+        uint64_t max_ram_below_4g = (1ULL << 32) - pci_hole_min_size;
+
+        if (max_ram_below_4g < HVM_BELOW_4G_RAM_END)
+        {
+            args->mmio_size = pci_hole_min_size;
+        }
+    }
+    return xc_hvm_build(xch, domid, args);
+}
+
+int xc_hvm_build_target_mem_with_hole(xc_interface *xch,
+                                      uint32_t domid,
+                                      int memsize,
+                                      int target,
+                                      const char *image_name,
+                                      uint64_t pci_hole_min_size)
+{
     struct xc_hvm_build_args args = {};
 
     memset(&args, 0, sizeof(struct xc_hvm_build_args));
     args.mem_size = (uint64_t)memsize << 20;
     args.mem_target = (uint64_t)target << 20;
     args.image_file_name = image_name;
-
-    return xc_hvm_build(xch, domid, &args);
+    return xc_hvm_build_with_hole(xch, domid, &args, pci_hole_min_size);
 }
 
 /*
diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h
index 1f216cd..0e9c5f0 100644
--- a/tools/libxc/xenguest.h
+++ b/tools/libxc/xenguest.h
@@ -248,12 +248,23 @@ struct xc_hvm_build_args {
 int xc_hvm_build(xc_interface *xch, uint32_t domid,
                  struct xc_hvm_build_args *hvm_args);
 
+int xc_hvm_build_with_hole(xc_interface *xch, uint32_t domid,
+                           struct xc_hvm_build_args *args,
+                           uint64_t pci_hole_min_size);
+
 int xc_hvm_build_target_mem(xc_interface *xch,
                             uint32_t domid,
                             int memsize,
                             int target,
                             const char *image_name);
 
+int xc_hvm_build_target_mem_with_hole(xc_interface *xch,
+                                      uint32_t domid,
+                                      int memsize,
+                                      int target,
+                                      const char *image_name,
+                                      uint64_t pci_hole_min_size);
+
 /*
  * Sets *lockfd to -1.
  * Has deallocated everything even on error.
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index d6b8a29..b90d787 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -23,6 +23,7 @@
 #include <xc_dom.h>
 #include <xenguest.h>
 #include <xen/hvm/hvm_info_table.h>
+#include <xen/hvm/e820.h>
 
 int libxl__domain_create_info_setdefault(libxl__gc *gc,
                                          libxl_domain_create_info *c_info)
@@ -402,13 +403,26 @@ int libxl__domain_build(libxl__gc *gc,
         vments[4] = "start_time";
         vments[5] = libxl__sprintf(gc, "%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
 
-        localents = libxl__calloc(gc, 7, sizeof(char *));
-        localents[0] = "platform/acpi";
-        localents[1] = libxl_defbool_val(info->u.hvm.acpi) ? "1" : "0";
-        localents[2] = "platform/acpi_s3";
-        localents[3] = libxl_defbool_val(info->u.hvm.acpi_s3) ? "1" : "0";
-        localents[4] = "platform/acpi_s4";
-        localents[5] = libxl_defbool_val(info->u.hvm.acpi_s4) ? "1" : "0";
+        localents = libxl__calloc(gc, 9, sizeof(char *));
+        i = 0;
+        localents[i++] = "platform/acpi";
+        localents[i++] = libxl_defbool_val(info->u.hvm.acpi) ? "1" : "0";
+        localents[i++] = "platform/acpi_s3";
+        localents[i++] = libxl_defbool_val(info->u.hvm.acpi_s3) ? "1" : "0";
+        localents[i++] = "platform/acpi_s4";
+        localents[i++] = libxl_defbool_val(info->u.hvm.acpi_s4) ? "1" : "0";
+        if (info->u.hvm.pci_hole_min_size) {
+            uint64_t max_ram_below_4g =
+                (1ULL << 32) - info->u.hvm.pci_hole_min_size;
+
+            if (max_ram_below_4g < HVM_BELOW_4G_RAM_END) {
+                localents[i++] = "platform/pci_hole_min_size";
+                localents[i++] =
+                    libxl__sprintf(gc, "%"PRIu64,
+                                   info->u.hvm.pci_hole_min_size);
+            }
+        }
+        assert(i < 9);
 
         break;
     case LIBXL_DOMAIN_TYPE_PV:
@@ -434,6 +448,7 @@ int libxl__domain_build(libxl__gc *gc,
             vments[i++] = "image/cmdline";
             vments[i++] = (char *) state->pv_cmdline;
         }
+        assert(i < 11);
 
         break;
     default:
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index addacdb..147e682 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -18,6 +18,7 @@
 #include "libxl_osdeps.h" /* must come before any other headers */
 
 #include "libxl_internal.h"
+#include <xen/hvm/e820.h>
 
 static const char *libxl_tapif_script(libxl__gc *gc)
 {
@@ -393,6 +394,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
     const libxl_vnc_info *vnc = libxl__dm_vnc(guest_config);
     const libxl_sdl_info *sdl = dm_sdl(guest_config);
     const char *keymap = dm_keymap(guest_config);
+    char *machinearg;
     flexarray_t *dm_args;
     int i;
     uint64_t ram_size;
@@ -658,10 +660,27 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
             /* Switching here to the machine "pc" which does not add
              * the xen-platform device instead of the default "xenfv" machine.
              */
-            flexarray_append(dm_args, "pc,accel=xen");
+            machinearg = libxl__sprintf(gc, "pc,accel=xen");
         } else {
-            flexarray_append(dm_args, "xenfv");
+            machinearg = libxl__sprintf(gc, "xenfv");
         }
+        if (b_info->u.hvm.pci_hole_min_size) {
+            uint64_t max_ram_below_4g = (1ULL << 32) -
+                b_info->u.hvm.pci_hole_min_size;
+
+            if (max_ram_below_4g >= HVM_BELOW_4G_RAM_END) {
+                LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
+                           "pci_hole_min_size(%"PRIu64
+                           ") too big => max-ram-below-4g=%"PRIu64
+                           " >= %u ignored.\n",
+                           b_info->u.hvm.pci_hole_min_size,
+                           max_ram_below_4g, HVM_BELOW_4G_RAM_END);
+            } else {
+                machinearg = libxl__sprintf(gc, "%s,max-ram-below-4g=%"PRIu64,
+                                            machinearg, max_ram_below_4g);
+            }
+        }
+        flexarray_append(dm_args, machinearg);
         for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; i++)
             flexarray_append(dm_args, b_info->extra_hvm[i]);
         break;
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 661999c..b8559ea 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -655,7 +655,8 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
         goto out;
     }
 
-    ret = xc_hvm_build(ctx->xch, domid, &args);
+    ret = xc_hvm_build_with_hole(ctx->xch, domid, &args,
+                                 info->u.hvm.pci_hole_min_size);
     if (ret) {
         LOGEV(ERROR, ret, "hvm building failed");
         goto out;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 1018142..3b72565 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -351,6 +351,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                        ("timeoffset",       string),
                                        ("hpet",             libxl_defbool),
                                        ("vpt_align",        libxl_defbool),
+                                       ("pci_hole_min_size",UInt(64, init_val = 0)),
                                        ("timer_mode",       libxl_timer_mode),
                                        ("nested_hvm",       libxl_defbool),
                                        ("smbios_firmware",  string),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index be041f2..3c1fc47 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -34,6 +34,7 @@
 #include <ctype.h>
 #include <inttypes.h>
 #include <limits.h>
+#include <xen/hvm/e820.h>
 
 #include "libxl.h"
 #include "libxl_utils.h"
@@ -969,6 +970,18 @@ static void parse_config_data(const char *config_source,
         xlu_cfg_get_defbool(config, "hpet", &b_info->u.hvm.hpet, 0);
         xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align, 0);
 
+        if (!xlu_cfg_get_long(config, "pci_hole_min_size", &l, 0)) {
+            b_info->u.hvm.pci_hole_min_size = (uint64_t) l;
+            if (b_info->u.hvm.pci_hole_min_size < HVM_BELOW_4G_MMIO_LENGTH ||
+                b_info->u.hvm.pci_hole_min_size > HVM_BELOW_4G_RAM_END) {
+                fprintf(stderr, "ERROR: invalid value 0x%"PRIx64" (%"PRIu64
+                        ") for \"pci_hole_min_size\"\n",
+                        b_info->u.hvm.pci_hole_min_size,
+                        b_info->u.hvm.pci_hole_min_size);
+                exit (1);
+            }
+        }
+
         if (!xlu_cfg_get_long(config, "timer_mode", &l, 1)) {
             const char *s = libxl_timer_mode_to_string(l);
             fprintf(stderr, "WARNING: specifying \"timer_mode\" as an integer is deprecated. "
-- 
1.8.4

Re: [PATCH v3 1/1] Add pci_hole_min_size

[Jan Beulich]

>>> On 26.06.14 at 19:47, <dslutz@verizon.com> wrote:
> --- a/tools/firmware/hvmloader/config.h
> +++ b/tools/firmware/hvmloader/config.h
> @@ -53,7 +53,6 @@ extern struct bios_config ovmf_config;
>  #define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI connected */
>  
>  /* MMIO hole: Hardcoded defaults, which can be dynamically expanded. */
> -#define PCI_MEM_START       0xf0000000
>  #define PCI_MEM_END         0xfc000000

This pair already shows that ... [skip next comment for continuation]

> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -28,9 +28,10 @@
>  #include <xen/memory.h>
>  #include <xen/hvm/ioreq.h>
>  #include <xen/hvm/hvm_xs_strings.h>
> +#include <xen/hvm/e820.h>
>  #include <stdbool.h>
>  
> -unsigned long pci_mem_start = PCI_MEM_START;
> +unsigned long pci_mem_start = HVM_BELOW_4G_RAM_END;

Perhaps (also related to the comment below) better to use
HVM_BELOW_4G_MMIO_START here, even if right now both evaluate
to the same number.

> @@ -237,6 +243,26 @@ void pci_setup(void)
>          pci_writew(devfn, PCI_COMMAND, cmd);
>      }
>  
> +    if ( pci_hole_min_size )
> +    {
> +        uint64_t max_ram_below_4g = (1ULL << 32) - pci_hole_min_size;

... this isn't really correct: The space immediately below 4G is used for
MMIO unrelated to PCI. Hence, to not confuse people, I'd really like
the variable to be properly named (whether the xl config option should
also get renamed I'm not sure - the xl maintainers will know), i.e.
mmio_hole_min_size.

> +
> +        if ( max_ram_below_4g >= HVM_BELOW_4G_RAM_END )
> +        {
> +            printf("max_ram_below_4g=0x"PRIllx
> +                   " too big for pci_hole_min_size=0x"PRIllx"\n",
> +                   PRIllx_arg(max_ram_below_4g),
> +                   PRIllx_arg(pci_hole_min_size));

Probably worth making clear in the logged message that the option
therefore is being ignored?

> +        }
> +        else
> +        {
> +            pci_mem_start = max_ram_below_4g;
> +            printf("pci_mem_start=0x%lx (was 0x%x) for pci_hole_min_size=%lu\n",
> +                   pci_mem_start, HVM_BELOW_4G_RAM_END,
> +                   (long)pci_hole_min_size);
> +        }
> +    }

Furthermore the whole concept collides with the auto-growing of
the hole immediately following this code. Both logically (both having
a similar purpose) and, what's worse, functionally in that the
shifting done there requires that ((1ULL << 32) - pci_mem_start)
be a power of 2. An option might be to skip this auto-sizing if the
option was given.

Jan

Re: [PATCH v3 1/1] Add pci_hole_min_size

[Don Slutz]

On 06/27/14 04:34, Jan Beulich wrote:
>>>> On 26.06.14 at 19:47, <dslutz@verizon.com> wrote:
>> --- a/tools/firmware/hvmloader/config.h
>> +++ b/tools/firmware/hvmloader/config.h
>> @@ -53,7 +53,6 @@ extern struct bios_config ovmf_config;
>>   #define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI connected */
>>   
>>   /* MMIO hole: Hardcoded defaults, which can be dynamically expanded. */
>> -#define PCI_MEM_START       0xf0000000
>>   #define PCI_MEM_END         0xfc000000
> This pair already shows that ... [skip next comment for continuation]
>
>> --- a/tools/firmware/hvmloader/pci.c
>> +++ b/tools/firmware/hvmloader/pci.c
>> @@ -28,9 +28,10 @@
>>   #include <xen/memory.h>
>>   #include <xen/hvm/ioreq.h>
>>   #include <xen/hvm/hvm_xs_strings.h>
>> +#include <xen/hvm/e820.h>
>>   #include <stdbool.h>
>>   
>> -unsigned long pci_mem_start = PCI_MEM_START;
>> +unsigned long pci_mem_start = HVM_BELOW_4G_RAM_END;
> Perhaps (also related to the comment below) better to use
> HVM_BELOW_4G_MMIO_START here, even if right now both evaluate
> to the same number.

Will do.

>
>> @@ -237,6 +243,26 @@ void pci_setup(void)
>>           pci_writew(devfn, PCI_COMMAND, cmd);
>>       }
>>   
>> +    if ( pci_hole_min_size )
>> +    {
>> +        uint64_t max_ram_below_4g = (1ULL << 32) - pci_hole_min_size;
> ... this isn't really correct: The space immediately below 4G is used for
> MMIO unrelated to PCI. Hence, to not confuse people, I'd really like
> the variable to be properly named (whether the xl config option should
> also get renamed I'm not sure - the xl maintainers will know), i.e.
> mmio_hole_min_size.

I think it is better to have at most 2 names.  So either
s/pci_hole_min_size/mmio_hole_min_size/ or switch to
max_ram_below_4g.

>> +
>> +        if ( max_ram_below_4g >= HVM_BELOW_4G_RAM_END )
>> +        {
>> +            printf("max_ram_below_4g=0x"PRIllx
>> +                   " too big for pci_hole_min_size=0x"PRIllx"\n",
>> +                   PRIllx_arg(max_ram_below_4g),
>> +                   PRIllx_arg(pci_hole_min_size));
> Probably worth making clear in the logged message that the option
> therefore is being ignored?

Sure.

>> +        }
>> +        else
>> +        {
>> +            pci_mem_start = max_ram_below_4g;
>> +            printf("pci_mem_start=0x%lx (was 0x%x) for pci_hole_min_size=%lu\n",
>> +                   pci_mem_start, HVM_BELOW_4G_RAM_END,
>> +                   (long)pci_hole_min_size);
>> +        }
>> +    }
> Furthermore the whole concept collides with the auto-growing of
> the hole immediately following this code. Both logically (both having
> a similar purpose) and, what's worse, functionally in that the
> shifting done there requires that ((1ULL << 32) - pci_mem_start)
> be a power of 2. An option might be to skip this auto-sizing if the
> option was given.

I am fine with disabling the auto-growing (since as far as I know
this is bug #28 (


  #28 - support PCI hole resize in qemu-xen

)

Which this change is a kind of work around.

I would also drop the _min part of the name (i.e. use mmio_hole_size
or max_ram_below_4g).

     -Don Slutz


> Jan
>

Re: [PATCH v3 1/1] Add pci_hole_min_size

[Jan Beulich]

>>> On 01.07.14 at 13:56, <dslutz@verizon.com> wrote:
> On 06/27/14 04:34, Jan Beulich wrote:
>> Furthermore the whole concept collides with the auto-growing of
>> the hole immediately following this code. Both logically (both having
>> a similar purpose) and, what's worse, functionally in that the
>> shifting done there requires that ((1ULL << 32) - pci_mem_start)
>> be a power of 2. An option might be to skip this auto-sizing if the
>> option was given.
> 
> I am fine with disabling the auto-growing (since as far as I know
> this is bug #28 (
> 
> 
>   #28 - support PCI hole resize in qemu-xen
> 
> )

But that's only with upstream qemu I suppose; the auto-resizing
was added for a reason (see the history of the file). So this is
definitely not going to be an unconditional disabling (other than
what I kind of read from your reply).

Jan

Re: [PATCH v3 1/1] Add pci_hole_min_size

[Don Slutz]

On 07/01/14 08:48, Jan Beulich wrote:
>>>> On 01.07.14 at 13:56, <dslutz@verizon.com> wrote:
>> On 06/27/14 04:34, Jan Beulich wrote:
>>> Furthermore the whole concept collides with the auto-growing of
>>> the hole immediately following this code. Both logically (both having
>>> a similar purpose) and, what's worse, functionally in that the
>>> shifting done there requires that ((1ULL << 32) - pci_mem_start)
>>> be a power of 2. An option might be to skip this auto-sizing if the
>>> option was given.
>> I am fine with disabling the auto-growing (since as far as I know
>> this is bug #28 (
>>
>>
>>    #28 - support PCI hole resize in qemu-xen
>>
>> )
> But that's only with upstream qemu I suppose; the auto-resizing
> was added for a reason (see the history of the file). So this is
> definitely not going to be an unconditional disabling (other than
> what I kind of read from your reply).

I was not clear.  I am thinking that if pci_hole_min_size (or what ever
the name is) is non-zero (i.e. not default (could be exists also)), I would
disable the auto-resizing code.  As far as I know, the auto-resizing only
works with qemu-xen-traditional.  I was not planning to add
max-ram-below-4g to qemu-xen-traditional (but it would not be a big
patch nor as complex as what is in qemu-xen (upstream master)).

so currently it would be:

+   }
+    else
+   {
/*
      * 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;
+  }



But looking closer I could just set:

HVM_XS_ALLOW_MEMORY_RELOCATE to 0

to stop part of this.  And the other part should be false do to
xc_hvm_build_with_hole()

but this will need testing.

      -Don Slutz


> Jan
>