Re: [Xen-devel] [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Aug 22, 2018 at 06:48:05PM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 22/08/18 16:18, Roger Pau Monné wrote:
> > On Wed, Aug 22, 2018 at 04:00:45PM +0100, Julien Grall wrote:
> > > -void libxl__arch_domain_build_info_setdefault(libxl_domain_build_info 
> > > *b_info)
> > > +void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> > > +                                              libxl_domain_build_info 
> > > *b_info)
> > >   {
> > >       /* ACPI is disabled by default */
> > >       libxl_defbool_setdefault(&b_info->acpi, false);
> > > +
> > > +    /*
> > > +     * Arm guest are now considered as PVH by the toolstack. To allow
> > > +     * compatibility with previous toolstack, PV guest are automatically
> > > +     * converted to PVH.
> > > +     */
> > > +    if (b_info->type != LIBXL_DOMAIN_TYPE_PV)
> > > +        return;
> > > +
> > > +    LOG(WARN, "Converting PV guest to PVH");
> > > +    LOG(WARN, "ARM guest are now PVH. Please update your toolstack.");
> > 
> > "Please update your configuration file.". Updating the toolstack won't
> > make this message go away AFAICT :).
> 
> In the case of libvirt, you don't have PVH support. So you would need to
> update your toolstack here.
> 
> How about "Please fix your configuration file/toolstack"?

I would use "fix your configuration or update your toolstack", but I'm
fine with the above.

> > 
> > > +
> > > +    b_info->type = LIBXL_DOMAIN_TYPE_PVH;
> > > +
> > > +    /*
> > > +     * They only field in u.pv that matters on Arm are: kernel, cmdline,
> > > +     * ramdisk.
> > > +     */
> > > +
> > > +    if (!b_info->kernel && b_info->u.pv.kernel)
> > > +            b_info->kernel = b_info->u.pv.kernel;
> > > +
> > > +    if (!b_info->ramdisk && b_info->u.pv.ramdisk)
> > > +        b_info->ramdisk = b_info->u.pv.ramdisk;
> > > +
> > > +    if (!b_info->cmdline && b_info->u.pv.cmdline)
> > > +        b_info->cmdline = b_info->u.pv.cmdline;
> > > +
> > > +    /* Reset b_info->u.pvh to default values */
> > > +    memset(&b_info->u.pvh, 0, sizeof(b_info->u.pvh));
> > 
> > I'm afraid that's not correct. The default values for u.pvh are set
> > by libxl__domain_build_info_setdefault.
> 
> I thought that this should be covered by the switch right after the call of
> libxl__arch_domain_build_info_setdefault. Did I miss anything?

Oh right, libxl__arch_domain_build_info_setdefault is called by
libxl__domain_build_info_setdefault.

> What I wanted to do here is resetting the union to 0 so you don't get data
> mangled by the pv fields.

Another possible option I think would be to mark those fields as
deprecated in the IDL, and libxl__domain_build_info_copy_deprecated
will take care of copying them to the new place. In fact I think all
guest types should be using the top level kernel, ramdisk and cmdline
fields.

I'm not specially comfortable with changing the guest type in the
middle of libxl__domain_build_info_setdefault, but I also don't have a
much better suggestion apart from using the deprecation helper.

From what you say above I assume bootloader or bootloader arguments
are not used by ARM?

> > 
> > >   }
> > >   /*
> > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > > index d4fa06daea..a6431c5d3f 100644
> > > --- a/tools/libxl/libxl_create.c
> > > +++ b/tools/libxl/libxl_create.c
> > > @@ -215,7 +215,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> > >       if (!b_info->event_channels)
> > >           b_info->event_channels = 1023;
> > > -    libxl__arch_domain_build_info_setdefault(b_info);
> > > +    libxl__arch_domain_build_info_setdefault(gc, b_info);
> > >       libxl_defbool_setdefault(&b_info->dm_restrict, false);
> > >       switch (b_info->type) {
> > > diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> > > index 81523a568f..8b6759c089 100644
> > > --- a/tools/libxl/libxl_x86.c
> > > +++ b/tools/libxl/libxl_x86.c
> > > @@ -613,7 +613,8 @@ int 
> > > libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
> > >       return rc;
> > >   }
> > > -void libxl__arch_domain_build_info_setdefault(libxl_domain_build_info 
> > > *b_info)
> > > +void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> > > +                                              libxl_domain_build_info 
> > > *b_info)
> > >   {
> > >       libxl_defbool_setdefault(&b_info->acpi, true);
> > >   }
> > > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> > > index 971ec1bc56..0bda28152b 100644
> > > --- a/tools/xl/xl_parse.c
> > > +++ b/tools/xl/xl_parse.c
> > > @@ -1286,7 +1286,11 @@ void parse_config_data(const char *config_source,
> > >       }
> > >       if (c_info->type == LIBXL_DOMAIN_TYPE_INVALID)
> > > +#if defined(__arm__) || defined(__aarch64__)
> > 
> > I think #ifdef CONFIG_ARM should DTRT and it's cleaner IMO.
> 
> CONFIG_ARM is not defined in the tools C source. So that's the only way to
> know if you are on Arm. This follows what is done in libxc.
> 
> I would be happy to introduce CONFIG_ARM/CONFIG_X86 if people thinks this
> would be useful in other places.

The tools makefile already uses CONFIG_ARM/X86, so I think it would
make sense to have this for the code as well. In any case, I don't
feel this should be done just for this patch, so I'm fine as-is.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel