RE: [PATCH v1 01/11] xen/x86: add CPPC feature flag for AMD processors

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi,

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Sent: Tuesday, December 3, 2024 4:32 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Stabellini, Stefano <stefano.stabellini@xxxxxxx>; Huang, Ray
> <Ray.Huang@xxxxxxx>; Ragiadakou, Xenia <Xenia.Ragiadakou@xxxxxxx>;
> Andryuk, Jason <Jason.Andryuk@xxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>;
> Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Subject: Re: [PATCH v1 01/11] xen/x86: add CPPC feature flag for AMD
> processors
>
> On 03/12/2024 8:11 am, Penny Zheng wrote:
> > Add Collaborative Processor Performance Control feature flag for AMD
> > processors.
> >
> > amd-pstate is the AMD CPU performance scaling driver that introduces a
> > new CPU frequency control mechanism on modern AMD APU and CPU series.
> > There are two types of hardware implementations: "Full MSR Support"
> > and "Shared Memory Support".
> >
> > Right now, xen will only implement "Full MSR Support", and this new
> > feature flag indicates whether processor has this feature or not.
>
> Do you have a reference to where this is documented?
>

Yes, 
https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24594.pdf
See page 628 for CPUID Fn8000_0008_EBX Extended Feature Identifiers

> >
> > Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> > ---
> >  xen/arch/x86/include/asm/cpufeature.h       | 1 +
> >  xen/include/public/arch-x86/cpufeatureset.h | 1 +
> >  2 files changed, 2 insertions(+)
> >
> > diff --git a/xen/arch/x86/include/asm/cpufeature.h
> > b/xen/arch/x86/include/asm/cpufeature.h
> > index 3a06b6f297..6935703e71 100644
> > --- a/xen/arch/x86/include/asm/cpufeature.h
> > +++ b/xen/arch/x86/include/asm/cpufeature.h
> > @@ -170,6 +170,7 @@ static inline bool boot_cpu_has(unsigned int feat)
> >  #define cpu_has_amd_ssbd        boot_cpu_has(X86_FEATURE_AMD_SSBD)
> >  #define cpu_has_virt_ssbd       boot_cpu_has(X86_FEATURE_VIRT_SSBD)
> >  #define cpu_has_ssb_no          boot_cpu_has(X86_FEATURE_SSB_NO)
> > +#define cpu_has_cppc            boot_cpu_has(X86_FEATURE_CPPC)
> >  #define cpu_has_auto_ibrs       boot_cpu_has(X86_FEATURE_AUTO_IBRS)
> >
> >  /* CPUID level 0x00000007:0.edx */
> > diff --git a/xen/include/public/arch-x86/cpufeatureset.h
> > b/xen/include/public/arch-x86/cpufeatureset.h
> > index 8fa3fb711a..15f707639b 100644
> > --- a/xen/include/public/arch-x86/cpufeatureset.h
> > +++ b/xen/include/public/arch-x86/cpufeatureset.h
> > @@ -265,6 +265,7 @@ XEN_CPUFEATURE(AMD_PPIN,      8*32+23) /*
> Protected Processor Inventory Number
> >  XEN_CPUFEATURE(AMD_SSBD,      8*32+24) /*S  MSR_SPEC_CTRL.SSBD
> available */
> >  XEN_CPUFEATURE(VIRT_SSBD,     8*32+25) /*!
> MSR_VIRT_SPEC_CTRL.SSBD */
> >  XEN_CPUFEATURE(SSB_NO,        8*32+26) /*A  Hardware not vulnerable to
> SSB */
> > +XEN_CPUFEATURE(CPPC,          8*32+27) /*A  Collaborative Processor
> Performance Control */
>
> There needs to be no "A" in this patch, because it's certainly not ready to 
> advertise
> to guests yet.
>
> If you're intending it for eventual use by guests, then the "A" only goes 
> back in at the
> end of the series when the support to correctly virtualise it appears, but it 
> looks like
> it will be Xen-only.
>

Understood! Thx for the explanation. I'll remove in next Serie

> ~Andrew

Many thanks,
Penny