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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Roger,

On 10/01/2018 05:37 PM, Roger Pau Monné wrote:
> On Mon, Oct 01, 2018 at 04:33:45PM +0100, Julien Grall wrote:
> > 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.
> Yes, the field is cleared/disposed after the value is copied over, see
> libxl_C_type_copy_deprecated in gentypes.py.
That's answer part of my question :). AFAIU, nothing promise that a 
default value will always be 0 (see libxl_devid). I am concerned that 
someone may decide to add such field in either PV or PVH in the future.
Anyway, I think I find a way to address that, I will resend a new 
version later today.
Cheers,

--
Julien Grall

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