Re: [PATCH v3 3/6] libxl: introduce MSR data in libxl_cpuid_policy

[Anthony PERARD <anthony.perard@citrix.com>]

On Thu, Jul 20, 2023 at 10:25:37AM +0200, Roger Pau Monne wrote:
> ---
> It would be nice to rename the json output field to 'cpu_policy'
> instead of 'cpuid', so that it looks like:
> 
> "cpu_policy": {
>     "cpuid": [
>         {
>             "leaf": 7,
>             "subleaf": 0,
>             "edx": "xx1xxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
>         },
>         {
>             "leaf": 1,
>             "ebx": "xxxxxxxxxxxxxxxx00010000xxxxxxxx"
>         }
>         }
>         }
>     ],
>     "msr": [
>         {
>             "index": 266,
>             "policy": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx1xx1x1"
>         }
>     ]
> },
> 
> Sadly I have no idea how to do that, and can be done in a followup
> change anyway.

I don't think that would be possible. I think that would mean renaming
the field in "struct libxl_domain_build_info", and changing a fields
name seems complicated.

> diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> index 3c8b2a72c0b8..68b797886642 100644
> --- a/tools/libs/light/libxl_cpuid.c
> +++ b/tools/libs/light/libxl_cpuid.c
> @@ -592,17 +641,24 @@ int libxl__cpuid_policy_list_parse_json(libxl__gc *gc,
>  {
>      int i, size;
>      struct xc_xend_cpuid *l;
> +    struct xc_msr *msr;
> +    const libxl__json_object *co;
>      flexarray_t *array;
>  
> -    if (!libxl__json_object_is_array(o))
> +    if (!libxl__json_object_is_map(o))
>          return ERROR_FAIL;

We still need to be able to migrate a VM from a previous release of Xen,
and we are going to have an array instead of a map. Could you try to
handle both the old and new format of "cpuid"? If we don't then the
scenario "migrate then reboot" is going to fail to use the expected cpu
policy.

> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index 9e48bb772646..887824fdd828 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -19,7 +19,7 @@ libxl_mac = Builtin("mac", json_parse_type="JSON_STRING", passby=PASS_BY_REFEREN
>  libxl_bitmap = Builtin("bitmap", json_parse_type="JSON_ARRAY", dispose_fn="libxl_bitmap_dispose", passby=PASS_BY_REFERENCE,
>                         check_default_fn="libxl_bitmap_is_empty", copy_fn="libxl_bitmap_copy_alloc")
>  libxl_cpuid_policy_list = Builtin("cpuid_policy_list", dispose_fn="libxl_cpuid_dispose", passby=PASS_BY_REFERENCE,
> -                                  json_parse_type="JSON_ARRAY", check_default_fn="libxl__cpuid_policy_is_empty",
> +                                  json_parse_type="JSON_MAP", check_default_fn="libxl__cpuid_policy_is_empty",

Maybe we should have json_parse_type as either "JSON_ARRAY | JSON_MAP"
or just "JSON_ANY" since nothing beside libxl is supposed to do
something with it, and when migrating from a previous version, we are
going to have an array.

I don't think anything is using json_parse_type for this
libxl_cpuid_policy_list, so I don't think it matter which type we use.


Thanks,

-- 
Anthony PERARD