Message-ID: <20230520010237.3tepk3q44j52leuk@desk>
Date:   Fri, 19 May 2023 18:02:37 -0700
From:   Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Xiaoyao Li <xiaoyao.li@...el.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>
Subject: Re: [PATCH] KVM: x86: Track supported ARCH_CAPABILITIES in kvm_caps

On Thu, May 18, 2023 at 10:33:15AM -0700, Sean Christopherson wrote:
> +Jim
> 
> On Thu, May 18, 2023, Xiaoyao Li wrote:
> > On 5/6/2023 11:04 AM, Chao Gao wrote:
> > > to avoid computing the supported value at runtime every time.
> > > 
> > > No functional change intended.
> > 
> > the value of kvm_get_arch_capabilities() can be changed due to
> > 
> > 	if (l1tf_vmx_mitigation != VMENTER_L1D_FLUSH_NEVER)
> > 		data |= ARCH_CAP_SKIP_VMENTRY_L1DFLUSH;
> > 
> > and l1tf_vmx_mitigation can be runtime changed by vmentry_l1d_flush module
> > param.
> 
> Nice catch!
> 
> > We need a detailed analysis that in no real case can
> > ARCH_CAP_SKIP_VMENTRY_L1DFLUSH bit change runtime.
> 
> No, the fact that it _can_ be modified by a writable module param is enough to
> make this patch buggy.
> 
> I do like snapshotting and then updating the value, even though there's likely no
> meaningful performance benefit, as that would provide a place to document that
> the "supported" value is dynamic.  Though the fact that it's dynamic is arguably a bug
> in its own right, e.g. if userspace isn't careful, a VM can have vCPUs with different
> values for ARCH_CAPABILITIES.  But fixing that is probably a fool's errand.  So
> I vote to snapshot the value and toggle the ARCH_CAP_SKIP_VMENTRY_L1DFLUSH bit
> when l1tf_vmx_mitigation is modified.
> 
> On a somewhat related topic, what in the absolute #$#$ is going on with FB_CLEAR_DIS!?!?
> I made the mistake of digging into why KVM doesn't advertise ARCH_CAP_FB_CLEAR_CTRL...
> 
>   1. I see *nothing* in commit 027bbb884be0 ("KVM: x86/speculation: Disable Fill
>      buffer clear within guests") that justifies 1x RDMSR and 2x WRMSR on every
>      entry+exit.

Unnecessary VERWs in guest will have much higher impact than due to MSR
read/write at vmentry/exit. On an Icelake system it is pointless for a
guest to incur VERW penalty when the system is not affected by MDS/TAA and
guests don't need mitigation for MMIO Stale Data. MSR writes are only
done when the guest is likely to execute unnecessary VERWs(e.g. when the
guest thinks its running on an older gen CPU).

>   2. I'm pretty sure conditioning mmio_stale_data_clear on kvm_arch_has_assigned_device()
>      is a bug.  AIUI, the vulnerability applies to _any_ MMIO accesses.

Vulnerability applies to MMIO access to devices that don't respect "byte
enable" (which indicates valid bytes in a transaction), and don't error
on incorrect read or write size.

>      Assigning a device is necessary to let the device DMA into the
>      guest, but it's not necessary to let the guest access MMIO
>      addresses, that's done purely via memslots.

I will get back on this. The guest would typically need access to an
area that doesn't fail an incorrectly sized MMIO.

>   3. Irrespective of whether or not there is a performance benefit, toggling the
>      MSR on every entry+exit is completely unnecessary if KVM won't do VERW before
>      VM-Enter, i.e. if (!mds_user_clear && !mmio_stale_data_clear), then the
>      toggling can be done in vmx_prepare_switch_to_{guest,host}().  This probably
>      isn't worth pursuing though, as #4 below is more likely, especially since
>      X86_BUG_MSBDS_ONLY is limited to Atom (and MIC, lol) CPUs.
>
>   4. If the host will will _never_ do VERW, i.e. #3 + !X86_BUG_MSBDS_ONLY, then

Is it likely that KVM will not do VERW when affected by MMIO Stale Data?
If you mean on a hardware that is not vulnerable to MDS and MMIO Stale
Data, in that case MSR writes are unnecessary and will be skipped
because FB_CLEAR_DIS is not available on unaffected hardware.

>      KVM just needs to context switch the MSR between guests since the value that's
>      loaded while running in the host is irrelevant.  E.g. use a percpu cache to

I will be happy to avoid the MSR read/write, but its worth considering
that this MSR can receive more bits that host may want to toggle, then
percpu cache implementation would likely change.

>      track the current value.
> 
>   5. MSR_IA32_MCU_OPT_CTRL is not modified by the host after a CPU is brought up,
>      i.e. the host's desired value is effectively static post-boot, and barring
>      a buggy configuration (running KVM as a guest), the boot CPU's value will be
>      the same as every other CPU.

Would the MSR value be same on every CPU, if only some guests have
enumerated FB_CLEAR and others haven't? MSR writes (to disable FB_CLEAR)
are not done when a guest enumerates FB_CLEAR. Enumeration of FB_CLEAR
in guest will depend on its configuration.

>   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.

>   7. Why on earth did Intel provide a knob that affects both the host and guest,
>      since AFAICT the intent of the MSR is purely to suppress FB clearing for an
>      unsuspecting (or misconfigured?) guest!?!?!

Thats true.

Powered by blists - more mailing lists