[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5 1/1] Add mmio_hole_size




On 09/30/14 09:15, George Dunlap wrote:
On Thu, Sep 11, 2014 at 5:20 PM, Don Slutz <dslutz@xxxxxxxxxxx> wrote:
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 MMIO hole to the size needed.

This may help with using pci passthru and HVM.

Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
FYI you'll have to rebase this since it doesn't apply cleanly anymore.

Or, I have rebased.

Also, I've been meaning to look at this for months; sorry it's taken so long.

Overall looks fine, just a few comments:

@@ -237,26 +243,49 @@ void pci_setup(void)
          pci_writew(devfn, PCI_COMMAND, cmd);
      }

-    /*
-     * 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_hole_size )
+    {
+        uint64_t max_ram_below_4g = (1ULL << 32) - mmio_hole_size;
+
+        if ( max_ram_below_4g > HVM_BELOW_4G_MMIO_START )
+        {
+            printf("max_ram_below_4g=0x"PRIllx
+                   " too big for mmio_hole_size=0x"PRIllx
+                   " has been ignored.\n",
+                   PRIllx_arg(max_ram_below_4g),
+                   PRIllx_arg(mmio_hole_size));
+        }
+        else
+        {
+            pci_mem_start = max_ram_below_4g;
+            printf("pci_mem_start=0x%lx (was 0x%x) for mmio_hole_size=%lu\n",
+                   pci_mem_start, HVM_BELOW_4G_MMIO_START,
+                   (long)mmio_hole_size);
+        }
+    }
+    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;
+    }
I don't think these need to be disjoint.  There's no reason you
couldn't set the size for the default, and then allow the code to make
it bigger for guests which allow that.

The support for changing mmio_hole_size is still "missing" from QEMU.
So this code only works for qemu-traditional.  I think Jan said
back on v1 or v2 (sorry, e-mail issues) that since this is a config,
disable the auto changing code.


>  But if this needs to be rushed to get in, this is fine with me.

:)

      if ( mmio_total > (pci_mem_end - pci_mem_start) )
      {
diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index c81a25b..94da7fa 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 mmio_hole_size)
+{
+    if (mmio_hole_size)
+    {
+        uint64_t max_ram_below_4g = (1ULL << 32) - mmio_hole_size;
+
+        if (max_ram_below_4g < HVM_BELOW_4G_MMIO_START)
+        {
+            args->mmio_size = mmio_hole_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 mmio_hole_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, mmio_hole_size);
  }

  /*
diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h
index 40bbac8..d35f38d 100644
--- a/tools/libxc/xenguest.h
+++ b/tools/libxc/xenguest.h
@@ -244,12 +244,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 mmio_hole_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 mmio_hole_size);
Why on earth do we need all of these extra functions?  Particularly
ones like xc_hvm_build_target_mem_with_hole(), which isn't even called
by anyone, AFAICT?

int xc_hvm_build_target_mem(xc_interface *xch,
                           uint32_t domid,
                           int memsize,
                           int target,
                           const char *image_name)
{
    return xc_hvm_build_target_mem_with_hole(xch, domid, memsize, target,
                                             image_name, 0);
}

So it is called...


libxc isn't a stable interface -- can't we just have the callers set
mmio_size in hvm_args and have them call xc_hvm_build()?

I am looking into this.  I have was not sure that I could change or
drop xc_hvm_build_target_mem().


@@ -696,10 +698,28 @@ 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.mmio_hole_size) {
+            uint64_t max_ram_below_4g = (1ULL << 32) -
+                b_info->u.hvm.mmio_hole_size;
+
+            if (max_ram_below_4g > HVM_BELOW_4G_MMIO_START) {
+                LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
+                           "max-ram-below-4g=%"PRIu64
+                           " (mmio_hole_size=%"PRIu64
+                           ") too big > %u ignored.\n",
+                           max_ram_below_4g,
+                           b_info->u.hvm.mmio_hole_size,
+                           HVM_BELOW_4G_MMIO_START);
+            } else {
+                machinearg = libxl__sprintf(gc, "%s,max-ram-below-4g=%"PRIu64,
+                                            machinearg, max_ram_below_4g);
+            }
+        }
What happens if the version of qemu that's installed doesn't support
max-ram-below-4g?

QEMU will report an error and abort.  Guest will not start.

    -Don Slutz

  -George


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.