Re: [RFC PATCH v2 02/10] x86: Define arch LBR feature bits

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.01.2025 09:45, Tu Dinh wrote:
> --- a/xen/arch/x86/include/asm/cpufeature.h
> +++ b/xen/arch/x86/include/asm/cpufeature.h
> @@ -219,6 +219,11 @@ static inline bool boot_cpu_has(unsigned int feat)
>  #define cpu_has_rfds_no         boot_cpu_has(X86_FEATURE_RFDS_NO)
>  #define cpu_has_rfds_clear      boot_cpu_has(X86_FEATURE_RFDS_CLEAR)
>  
> +/* CPUID level 0x0000001c.eax */
> +
> +#define current_cpu_has_lbr_lip cpu_has(&current_cpu_data, \
> +                                        X86_FEATURE_LBR_LIP);

Why would this, unlike all other similar constructs, need to use
current_cpu_data rather than boot_cpu_has()? If there is a reason, it almost
certainly wants naming in the description.

> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -284,7 +284,7 @@ XEN_CPUFEATURE(SERIALIZE,     9*32+14) /*A  SERIALIZE 
> insn */
>  XEN_CPUFEATURE(HYBRID,        9*32+15) /*   Heterogeneous platform */
>  XEN_CPUFEATURE(TSXLDTRK,      9*32+16) /*a  TSX load tracking suspend/resume 
> insns */
>  XEN_CPUFEATURE(PCONFIG,       9*32+18) /*   PCONFIG instruction */
> -XEN_CPUFEATURE(ARCH_LBR,      9*32+19) /*   Architectural Last Branch Record 
> */
> +XEN_CPUFEATURE(ARCH_LBR,      9*32+19) /*s  Architectural Last Branch Record 
> */

The 's' here (and below) may only be added once all of the respective handling 
is
complete.

> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -22,6 +22,9 @@
>  #define FEATURESET_7d1       15 /* 0x00000007:1.edx    */
>  #define FEATURESET_m10Al     16 /* 0x0000010a.eax      */
>  #define FEATURESET_m10Ah     17 /* 0x0000010a.edx      */
> +#define FEATURESET_1Ca       18 /* 0x0000001c.eax      */
> +#define FEATURESET_1Cb       19 /* 0x0000001c.ebx      */
> +#define FEATURESET_1Cc       20 /* 0x0000001c.ecx      */
>  
>  struct cpuid_leaf
>  {
> @@ -85,7 +88,7 @@ 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 CPUID_GUEST_NR_BASIC      (0xdu + 1)
> +#define CPUID_GUEST_NR_BASIC      (0x1cu + 1)

Are you sure this can be done with no other prep work? I've been sitting
on AMX and AVX10 patches where I need to bump this, too. Yet I continue
to think that something along the lines of the 3-patch series at [1] is
necessary up front.

> @@ -158,6 +161,52 @@ struct cpu_policy
>              uint64_t :64, :64; /* Leaf 0xb - Topology. */
>              uint64_t :64, :64; /* Leaf 0xc - rsvd */
>              uint64_t :64, :64; /* Leaf 0xd - XSTATE. */
> +
> +            uint64_t :64, :64; /* Leaf 0xe - rsvd */
> +            uint64_t :64, :64; /* Leaf 0xf - rsvd */
> +            uint64_t :64, :64; /* Leaf 0x10 - rsvd */
> +            uint64_t :64, :64; /* Leaf 0x11 - rsvd */
> +            uint64_t :64, :64; /* Leaf 0x12 - rsvd */
> +            uint64_t :64, :64; /* Leaf 0x13 - rsvd */
> +            uint64_t :64, :64; /* Leaf 0x14 - rsvd */
> +            uint64_t :64, :64; /* Leaf 0x15 - rsvd */
> +            uint64_t :64, :64; /* Leaf 0x16 - rsvd */
> +            uint64_t :64, :64; /* Leaf 0x17 - rsvd */
> +            uint64_t :64, :64; /* Leaf 0x18 - rsvd */
> +            uint64_t :64, :64; /* Leaf 0x19 - rsvd */
> +            uint64_t :64, :64; /* Leaf 0x1a - rsvd */
> +            uint64_t :64, :64; /* Leaf 0x1b - rsvd */
> +
> +            union {
> +                uint32_t _1Ca;
> +                struct {
> +                    uint32_t supported_depths:8;

According to XEN_CPUFEATURE(LBR_DEPTH_...) further up these are 8 individual
bits. Further, is there a reason you don't use here what the additions there
produce in the generated header, but you rather re-define the fields from
scratch?

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2024-08/msg00591.html