Re: hvmloader - allow_memory_relocate overlaps

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.12.2023 08:01, Neowutran wrote:
> I am wondering if the variable "allow_memory_relocate" is still
> relevant today and if its default value is still relevant. 
> Should it be defined to 0 by default instead of 1 (it seems to be a
> workaround for qemu-traditional, so maybe an outdated default value ? ) ? 

So are you saying you use qemu-trad? Otherwise isn't libxl suppressing
this behavior anyway? Or did that code go stale?

> Code extract: 
> 
> tools/firmware/hvmloader/pci.c 
> "
>    /*
>      * 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) means that those using qemu-xen will still
>      * experience the memory relocation bug described below; but it
>      * also means that those using q 
> emu-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 only use qemu-traditional, I think this is the
>      * option that will have the least impact.
>      */
>     bool allow_memory_relocate = 1;
> "
> 
> "
>         /*
>          * 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 t
> he 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;
> "
> 
> The issue it cause is documented in the source code: guest memory can
> be trashed by the hvmloader. 
> 
> Due to this issue, it is impossible to passthrough a large PCI device to a 
> HVM with a lot of ram.
> (https://github.com/QubesOS/qubes-issues/issues/4321). 
> 
> (Forcing "allow_memory_relocate" to be 0 seems to solve the issue
> linked)

What I don't understand here (and what I also can't find helpful logs for)
is: The code in hvmloader is in principle intended to work in both cases.
If there's suspected guest memory corruption, can we get to see the actual
results of the MMIO hole creation and its using for device ranges? If there
indeed is an overlap with guest RAM, that's a bug which wants fixing. I
don't see why we would want to drop allow_memory_relocate in the way you
suggest; quite the other way around, if upstream qemu still can't tolerate
hvmloader doing this, then it wants fixing there such that RAM relocation
can be enabled unconditionally. Then the variable will indeed not be needed
anymore.

Jan