Re: [PATCH 1/4] x86: Add Intel architectural LBR featureset bits

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.11.2024 09:49, ngoc-tu.dinh@xxxxxxxxxx wrote:
> From: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>
> 
> Expose ARCH_LBR feature to guests.
> 
> Extend CPU featureset with 3 words for CPUID leaf 0x1c.
> 
> Signed-off-by: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>

First a general remark: Please Cc maintainers on patch submissions.

> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -656,7 +656,7 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
> domid, bool restore,
>              p->policy.feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
>          }
>  
> -        p->policy.basic.max_leaf = min(p->policy.basic.max_leaf, 0xdu);
> +        p->policy.basic.max_leaf = min(p->policy.basic.max_leaf, 0x1cu);

We'll need to update this again and again (AMXC, AVX10) if we continue using
literal numbers here. This wants switching to ARRAY_SIZE(), and imo ideally
in a separate prereq patch (which could likely go straight in).

> --- a/tools/misc/xen-cpuid.c
> --- a/xen/arch/x86/cpu-policy.c
> +++ b/xen/arch/x86/cpu-policy.c
> @@ -271,6 +271,8 @@ static void recalculate_misc(struct cpu_policy *p)
>  
>      p->basic.raw[0xc] = EMPTY_LEAF;
>  
> +    zero_leaves(p->basic.raw, 0xe, 0x1b);
> +
>      p->extd.e1d &= ~CPUID_COMMON_1D_FEATURES;
>  
>      /* Most of Power/RAS hidden from guests. */
> @@ -303,6 +305,7 @@ static void recalculate_misc(struct cpu_policy *p)
>          zero_leaves(p->basic.raw, 0x2, 0x3);
>          memset(p->cache.raw, 0, sizeof(p->cache.raw));
>          zero_leaves(p->basic.raw, 0x9, 0xa);
> +        p->basic.raw[0x1c] = EMPTY_LEAF;

I'm not convinced this wants to live here. The leaf rather wants clearing
when the feature isn't there in the policy. See e.g. [1] and [2].

> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -505,6 +505,13 @@ static void generic_identify(struct cpuinfo_x86 *c)
>                           &c->x86_capability[FEATURESET_Da1],
>                           &tmp, &tmp, &tmp);
>  
> +     if (c->cpuid_level >= 0x1c)
> +             cpuid(0x1c,
> +                       &c->x86_capability[FEATURESET_1Ca],
> +                       &c->x86_capability[FEATURESET_1Cb],
> +                       &c->x86_capability[FEATURESET_1Cc],
> +                       &tmp);

Nit: Indentation (arguments want to align vertically).

> --- 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 
> */

'S' is too early. You first need to implement everything, and only then
you can make the feature visible to guests. It'll also only be by that
time that it'll be (halfway) clear whether 'S' is appropriate, or whether
for starters it wouldn't better be 's'. Same for the other markers below.

Jan