On 18.11.2024 09:49, ngoc-tu.d...@vates.tech wrote: > From: Tu Dinh <ngoc-tu.d...@vates.tech> > > Expose ARCH_LBR feature to guests. > > Extend CPU featureset with 3 words for CPUID leaf 0x1c. > > Signed-off-by: Tu Dinh <ngoc-tu.d...@vates.tech>
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
