Received-SPF: pass (google.com: domain of linux-kernel+bounces-87667-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <f345b6d8-0065-4ca8-9526-543a601c3e90@intel.com>
Date: Fri, 1 Mar 2024 12:14:46 +1300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 07/16] KVM: x86: Move synthetic PFERR_* sanity checks to
 SVM's #NPF handler
Content-Language: en-US
To: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, Yan Zhao <yan.y.zhao@intel.com>, "Isaku
 Yamahata" <isaku.yamahata@intel.com>, Michael Roth <michael.roth@amd.com>,
	"Yu Zhang" <yu.c.zhang@linux.intel.com>, Chao Peng
	<chao.p.peng@linux.intel.com>, Fuad Tabba <tabba@google.com>, David Matlack
	<dmatlack@google.com>
References: <20240228024147.41573-1-seanjc@google.com>
 <20240228024147.41573-8-seanjc@google.com>
 <d0d5d6b7-218b-4769-9aa7-a393f174410e@intel.com>
 <ZeEKq7L8oOqqfknb@google.com>
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <ZeEKq7L8oOqqfknb@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YVBjZDRCaEN5NW9sUzZ5Z2hENFo1UjlCb0pQdVJweGl6empkMVdwWVdaODRw?=
 =?utf-8?B?bityZ1JOLzBTeWhNa1R1SWJLbGlldExpSW94ZlhMbWVzOTNZdVY5YTZrQ1l1?=
 =?utf-8?B?d3l6SXZIS0NSczZWd05ZSWtnbVlvYlZHYmozMU4xcjAvVmFxVjZGZFVZUG80?=
 =?utf-8?B?SVlYQ0VQOUMxeCsvNVNMc3VOUlBQVzlvRDBrK1oxK3BMZWdmVHZxNHAzQ0lj?=
 =?utf-8?B?RTkwR0twTno1S3BQb2E3dWhSU0diREVHVStzbU1NT0tuQkdCQ2l5YjRiQWEv?=
 =?utf-8?B?UFBXTzJrYzFlMnRsWlRTRXZ4eVNHZTlVbEdwWVZheUhCZ3E3RnovREpPdzZC?=
 =?utf-8?B?M2pnL1R1dmo2d0hWVGhIWThmdkcwVGdHZ0o0ZldTN1EvN3IvRDRzYVZkMCs5?=
 =?utf-8?B?WFEyMTF5ekZ4eVkxYW1LdjlJSzdUQkUzNkI0Y3RkaWdMeXV3TXF1NnMzN3FR?=
 =?utf-8?B?MUpVVlA3eHRGcmRCQkxFcGpXNUNtOXh6Y2l0TzVRbDdYK3orYzNUR1BkOEdB?=
 =?utf-8?B?NnA1RWZWWUhDUTJtY3JGQWRxV3JSSHVOdm55ZExJbEFoSmZJaldHcE1NRnNw?=
 =?utf-8?B?S05kcDgyRXBVTWtoYmo5QjBWNjlkTmcrdlNRVTg4NVdDUU9IR2RnWldxWmNL?=
 =?utf-8?B?QktzQ1ZLRWFsb2tHaE91K3Z3WFR1bDhiellmYkxTeTltK0pMNnpjRVFhMGtZ?=
 =?utf-8?B?ZEtEbjJSZ0dwais0S1ZlZE8yUkZLTkl1M0VZVmpBWDBCWFdnRnB3Z29GOEdP?=
 =?utf-8?B?SVFvclN6UTIwdXl2UkpkY2hsbUcvMU5HdmkzM1B6cHdwblo2Z29LLy9hbkRC?=
 =?utf-8?B?RkNVd3p0ZUdvSWpSdXpQUENZZ3FhU2pmU1ZUTE1BU1loRlBVYk1RSG9qcE9S?=
 =?utf-8?B?SmtEajBnM2tRdmhBbUxhK0o2K3psTnQ2YUF0Lzh2ZjNudGZpM1lEQ2V3Slkv?=
 =?utf-8?B?Vms3bnBVS2s3MWF2R2tWSThLL0IzTjdrSU1zaW9Lb1E5djZiRGg4QXBBZUJs?=
 =?utf-8?B?VFVydDJ0OVpCTzRZa0NuT3JNM0FsejA3U0ZZdVVZbHcvSnJHOFpPdEk2SWd1?=
 =?utf-8?B?OVY0ZXNjMXVQdWkxYzlxZ1crUkFJd3ZZZWdHWEV3NjVEVVQyMGZSajFYZGdm?=
 =?utf-8?B?V0gwa0owMHV2aU9KL3NORmhmbUpLdXhZNWhrRWxVeGhLRXlyejl5ZGJYbHgv?=
 =?utf-8?B?QzNQaWxPS1VNbE1Jd0tNZTcrVmsrQlNmeVgwTlpnVStQem1PdGxLd0VmekxC?=
 =?utf-8?B?S1l4Q3BQN3JZdU1ZTkM2cDBYR3M2K0VtT1gvUktjUTRpcm5ROTl2ajJqZEpu?=
 =?utf-8?B?eU53cDFDdXhUcTZ6c1ZxTDNtdHJnK1ZFOU5QZVNORHR0dGlGbzl0Y2tyM28w?=
 =?utf-8?B?dlc2bmhYQmUrTnQ3a0daQjNtZHhKRzBrRDcyVFY2ZVhvbmZvYkJhZEIwVHZp?=
 =?utf-8?B?Y2I3dnhvTkU1RnljaVFKSEZISTRPSmhwSG1xL1ZPWUhCcjB2OUQ0cmFnd2E4?=
 =?utf-8?B?NzVaeG55K3IwekNSMTJ6V0lRUDNOQTk5REE2SmE1bjAwbXdLNXkrNFpLcDNQ?=
 =?utf-8?B?cHMxaytwUnpzN0RzTDYzRXJQQXdYSEZhR1NSVHVlSHcreVlkQlQzYVN2Mlhw?=
 =?utf-8?B?N1dPTkpXdnhZWURXZHhWditlU2lpcElVamQxTUdIM1BlWnFMclgrN05ZLzdS?=
 =?utf-8?B?RFVUZXBCTDRsZ1FyZEtLN2ZkekZ5clRYMk1XdHFDZ0UraHc4dWxRbWNuUW5z?=
 =?utf-8?B?YnA0WFlZVzhhRWgwS3NHSmVMUjZPNEl5QjUxc2M0WkZmMnE1WjBqMFdJTC9t?=
 =?utf-8?B?VUJmSWxnQ3FVK2xBcG9xMnB3RXhLZHFVYXNuQTBOb0UyVmpoeEpSTnVRR3VX?=
 =?utf-8?B?ejg5SGFHTDBmRWtWOFNOR3Vkb1pvUDI5QnF4VXBKRURlL29XMC9kUkhzcTlN?=
 =?utf-8?B?OCtDZUZ6MzRBSVVIK3pWdkU1T2Z3OXNpRHhMVGM5TXF0ZUw0aDlIeHRVa2Z1?=
 =?utf-8?B?eHZpUHE1Z3doTHR5bFRGeFFGUDFEbTkvdVE4ckhGMjdJOEM0M0Q5akpkVlRN?=
 =?utf-8?B?WWRIcWhGSi9sUUlra203MkduaUEwbys1VEUyQlB5Z2ZDZWZubDJEa2F2Vmc2?=
 =?utf-8?Q?7SSSL9hVrwjwAowgsWpUlCi+e?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6c4c1952-d56f-467a-d125-08dc397c3de3
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Feb 2024 23:14:55.8511
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: cGoOHpGOIjxOTfik2462Q1kmWAc5qyxcyvPNvYTgWUAITbo9McqdjuRL6jbCjefsysO4bo2MC3HLIiNlOc/j7A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5072
X-OriginatorOrg: intel.com


