Re: [PATCH v3 8/8] libxl, hvmloader: Don't relocate memory for MMIO hole

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

On 20/06/13 18:38, Stefano Stabellini wrote:
> On Thu, 20 Jun 2013, George Dunlap wrote:
>> At the moment, qemu-xen can't handle memory being relocated by
>> hvmloader.  This may happen if a device with a large enough memory
>> region is passed through to the guest.  At the moment, if this
>> happens, then at some point in the future qemu will crash and the
>> domain will hang.  (qemu-traditional is fine.)
>>
>> It's too late in the release to do a proper fix, so we try to do
>> damage control.
>>
>> hvmloader already has mechanisms to relocate memory to 64-bit space
>> if it can't make a big enough MMIO hole.  By default this is 2GiB; if
>> we just refuse to make the hole bigger if it will overlap with guest
>> memory, then the relocation will happen by default.
>>
>> v3:
>>   - Fix polarity of comparison
>>   - Move diagnostic messages to another patch
>>   - Tested with xen platform pci device hacked to have different BAR sizes
>>     {256MiB, 1GiB} x {qemu-xen, qemu-traditional} x various memory
>>     configurations
>>   - Add comment explaining why we default to "allow"
>>   - Remove cast to bool
>> v2:
>>   - style fixes
>>   - fix and expand comment on the MMIO hole loop
>>   - use "%d" rather than "%s" -> (...)?"1":"0"
>>   - use bool instead of uint8_t
>>   - Move 64-bit bar relocate detection to another patch
>>   - Add more diagnostic messages
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>> CC: Hanweidong <hanweidong@huawei.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Keir Fraser <keir@xen.org>
>> ---
>>   tools/firmware/hvmloader/pci.c          |   49 +++++++++++++++++++++++++++++--
>>   tools/libxl/libxl_dm.c                  |    6 ++++
>>   xen/include/public/hvm/hvm_xs_strings.h |    1 +
>>   3 files changed, 54 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
>> index 3108c8a..2364177 100644
>> --- a/tools/firmware/hvmloader/pci.c
>> +++ b/tools/firmware/hvmloader/pci.c
>> @@ -27,6 +27,8 @@
>>   
>>   #include <xen/memory.h>
>>   #include <xen/hvm/ioreq.h>
>> +#include <xen/hvm/hvm_xs_strings.h>
>> +#include <stdbool.h>
>>   
>>   unsigned long pci_mem_start = PCI_MEM_START;
>>   unsigned long pci_mem_end = PCI_MEM_END;
>> @@ -57,6 +59,32 @@ void pci_setup(void)
>>       } *bars = (struct bars *)scratch_start;
>>       unsigned int i, nr_bars = 0;
>>   
>> +    const char *s;
>> +    /*
>> +     * Do we allow hvmloader to relocate guest memory in order to
>> +     * increase the size of the lowmem MMIO hole?  Defaulting to 1
>> +     * here will mean that non-libxl toolstacks (including xend and
>> +     * home-grown ones) will experience this series as "no change".
>> +     * It does mean that those using qemu-xen will still experience
>> +     * the bug (described below); but it also means that those using
>> +     * qemu-traditional will *not* experience any change; and it also
>> +     * means that there is a work-around for those using qemu-xen,
>> +     * namely switching to qemu-traditional.
>> +     *
>> +     * If we defaulted to 0, and failing to resize the hole caused any
>> +     * problems with qemu-traditional, then there is no work-around.
>> +     *
>> +     * Since xend can't talk to qemu-traditional, I think this is the
>> +     * option that will have the least impact.
>> +     */
>> +    bool allow_memory_relocate = 1;
>> +
>> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
>> +    if ( s )
>> +        allow_memory_relocate = strtoll(s, NULL, 0);
> Considering that strtoll retuns a long long, are we sure that this
> allocation does what we want for all the possible long long values
> that can be returned?
>
> For example, if strtoll returns -1, do we want allow_memory_relocate to
> be set to true?

The only valid values here are "0" and "1"; everything else is undefined.

Look, the bike shed is already painted, the brushes have been washed and 
put away.  Leave it be. :-)

>
>
>
>> +    printf("Relocating guest memory for lowmem MMIO space %s\n",
>> +           allow_memory_relocate?"enabled":"disabled");
>> +
>>       /* Program PCI-ISA bridge with appropriate link routes. */
>>       isa_irq = 0;
>>       for ( link = 0; link < 4; link++ )
>> @@ -208,8 +236,25 @@ void pci_setup(void)
>>           pci_writew(devfn, PCI_COMMAND, cmd);
>>       }
>>   
>> -    while ( (mmio_total > (pci_mem_end - pci_mem_start)) &&
>> -            ((pci_mem_start << 1) != 0) )
>> +    /*
>> +     * 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_total > (pci_mem_end - pci_mem_start) ) {
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index ac1f90e..342d2ce 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -1154,6 +1154,12 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
>>           libxl__xs_write(gc, XBT_NULL,
>>                           libxl__sprintf(gc, "%s/hvmloader/bios", path),
>>                           "%s", libxl_bios_type_to_string(b_info->u.hvm.bios));
>> +        /* Disable relocating memory to make the MMIO hole larger
>> +         * unless we're running qemu-traditional */
>> +        libxl__xs_write(gc, XBT_NULL,
>> +                        libxl__sprintf(gc, "%s/hvmloader/allow-memory-relocate", path),
>> +                        "%d",
>> +                        b_info->device_model_version==LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL);
> line length

Blame the person who made a *47-character* identifier.

Seriously, would you rather have the "==" on a separate line or 
something? That seems really ridiculous.

I'll break the sprintf though.

  -George