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(¤t_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
