Re: [PATCH v5 1/1] Add mmio_hole_size

[Don Slutz <dslutz@verizon.com>]

On 09/30/14 09:15, George Dunlap wrote:
> On Thu, Sep 11, 2014 at 5:20 PM, Don Slutz <dslutz@verizon.com> 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@verizon.com>
> 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