Re: hvmloader - allow_memory_relocate overlaps

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.12.2023 19:08, Neowutran wrote:
> On 2023-12-19 17:12, Jan Beulich wrote:
>> 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?
> No, I am using "qemu_xen" ( from  
> xenstore-read -> 'device-model =
> "qemu_xen"' 
> 
>> Otherwise isn't libxl suppressing this behavior anyway?
> If by "isn't libxl suppressing this behavior" you means if libxl is setting
> the value of "allow_memory_relocate", then the answer is no. 
> 
> Following this lead, I checked in what code path
> "allow_memory_relocate" could be defined. 
> 
> It is only defined in one code path, 
> 
> In the file "tools/libs/light/libxl_dm.c",
> in the function "void libxl__spawn_local_dm(libxl__egc *egc, 
> libxl__dm_spawn_state *dmss)": 
> 
> '''
>  // ...
>     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
>  // ...
> 
>         libxl__xs_printf(gc, XBT_NULL,
>                          GCSPRINTF("%s/hvmloader/allow-memory-relocate", 
> path),
>                          "%d",
>                          
> b_info->device_model_version==LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL 
> &&
>                          !libxl__vnuma_configured(b_info));
> // ...
> '''
> 
> However, on QubesOS (my system), "local_dm" is never used, "stub_dm"
> is always used. 
> 
> In the function "void lib 
> xl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)", 
> the value of "allow_memory_relocate" is never defined. 
> 
> I tried to patch the code to define "allow_memory_relocate" in
> "libxl__spawn_stub_dm": 
> 
> '''
> --- a/tools/libs/light/libxl_dm.c
> +++ b/tools/libs/light/libxl_dm.c
> @@ -2431,6 +2431,10 @@ void libxl__spawn_stub_dm(libxl__egc *egc, 
> libxl__stub_dm_spawn_state *sdss)
>                                         libxl__xs_get_dompath(gc, 
> guest_domid)),
>                          "%s",
>                          
> libxl_bios_type_to_string(guest_config->b_info.u.hvm.bios));
> +        libxl__xs_printf(gc, XBT_NULL,
> +                         libxl__sprintf(gc, 
> "%s/hvmloader/allow-memory-relocate", libxl__xs_get_dompath(gc, guest_domid)),
> +                         "%d",
> +                         0);
>      }
>      ret = xc_domain_set_target(ctx->xch, dm_domid, guest_domid);
>      if (ret<0) {
> '''
> 
> And it is indeed another way to solve my issue. 
> However I do not understand the xen hypervisor enough to known i 
> f
> "allow-memory-relocate" should have been defined in
> "libxl__spawn_stub_dm" or if the real issue is somewhere else. 

I think it should be done the same no matter where qemu runs. Back at the
time iirc only qemu-trad could run in a stubdom, which may explain the
omission. Cc-ing the two guys who are likely in the best position to tell
for sure.

>>> 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.
> 
> tools/firmware/ 
> hvmloader/pci.c, function "void pci_setup(void)"
> I added a log to print if the hvmloader would have tried to overlaps
> guest memory
> 
> '''
> 
> +        printf("NEOWUTRAN pci.c: pci_mem_end: %d\n",pci_mem_end);
> +        printf("NEOWUTRAN pci.c: pci_mem_start: %d\n",pci_mem_start);
> +        printf("NEOWUTRAN pci.c: allow_memory_relocate: 
> %d\n",allow_memory_relocate);
> +        printf("NEOWUTRAN pci.c: hvm_info->low_mem_pgend: 
> %d\n",hvm_info->low_mem_pgend);
> +
> +
>         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))
> +                && ((pci_mem_start << 1) != 0)
> +                && (1
> +                    || (((pci_mem_start << 1) >> PAGE_SHIFT)
> +                        >= hvm_info->low_mem_pgend)) ){
> +                
> printf("NEOWUTRAN pci.c: HVM_LOADER would have tried to relocate guest 
> memory\n");
> +               printf("NEOWUTRAN pci.c: pci_mem_start: %d\n",pci_mem_start);
> +
> +        }
> '''
> 
> And the associated result is: 
> 
> (d22) NEOWUTRAN pci.c: pci_mem_end: -67108864
> (d22) NEOWUTRAN pci.c: pci_mem_start: -268435456
> (d22) NEOWUTRAN pci.c: allow_memory_relocate: 0
> (d22) NEOWUTRAN pci.c: hvm_info->low_mem_pgend: 983040
> (d22) NEOWUTRAN pci.c: HVM_LOADER would have tried to relocate guest memory
> (d22) NEOWUTRAN pci.c: pci_mem_start: -268435456
> 
> So if "allow_memory_relocate" was not defined to 0, the hvmloader
> would have tried to overlaps guest memory, and it seems that is
> something that qemu_xen cannot handle.

Well, memory addresses printed in decimal are pretty hard to work with.
But I'd like to ask anyway that you supply all of the log messages for
such a guest starting, to be able to correlate what you've added with
other items also logged.

Jan