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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jul 12, 2023 at 05:39:01PM +0100, Anthony PERARD wrote:
> On Tue, Jul 11, 2023 at 11:22:27AM +0200, Roger Pau Monne wrote:
> > Add a new array field to libxl_cpuid_policy in order to store the MSR
> > policies.
> > 
> > Note that libxl_cpuid_policy_list_{copy,length,parse_json,gen_json}
> > are not adjusted to deal with the new MSR array now part of
> > libxl_cpuid_policy_list.
> 
> Why? Isn't this going to be an issue? Or maybe that going to be dealt
> with in a future patch?

I'm unsure what's the point of those?  The CPUID/MSR data is passed as
a migration stream record from the hypervisor, so I don't see much
point into converting it into json.  It also seems quite complex, and
can't likely we done without breaking (or adjusting) the current
format.

My plan was to leave this unimplemented and if someone is in
interested in having the data in json they can as well implement it.

Would you be OK if I add a note to the commit message that
implementing json marshalling is left to implement for interested
parties?

> > 
> > Adding the MSR data in the libxl_cpuid_policy_list type is done so
> > that existing users can seamlessly pass MSR features as part of the
> > CPUID data, without requiring the introduction of a separate
> > domain_build_info field, and a new set of handlers functions.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> >  tools/libs/light/libxl_cpuid.c    | 6 +++++-
> >  tools/libs/light/libxl_internal.h | 1 +
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> > index 724cb4f182d4..65cad28c3ef0 100644
> > --- a/tools/libs/light/libxl_cpuid.c
> > +++ b/tools/libs/light/libxl_cpuid.c
> > @@ -40,6 +40,9 @@ void libxl_cpuid_dispose(libxl_cpuid_policy_list *pl)
> >          free(policy->cpuid);
> >      }
> >  
> > +    if (policy->msr)
> 
> You don't need to test for NULL, you can call free() in this case as
> well.

Right, thanks, will adjust.

Roger.