Re: [PATCH 2/2] target/i386: Add missed features to Cooperlake CPU model

From: Eduardo Habkost <ehabkost@redhat.com>
To: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: qemu-devel@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>,
	Cathy Zhang <cathy.zhang@intel.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH 2/2] target/i386: Add missed features to Cooperlake CPU model
Date: Tue, 25 Aug 2020 10:01:45 -0400	[thread overview]
Message-ID: <20200825140145.GU642093@habkost.net> (raw)
In-Reply-To: <9cdaaa8d-4ce2-88c7-2400-eed62ffe779b@intel.com>

On Tue, Aug 25, 2020 at 08:20:35AM +0800, Xiaoyao Li wrote:
> On 8/25/2020 6:07 AM, Eduardo Habkost wrote:
> > On Wed, Dec 25, 2019 at 02:30:18PM +0800, Xiaoyao Li wrote:
> > > It lacks VMX features and two security feature bits (disclosed recently) in
> > > MSR_IA32_ARCH_CAPABILITIES in current Cooperlake CPU model, so add them.
> > > 
> > > Fixes: 22a866b6166d ("i386: Add new CPU model Cooperlake")
> > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > > ---
> > >   target/i386/cpu.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 50 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index e1eb9f473989..c9798ac8652b 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -3198,7 +3198,8 @@ static X86CPUDefinition builtin_x86_defs[] = {
> > >               CPUID_7_0_EDX_SPEC_CTRL_SSBD | CPUID_7_0_EDX_ARCH_CAPABILITIES,
> > >           .features[FEAT_ARCH_CAPABILITIES] =
> > >               MSR_ARCH_CAP_RDCL_NO | MSR_ARCH_CAP_IBRS_ALL |
> > > -            MSR_ARCH_CAP_SKIP_L1DFL_VMENTRY | MSR_ARCH_CAP_MDS_NO,
> > > +            MSR_ARCH_CAP_SKIP_L1DFL_VMENTRY | MSR_ARCH_CAP_MDS_NO |
> > > +            MSR_ARCH_CAP_PSCHANGE_MC_NO | MSR_ARCH_CAP_TAA_NO,
> > 
> > This seems to break on some Cooperlake hosts, see:
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1860743
> > 
> > Are all Cooperlake hosts supposed to have TAA_NO set?  Are there
> > hosts where this requires a microcode update to be installed?
> > 
> 
> All the production CPX in market should have IAA_NO bit. We can check it
> directly with rdmsr(0x10a).
> 
> The problem of this issue is due to commit db616173d787 ("x86/tsx: Add
> config options to set tsx=on|off|auto"), which sets the default to "off" for
> 100% safety. However, default to off may cause noticeable regressions on TSX
> safe platform, e.g., CPX.
> 
> Maybe we need to set CONFIG_X86_INTEL_TSX_MODE_AUTO=y for OSV released
> kernel?

Considering that disabling TSX is a policy decision likely to be
taken by the OS vendor or by the system administrator, we could
at least make the CPU model easier to use on those cases.

Maybe we should provide a version of Cooperlake without TSX, like
we already do for the other CPU models?

-- 
Eduardo