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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Aug 22, 2017 at 10:49:02AM +0100, Roger Pau Monne wrote:
> Move the HVM/PV sub-structure fields of libxl_domain_build_info into
> the top-level structure. xl is also modified to start using those
> fields.
> 
> This is required because those options will be used by the new PVH
> guest type.
> 
> Defines are added in order to signal consumers that the fields are
> available.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---

(Haven't done a detailed review)

>  
>      /* 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.


> +    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?

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