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

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

On 06/18/2013 05:46 PM, 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.
>
> 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>
> ---
>   tools/firmware/hvmloader/pci.c          |   41 ++++++++++++++++++++++++++++---
>   tools/libxl/libxl_dm.c                  |    6 +++++
>   xen/include/public/hvm/hvm_xs_strings.h |    1 +
>   3 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> index 63d79a2..1ab5124 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;
> @@ -58,6 +60,15 @@ void pci_setup(void)
>       } *bars = (struct bars *)scratch_start;
>       unsigned int i, nr_bars = 0;
>
> +    const char *s;
> +    bool allow_memory_relocate = 1;
> +
> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
> +    if ( s )
> +        allow_memory_relocate = (bool)strtoll(s, NULL, 0);
> +    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++ )
> @@ -209,14 +220,38 @@ 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)) )

Hmm, the polarity on the comparison here is wrong -- it should be >=, not <.

I've hacked up qemu so that it presents a 256MiB xen platform pci 
device.  I'll re-send when I've done some more testing.

But I have, I think, reproduced the original problem; the above error 
means that for qemu-xen, it will resize the MMIO hole only if it *can* 
move guest memory. :-)

  -George