Re: hvmloader - allow_memory_relocate overlaps

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Dec 22, 2023 at 12:35:57PM +0100, Jan Beulich wrote:
> 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.

This indeed looks like a missing piece of qemu-xen support in
stubdomain.
But also, As Jan pointed out, this would be better fixed at the qemu
side so memory relocation could be re-enabled.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature