Message-ID: <20230523033405.dr2ci7h3ol5se64o@desk>
Date:   Mon, 22 May 2023 20:34:05 -0700
From:   Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
To:     Xiaoyao Li <xiaoyao.li@...el.com>
Cc:     Sean Christopherson <seanjc@...gle.com>,
        Chao Gao <chao.gao@...el.com>, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org,
        Jim Mattson <jmattson@...gle.com>,
        antonio.gomez.iglesias@...ux.intel.com,
        Daniel Sneddon <daniel.sneddon@...ux.intel.com>
Subject: Re: [PATCH] KVM: x86: Track supported ARCH_CAPABILITIES in kvm_caps

On Tue, May 23, 2023 at 09:00:50AM +0800, Xiaoyao Li wrote:
> On 5/23/2023 5:23 AM, Pawan Gupta wrote:
> > On Tue, May 23, 2023 at 03:31:44AM +0800, Xiaoyao Li wrote:
> > > On 5/23/2023 1:43 AM, Sean Christopherson wrote:
> > > > > >     6. Performance aside, KVM should not be speculating (ha!) on what the guest
> > > > > >        will and will not do, and should instead honor whatever behavior is presented
> > > > > >        to the guest.  If the guest CPU model indicates that VERW flushes buffers,
> > > > > >        then KVM damn well needs to let VERW flush buffers.
> > > > > The current implementation allows guests to have VERW flush buffers when
> > > > > they enumerate FB_CLEAR. It only restricts the flush behavior when the
> > > > > guest is trying to mitigate against a vulnerability(like MDS) on a
> > > > > hardware that is not affected. I guess its common for guests to be
> > > > > running with older gen configuration on a newer hardware.
> > > > Right, I'm saying that that behavior is wrong.  KVM shouldn't assume the guest
> > > > the guest will do things a certain way and should instead honor the "architectural"
> > > > definition, in quotes because I realize there probably is no architectural
> > > > definition for any of this.
> > > > 
> > > > It might be that the code does (unintentionally?) honor the "architecture", i.e.
> > > > this code might actually be accurrate with respect to when the guest can expect
> > > > VERW to flush buffers.  But the comment is so, so wrong.
> > > 
> > > The comment is wrong and the code is wrong in some case as well.
> > > 
> > > If none of ARCH_CAP_FB_CLEAR, ARCH_CAP_MDS_NO, ARCH_CAP_TAA_NO,
> > > ARCH_CAP_PSDP_NO, ARCH_CAP_FBSDP_NO and ARCH_CAP_SBDR_SSDP_NO are exposed to
> > > VM, the VM is type of "affected by MDS".
> > > 
> > > And accroding to the page https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/processor-mmio-stale-data-vulnerabilities.html
> > > 
> > > if the VM enumerates support for both L1D_FLUSH and MD_CLEAR, it implicitly
> > > enumerates FB_CLEAR as part of their MD_CLEAR support.
> > 
> > This is the excerpt from the link that you mentioned:
> > 
> >    "For processors that are affected by MDS and support L1D_FLUSH
> >    operations and MD_CLEAR operations, the VERW instruction flushes fill
> >    buffers."
> > 
> > You are missing an important information here "For the processors
> > _affected_ by MDS". On such processors ...
> > 
> > > However, the code will leave vmx->disable_fb_clear as 1 if hardware supports
> > > it, and VERW intruction doesn't clear FB in the VM, which conflicts
> > > "architectural" definition.
> > 
> > ... Fill buffer clear is not enabled at all:
> > 
> >    vmx_setup_fb_clear_ctrl()
> >    {
> >    	u64 msr;
> >    	if (boot_cpu_has(X86_FEATURE_ARCH_CAPABILITIES) &&
> >    	    !boot_cpu_has_bug(X86_BUG_MDS) &&
> >    	    !boot_cpu_has_bug(X86_BUG_TAA)) {
> >    		rdmsrl(MSR_IA32_ARCH_CAPABILITIES, msr);
> >    		if (msr & ARCH_CAP_FB_CLEAR_CTRL)
> >    			vmx_fb_clear_ctrl_available = true;
> >    	}
> >    }
> 
> This is the check of bare metal, while the check in
> vmx_update_fb_clear_dis() is of guest VM.
> 
> For example, if the hardware (host) enumerates ARCH_CAP_TAA_NO,
> ARCH_CAP_MDS_NO, ARCH_CAP_PSDP_NO, ARCH_CAP_FBSDP_NO, ARCH_CAP_SBDR_SSDP_NO,
> ARCH_CAP_FB_CLEAR, and ARCH_CAP_FB_CLEAR_CTRL, the VERW on this hardware
> clears Fill Buffer (if FB_CLEAR_DIS is not enabled in
> MSR_IA32_MCU_OPT_CTRL). vmx_setup_fb_clear_ctrl() does set
> vmx_fb_clear_ctrl_available  to true.
> 
> If a guest is exposed without ARCH_CAP_TAA_NO, ARCH_CAP_MDS_NO,
> ARCH_CAP_PSDP_NO, ARCH_CAP_FBSDP_NO, ARCH_CAP_SBDR_SSDP_NO and
> ARCH_CAP_FB_CLEAR, vmx_update_fb_clear_dis() will leave
> vmx->disable_fb_clear as true. So VERW doesn't clear Fill Buffer for guest.
> But in the view of guset, it expects VERW to clear Fill Buffer.

That is correct, but whether VERW clears the CPU buffers also depends on
if the hardware is affected or not, enumerating MD_CLEAR solely does not
guarantee that VERW will flush CPU buffers. This was true even before
MMIO Stale Data was discovered.

If host(hardware) enumerates:

	MD_CLEAR | MDS_NO |   VERW behavior
	---------|--------|-------------------
	    1	 |    0	  | Clears CPU buffers

But on an MDS mitigated hardware(MDS_NO=1) if guest enumerates:

	MD_CLEAR | MDS_NO |   VERW behavior
	---------|--------|-----------------------
	    1	 |    0	  | Not guaranteed to clear
	                        CPU buffers

After MMIO Stale Data, FB_CLEAR_DIS was introduced to keep this behavior
intact(for hardware that is not affected by MDS/TAA). If the userspace
truly wants the guest to have VERW flush behavior, it can export
FB_CLEAR.

I see your point that from a guest's perspective it is being lied about
VERW behavior. OTOH, I am not sure if it is a good enough reason for
mitigated hardware to keep the overhead of clearing micro-architectural
buffers for generations of CPUs.

Powered by blists - more mailing lists