Re: [PATCH v5 1/1] Add mmio_hole_size

[George Dunlap <george.dunlap@eu.citrix.com>]

On 10/02/2014 08:36 AM, Jan Beulich wrote:
>>>> On 01.10.14 at 18:33, <dslutz@verizon.com> wrote:
>> On 09/30/14 09:15, George Dunlap wrote:
>>> On Thu, Sep 11, 2014 at 5:20 PM, Don Slutz <dslutz@verizon.com> wrote:
>>>> @@ -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.
> Because it didn't seem like you would want to properly take care
> of both cases together (iirc the fact that the configured hole size
> could be other than a power of 2 introduced a conflict with the
> current resizing logic). I.e. doing one or the other is a suitable
> first step imo, but with room for improvement.

Right -- I hadn't thought about the power of 2 thing for the current 
resizing logic.  That will take some actual fixing, so it's probably 
best if we put that off until later.

>
>>>> --- 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...
> You're kidding, aren't you? If this is the only caller, we can very well
> do without the ..._with_hole() one.

Indeed, I didn't even get what Don was saying at first.  When I looked, 
xc_hvm_build* was called exactly twice: xc_hvm_build() was called in 
libxl, and xc_hvm_build_target_mem() was called by xend. I think 
originally those functions were there for xapi compatibility (I was 
working on a XenServer feature when I wrote these; and this was before 
libxl was really a thing).

But now that we have libxl, breaking libxc compatibility is a feature to 
push people to use the libxl interfaces instead. :-)

  -George