Re: [PATCH 2/6] tools/xc: Add xc_cpu_policy to the public xenctrl.h header

[Alejandro Vallejo]

On 19/03/2024 17:55, Roger Pau Monné wrote:
> On Tue, Jan 09, 2024 at 03:38:30PM +, Alejandro Vallejo wrote:
>> Move struct xc_cpu_policy data structure out of xg_private.h and into
>> the public xenguest.h so it can be used by libxl.
> 
> I will let Andrew comment on this one, IIRC the initial idea was to
> not leak cpu_policy into libxl, and to instead have it as an opaque
> object that libxl can interact with using helpers.

I wrote this a while ago, and can't quite remember why I left this here.
Looking closely I think it's leftover from something else I eventually
turned into its own series[1]. I don't _think_ it's needed for this
series, so I'll just drop it in v2. If everything blows up I'll tell you
why I needed it :)

[1]
https://lore.kernel.org/xen-devel/20240207173957.19811-1-alejandro.vall...@cloud.com/

> 
> I haven't looked at followup patches - I assume this is done to
> manipulate the cpuid data more easily from libxl, and ultimately drop
> xc_xend_cpuid?

Yes, we can't drop that altogether because xl exposes an interface
allowing fine-grained manipulation of CPUID/MSR data, but it becomes
minimal. The general idea is to is to stop moving separate buffers
around in tandem and use a single data structure instead whenever possible.

Cheers,
Alejandro

Re: [PATCH 2/6] tools/xc: Add xc_cpu_policy to the public xenctrl.h header

[Roger Pau Monné]

On Tue, Jan 09, 2024 at 03:38:30PM +, Alejandro Vallejo wrote:
> Move struct xc_cpu_policy data structure out of xg_private.h and into
> the public xenguest.h so it can be used by libxl.

I will let Andrew comment on this one, IIRC the initial idea was to
not leak cpu_policy into libxl, and to instead have it as an opaque
object that libxl can interact with using helpers.

I haven't looked at followup patches - I assume this is done to
manipulate the cpuid data more easily from libxl, and ultimately drop
xc_xend_cpuid?

> Signed-off-by: Alejandro Vallejo 
> ---
>  tools/include/xenguest.h |  8 +++-
>  tools/libs/guest/xg_private.h| 10 --
>  xen/include/xen/lib/x86/cpu-policy.h |  5 +
>  3 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
> index e01f494b77..4e9078fdee 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 
> +
> +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;
>  
>  /* Create and free a xc_cpu_policy object. */
>  xc_cpu_policy_t *xc_cpu_policy_init(void);
> diff --git a/tools/libs/guest/xg_private.h b/tools/libs/guest/xg_private.h
> index d73947094f..d1940f1ea4 100644
> --- a/tools/libs/guest/xg_private.h
> +++ b/tools/libs/guest/xg_private.h
> @@ -170,14 +170,4 @@ int pin_table(xc_interface *xch, unsigned int type, 
> unsigned long mfn,
>  #define M2P_SIZE(_m)ROUNDUP(((_m) * sizeof(xen_pfn_t)), M2P_SHIFT)
>  #define M2P_CHUNKS(_m)  (M2P_SIZE((_m)) >> M2P_SHIFT)
>  
> -#if defined(__x86_64__) || defined(__i386__)
> -#include 
> -
> -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];
> -};
> -#endif /* x86 */
> -
>  #endif /* XG_PRIVATE_H */
> diff --git a/xen/include/xen/lib/x86/cpu-policy.h 
> b/xen/include/xen/lib/x86/cpu-policy.h
> index 14724cedff..65f6335b32 100644
> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -85,6 +85,11 @@ unsigned int x86_cpuid_lookup_vendor(uint32_t ebx, 
> uint32_t ecx, uint32_t edx);
>   */
>  const char *x86_cpuid_vendor_to_str(unsigned int vendor);
>  
> +#ifndef __XEN__
> +/* Needed for MAX() */
> +#include 
> +#endif /* __XEN__ */

I think for this header it is up to the user to provide the required
context, iow: it should be tools/include/xenguest.h to include
xen-tools/common-macros.h ahead of cpu-policy.h.  Otherwise we should
likely do the same for #ifdef __XEN__ branch and include whatever
header that defines MAX().

Thanks, Roger.