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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Wei,

On 28/08/18 17:45, Wei Liu wrote:
> On Thu, Aug 23, 2018 at 09:58:57AM +0200, Roger Pau Monné wrote:
> [...]
> > > 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
> I think this is a better approach.
>
> > 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.
> I think CONFIG_ARM should already work.
I don't think so. The top makefile does not pass -DCONFIG_ARM on 
compiler command line. This is only done in sub-directory such as 
console and libacpi.
> There are several CONFIG_ARM* in toolstack code, though not in libxl. In
> any case, I think the code is fine as-is.
I am happy to introduce CONFIG_ARM/CONFIG_X86 if you don't mind adding 
-DCONFIG_ARM on the command line.
Cheers,

--
Julien Grall

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