This enables a set of follow-up simplifications in the toolstack.

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 tools/include/xenguest.h             |  8 +++++++-
 tools/libs/guest/xg_private.h        | 10 ----------
 xen/include/xen/lib/x86/cpu-policy.h |  6 ++++--
 3 files changed, 11 insertions(+), 13 deletions(-)

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>
+
+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 d73947094f2e..d1940f1ea482 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 <xen/lib/x86/cpu-policy.h>
-
-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 d5e447e9dc06..0cd454dfc0b6 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -85,6 +85,8 @@ 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);
 
+#define XENLIB_MAX(x, y) ((x) < (y) ? (y) : (x))
+
 #define CPUID_GUEST_NR_BASIC      (0xdu + 1)
 #define CPUID_GUEST_NR_CACHE      (5u + 1)
 #define CPUID_GUEST_NR_FEAT       (2u + 1)
@@ -92,8 +94,8 @@ const char *x86_cpuid_vendor_to_str(unsigned int vendor);
 #define CPUID_GUEST_NR_XSTATE     (62u + 1)
 #define CPUID_GUEST_NR_EXTD_INTEL (0x8u + 1)
 #define CPUID_GUEST_NR_EXTD_AMD   (0x21u + 1)
-#define CPUID_GUEST_NR_EXTD       MAX(CPUID_GUEST_NR_EXTD_INTEL, \
-                                      CPUID_GUEST_NR_EXTD_AMD)
+#define CPUID_GUEST_NR_EXTD       XENLIB_MAX(CPUID_GUEST_NR_EXTD_INTEL, \
+                                             CPUID_GUEST_NR_EXTD_AMD)
 
 /*
  * Maximum number of leaves a struct cpu_policy turns into when serialised for
-- 
2.34.1

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,

-- 
Anthony PERARD

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