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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Jul 24, 2023 at 06:26:42PM +0100, Anthony PERARD wrote:
> 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.

I did wonder whether it would be possible to change the json field
name without changing the libxl_domain_build_info field name, but that
would need a lot of special casing AFAICT.

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

Yeah, we need to use JSON_ANY and then guess the version by whether
the object is an array or a map.

That should be easy to arrange, let me prepare v4.

Thanks, Roger.