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

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

On Tue, Sep 25, 2018 at 08:36:36PM +0100, Julien Grall wrote:
> Hi Roger,
> 
> Sorry for the late reply.
> 
> On 09/03/2018 03:40 PM, Roger Pau Monné wrote:
> > On Mon, Sep 03, 2018 at 12:15:16PM +0100, Julien Grall wrote:
> > > On 03/09/18 12:09, Julien Grall wrote:
> > > > 
> > > > 
> > > > On 23/08/18 08:58, Roger Pau Monné wrote:
> > > > > On Wed, Aug 22, 2018 at 06:48:05PM +0100, Julien Grall wrote:
> > > > > > > 
> > > > > > > > +
> > > > > > > > +    b_info->type = LIBXL_DOMAIN_TYPE_PVH;
> > > > > > > > +
> > > > > > > > +    /*
> > > > > > > > +     * They only fie ld 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 will have a look at it.
> > > > 
> > > > > 
> > > > > 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.
> > > 
> > > I forgot to answer to this bit. I don't think the deprecation helper will 
> > > do
> > > all the jobs. There are still PV specific parameters: slack_memkb, 
> > > features,
> > > e820_host.
> > 
> > Those can be left inside the PV sub-struct and shouldn't be marked as
> > deprecated.
> > 
> > > Those are not necessary for Arm, if you don't zero them then you will not
> > > initialize the PVH structure with default values. How do you suggest to
> > > handle them?
> > 
> > But I guess ARM doesn't use any of those fields (or else they should
> > be moved to the pvh sub-struct anyway)?
> 
> Those fields should not be used by Arm. Looking at the current fields in pv,
> they all should be zeroed.
> 
> However, I am not sure if we can assume that will always be the case.
> If we can assume it, then I think it would just be sufficient to have
> b_info->type = LIBXL_DOMAIN_TYPE_PVH in Arm.
> 
> Any thoughts?

If we go down this route for ARM I think you should add to xl.cfg that
the `type` option only applies to x86, and that there's only one guest
type on ARM. IMO it's best to not use type if there's only one type
available, so that it can be intruded later on ARM if required.

Thanks, Roger.

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