Re: [PATCH] KVM: x86: Track supported ARCH_CAPABILITIES in kvm_caps

[Xiaoyao Li <xiaoyao.li@intel.com>]

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.