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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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.

There are several CONFIG_ARM* in toolstack code, though not in libxl. In
any case, I think the code is fine as-is.

Wei.

> Thanks, Roger.

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