On 1/03/2024 11:52 am, Sean Christopherson wrote:
> On Fri, Mar 01, 2024, Kai Huang wrote:
>>
>>
>> On 28/02/2024 3:41 pm, Sean Christopherson wrote:
>>> Move the sanity check that hardware never sets bits that collide with KVM-
>>> define synthetic bits from kvm_mmu_page_fault() to npf_interception(),
>>> i.e. make the sanity check #NPF specific.  The legacy #PF path already
>>> WARNs if _any_ of bits 63:32 are set, and the error code that comes from
>>> VMX's EPT Violatation and Misconfig is 100% synthesized (KVM morphs VMX's
>>> EXIT_QUALIFICATION into error code flags).
>>>
>>> Add a compile-time assert in the legacy #PF handler to make sure that KVM-
>>> define flags are covered by its existing sanity check on the upper bits.
>>>
>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>> ---
>>>    arch/x86/kvm/mmu/mmu.c | 12 +++---------
>>>    arch/x86/kvm/svm/svm.c |  9 +++++++++
>>>    2 files changed, 12 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
>>> index 5d892bd59c97..bd342ebd0809 100644
>>> --- a/arch/x86/kvm/mmu/mmu.c
>>> +++ b/arch/x86/kvm/mmu/mmu.c
>>> @@ -4561,6 +4561,9 @@ int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code,
>>>    	if (WARN_ON_ONCE(error_code >> 32))
>>>    		error_code = lower_32_bits(error_code);
>>> +	/* Ensure the above sanity check also covers KVM-defined flags. */
>>> +	BUILD_BUG_ON(lower_32_bits(PFERR_SYNTHETIC_MASK));
>>> +
>>
>> Could you explain why adding this BUILD_BUG_ON() here, but not ...
>>
>>>    	vcpu->arch.l1tf_flush_l1d = true;
>>>    	if (!flags) {
>>>    		trace_kvm_page_fault(vcpu, fault_address, error_code);
>>> @@ -5845,15 +5848,6 @@ int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
>>>    	int r, emulation_type = EMULTYPE_PF;
>>>    	bool direct = vcpu->arch.mmu->root_role.direct;
>>> -	/*
>>> -	 * WARN if hardware generates a fault with an error code that collides
>>> -	 * with KVM-defined sythentic flags.  Clear the flags and continue on,
>>> -	 * i.e. don't terminate the VM, as KVM can't possibly be relying on a
>>> -	 * flag that KVM doesn't know about.
>>> -	 */
>>> -	if (WARN_ON_ONCE(error_code & PFERR_SYNTHETIC_MASK))
>>> -		error_code &= ~PFERR_SYNTHETIC_MASK;
>>> -
>>>    	if (WARN_ON_ONCE(!VALID_PAGE(vcpu->arch.mmu->root.hpa)))
>>>    		return RET_PF_RETRY;
>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>> index e90b429c84f1..199c4dd8d214 100644
>>> --- a/arch/x86/kvm/svm/svm.c
>>> +++ b/arch/x86/kvm/svm/svm.c
>>> @@ -2055,6 +2055,15 @@ static int npf_interception(struct kvm_vcpu *vcpu)
>>>    	u64 fault_address = svm->vmcb->control.exit_info_2;
>>>    	u64 error_code = svm->vmcb->control.exit_info_1;
>>> +	/*
>>> +	 * WARN if hardware generates a fault with an error code that collides
>>> +	 * with KVM-defined sythentic flags.  Clear the flags and continue on,
>>> +	 * i.e. don't terminate the VM, as KVM can't possibly be relying on a
>>> +	 * flag that KVM doesn't know about.
>>> +	 */
>>> +	if (WARN_ON_ONCE(error_code & PFERR_SYNTHETIC_MASK))
>>> +		error_code &= ~PFERR_SYNTHETIC_MASK;
>>> +
>>>    	trace_kvm_page_fault(vcpu, fault_address, error_code);
>>>    	return kvm_mmu_page_fault(vcpu, fault_address, error_code,
>>>    			static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
>>
>> ...  in npf_interception() or
> 
> The intent of the BUILD_BUG_ON() is to ensure that kvm_handle_page_fault()'s
> sanity check that bits 63:32 also serves as a sanity check that hardware doesn't
> generate an error code that collides with any of KVM's synthetic flags.
> 
> E.g. if we were to add a KVM-defined flag in the lower 32 bits, then the #NPF
> path would Just Work, because it already sanity checks all synthetic bits.  But
> the #PF path would need new code, thus the BUILD_BUG_ON() to scream that new code
> is needed.

Ah, right.  Thanks for explaining :-)

Reviewed-by: Kai Huang <kai.huang@intel.com>