Re: [PATCH] i386: pass CLZERO to guests with EPYC CPU model on AMD ZEN platform

[Eduardo Habkost]

Hi,

Sorry for the delayed reply.  I was away from work for the whole
month of January.

On Mon, Jan 20, 2020 at 10:56:43AM +0000, Ani Sinha wrote:
> Sorry Eduardo, it took a little while for me to get to this thread again.
> 
> > On Dec 18, 2019, at 8:41 PM, Eduardo Habkost <address@hidden> wrote:
> > 
> > On Wed, Dec 18, 2019 at 12:53:45PM +0100, Paolo Bonzini wrote:
> >> On 18/12/19 10:05, Ani Sinha wrote:
> >>> CLZERO CPUID should be passed on to the guests that use EPYC or EPYC-IBPB 
> >>> CPU
> >>> model when the AMD ZEN based host supports it. This change makes it 
> >>> recognize
> >>> this CPUID for guests which use EPYC or EPYC-IBPB CPU model.
> > 
> > Can you clarify what's the intended use case here?  Why the
> > "if host supports it" conditional?
> 
> Looking at https://www.amd.com/system/files/TechDocs/24594.pdf , it says :
> 
> "The CLZERO instruction is supported if the feature flag CPUID 
> Fn8000_0008_EBX[CLZERO] is set.”
> 
> This I interpreted to mean that not all AMD Zen architectures
> supports it. So when the host does support it, this CPUID
> should be passed on to the guest as well.

This is not a supported use case of named CPU models.  Named CPU
models should expose the same guest ABI on all hosts.  This means
CPUID should be the same on all hosts if using the same CPU
model (and same machine type).

If you need features to be automatically enabled/disabled
depending on host capabilities, I advise you to use "-cpu host"
or libvirt's mode=host-model.

> 
> 
> > 
> > If you need host-dependent CPU configuration, "-cpu host" (or the
> > libvirt "host-model" mode) is the most appropriate solution.
> 
> Yes that is an option but we are going to use EPYC-IBPB model for now. 
> 
> 
> > 
> >>> 
> >>> Signed-off-by: Ani Sinha <address@hidden>
> >>> ---
> >>> target/i386/cpu.c | 2 ++
> >>> 1 file changed, 2 insertions(+)
> >>> 
> >>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> >>> index 69f518a..55f0691 100644
> >>> --- a/target/i386/cpu.c
> >>> +++ b/target/i386/cpu.c
> >>> @@ -3813,6 +3813,8 @@ static X86CPUDefinition builtin_x86_defs[] = {
> >>>             CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A | CPUID_EXT3_ABM |
> >>>             CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM |
> >>>             CPUID_EXT3_TOPOEXT,
> >>> +        .features[FEAT_8000_0008_EBX] =
> >>> +            CPUID_8000_0008_EBX_CLZERO,
> >>>         .features[FEAT_7_0_EBX] =
> >>>             CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | 
> >>> CPUID_7_0_EBX_AVX2 |
> >>>             CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 | 
> >>> CPUID_7_0_EBX_RDSEED |
> >>> 
> >> 
> >> This needs to be done only for newer machine type (or is it CPU model
> >> versions now? need Eduardo to respond).
> > 
> > If we want to add it, it has to be done as a new CPU model version.
> 
> I see what you mean. 
> 
> > 
> > But I don't know yet if we want to add it.  Do all EPYC CPUs have
> > CLZERO available?  If not, it's probably not advisable to add it
> > to EPYC (even if it's just on EPYC-v3).
> 
> Ok so I think we need to get this clarified from AMD if all
> their EPYC platforms supports this CPUID or not. Is there any
> contact point within AMD where we can get this information?

I'm CCing Brijesh Singh and Babu Moger, who works on the EPYC CPU
model recently.

> 
> For our use case, I just verified that even without this patch,
> if we pass CLZERO through libvirt CPU definition xml, like "
> <feature policy='require' name='clzero’/>”, it gets passed on
> to the guest. So this patch is not super critical for us.

Note that this is done automatically by libvirt if using
mode=host-model.

-- 
Eduardo