Re: [Xen-devel] [PATCH 01/19] libxl/xl: move some HVM/PV specific fields of libxl_domain_build_info

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Thu, Aug 24, 2017 at 12:27:25PM +0100, Wei Liu wrote:
> On Tue, Aug 22, 2017 at 10:49:02AM +0100, Roger Pau Monne wrote:
> >      /* If the full path is not specified, check in the libexec path */
> >      if ( bootloader[0] != '/' ) {
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index 1158303e1a..2f03ebe586 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -406,6 +406,28 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> >              libxl_domain_type_to_string(b_info->type));
> >          return ERROR_INVAL;
> >      }
> > +
> > +    if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
> > +        libxl_defbool_setdefault(&b_info->nested_hvm,
> > +                                 
> > libxl_defbool_val(b_info->u.hvm.nested_hvm));
> > +        libxl_defbool_setdefault(&b_info->apic,
> > +                                 libxl_defbool_val(b_info->u.hvm.apic));
> > +    } else {
> > +        libxl_defbool_setdefault(&b_info->nested_hvm, false);
> > +        libxl_defbool_setdefault(&b_info->apic, true);
> > +    }
> > +
> 
> Something is missing: If the old field is set, it should be copied to
> the new field.

That's already done if the guest type is HVM (see above). If the guest
type is not HVM the default values are set.

> > +    if (b_info->timer_mode == LIBXL_TIMER_MODE_DEFAULT)
> > +        b_info->timer_mode = b_info->type == LIBXL_DOMAIN_TYPE_HVM
> > +                             ? b_info->u.hvm.timer_mode
> > +                             : LIBXL_TIMER_MODE_NO_DELAY_FOR_MISSED_TICKS;
> > +
> > +    if (b_info->type == LIBXL_DOMAIN_TYPE_PV && !b_info->bootloader) {
> > +        assert(!b_info->bootloader_args);
> > +        b_info->bootloader = b_info->u.pv.bootloader;
> > +        b_info->bootloader_args = b_info->u.pv.bootloader_args;
> > +    }
> 
> > +
> >      return 0;
> >  }
> [...]
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 6e80d36256..bf1652d367 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -506,10 +506,17 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >      # 65000 which is reserved by the toolstack.
> >      ("device_tree",      string),
> >      ("acpi",             libxl_defbool),
> > +    ("bootloader",       string),
> > +    ("bootloader_args",  libxl_string_list),
> > +    ("timer_mode",       libxl_timer_mode),
> > +    ("nested_hvm",       libxl_defbool),
> > +    ("apic",             libxl_defbool),
> 
> I'm not too sure about moving these here. They aren't needed for arm,
> for example. Maybe we should introduce arch_x86?

nested_hvm I guess will make sense for ARM at some point. The others
I'm not sure, timer_mode maybe, the rest look quite x86 specific
(although ARM is using the PV sub-struct IIRC, which already has
bootloader and bootloader_args).

apic certainly doesn't make sense to ARM, the more that I don't ever
see Xen creating an ARM guest without a GIC, which AFAIK is the ARM
equivalent of the x86 APIC.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel