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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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.

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.

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?

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()?

> @@ -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?

 -George

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