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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 10/01/2018 04:27 PM, Roger Pau Monné wrote:
> 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.
That's not what we discussed before. The idea is to default Arm guest to 
PVH for xl. But "type=" would still be available.
We still have to support other type as other toolstack (e.g libvirt) may 
use PV for Arm. So the idea here is libxl will check if it was PV and 
convert to PVH.
In an earlier reply, you said that we can use deprecated to copy most of 
the options over. However, can we always assume the pv.* fields will 
always be zero after the value was copied over? If not, then we would 
have to memset them.
I asked opinion on the last bits.


Cheers,

--
Julien Grall

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