Re: [PATCH 1/3] tools/xg: Move xc_cpu_policy_t to xenguest.h

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 30/04/2024 15:13, Anthony PERARD wrote:
> On Wed, Feb 07, 2024 at 05:39:55PM +0000, Alejandro Vallejo wrote:
>> diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
>> index e01f494b772a..4e9078fdee4d 100644
>> --- a/tools/include/xenguest.h
>> +++ b/tools/include/xenguest.h
>> @@ -784,7 +784,13 @@ xen_pfn_t *xc_map_m2p(xc_interface *xch,
>>                        unsigned long *mfn0);
>>  
>>  #if defined(__i386__) || defined(__x86_64__)
>> -typedef struct xc_cpu_policy xc_cpu_policy_t;
>> +#include <xen/lib/x86/cpu-policy.h>
> 
> I don't think it's a good idea to expose "cpu-policy.h" to "xenguest.h",
> and it's going to break the build of every tools outside of xen
> repository that are using xenguest.h. xenguest.h is installed on a
> system, but cpu-policy.h isn't.
> 
>> +typedef struct xc_cpu_policy {
>> +    struct cpu_policy policy;
>> +    xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES];
>> +    xen_msr_entry_t msrs[MSR_MAX_SERIALISED_ENTRIES];
>> +} xc_cpu_policy_t;
> 
> Would using an accessor be an option to access `leaves` and `msrs` for
> "xen-cpuid" tool? That would avoid the need to expose the "cpu-policy.h"
> headers.
> 
> With accessors, we might not need to expose xc_cpu_policy_serialise() to
> the world anymore, and it could be internal to libxenguest, probably, I
> haven't check.
> 
> Thanks,
> 

That would work out for xen-cpuid.

I'm on the fence in the general case because I think it'd be
advantageous to let clients (i.e: xl) manipulate directly the
deserialized form of the policy. That would allow flipping features on
or off a heck of lot easier. We could create per-feature accessors, but
then we'll end up with _a lot_ of them.

I guess we'll get there when we get there. The xen-cpuid case definitely
doesn't warrant it. I'll give this a go.

Cheers,
Alejandro