Received-SPF: pass (google.com: domain of linux-kernel+bounces-87632-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <d0d5d6b7-218b-4769-9aa7-a393f174410e@intel.com>
Date: Fri, 1 Mar 2024 11:19:37 +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>, Paolo Bonzini
	<pbonzini@redhat.com>
CC: <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>
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <20240228024147.41573-8-seanjc@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?NTdBL2dIVTY2RXRha3FTbUV5R3p2WXlaR0E4TGFJOTBLR2c5cjVMZE1DK2w3?=
 =?utf-8?B?ZXlBNEswNkNWZ2laSTVvRm9JVk9jT3FhZVppQzFRTWRLR2NqU3MzcXg1VU05?=
 =?utf-8?B?T3JINUJXSmpBUDlNT2JmSEYwUmZ3S281bWFxc0lkTktJVlA4RFpmWGs2ZjY5?=
 =?utf-8?B?M1lXL3lHdUJtaWJOZWRtWExhRXRvZmxaWmpLR2J2TStUOTFVY0tlNXkzL3lH?=
 =?utf-8?B?QUdVVWFwU3JScVM0cEJsSkYzZzNJbHp2bExUOE9VWnpyZ1h4YlY5RVYxK3dn?=
 =?utf-8?B?UWFpcTN1dnE3bVd1cXhQRG1ITk85VFVhbE1LRGwwc1FnRVBhMUowTCtReDJp?=
 =?utf-8?B?ZWZsR1FoN2UzaTQxVUtGS1FLRmJUSGhsbkg0NFozbHM5cGNuTTFqMFhORkxI?=
 =?utf-8?B?VHd0ZGVSeloxL0NjbHJnUG9EVTNNQUpkd2lsd3N2SGJZN1M2M29NWnpxaldn?=
 =?utf-8?B?TnFibGZiUDJKdEltcFNPQmNtVXQzc2NCa1VIajNlQjk3UFRkbmIvRUZhcXl3?=
 =?utf-8?B?U093bHJBdER5SVVOak1RdlVLa004N3dFenF5aEVTRCs3VXdhWUFTbnJxK0Fi?=
 =?utf-8?B?enpQT09xelZFMUprRXo2K2crUUwyWVl5WTRqZVAzUVh4c0NOdWMzU2x0bEFs?=
 =?utf-8?B?MVZITkFzSE94OEtuT0dVSXV5WnN1aWtUd3ZHRlNjeFZnN3p3UC9pR0daTHlD?=
 =?utf-8?B?MWQ4RXE5dTlXMHNkUGFWMDFTeXdvK0MvUTdsMHc1bGxhOE8wdEtpOGdwUzND?=
 =?utf-8?B?RjdSOE1mbEJISEtuUGVOaXFia1NqU0p1RUtxQml2a1l3TnowMStnSDU1MHk1?=
 =?utf-8?B?UGlDSVNkV3pVc2M4c0pPL01hZzRXTmtyTnZQdXprN1lxS0lyMDJ3eFJvRFps?=
 =?utf-8?B?MndJVVJuY29Pa1Y5UlBCSUlMd2lnNnNxTWRBTUxnaHppOXMxYzJuYVlTOVVn?=
 =?utf-8?B?Z0NVZHVLQTdpMTlZbFIvZ2tBRmpKb3FxNmp0dGtzVmYzNWI3cEI5QmhrMVFV?=
 =?utf-8?B?UElENExqSHpZeC8zN3JVRVpCWjkwdUxHNDQxa1RDR0wvN3B5YTJpNm5Gb1Ew?=
 =?utf-8?B?TjdDS1VxZmJnZlA0TkdHcHJ5K0VlNUhWc3o2cWVmaUx3d2pJaVM0THNoZVVs?=
 =?utf-8?B?ZDdqaDZ1UUYwTFpmb1Yzc1JyN09ZSm54cTJzcWVFMGZsQkU0b3UwbFZwZlNs?=
 =?utf-8?B?Rk5CRFdSby9QMFRkaWYyRGQvQUpGTlF0RlI5UWk3Tjg4UytiRVVhREMyV3Ju?=
 =?utf-8?B?T1R6MmxuczZkS1BNaWU3WVhPVEdTWUZzc0xnekZwR0xGRDRHUCtzQ1lEMGdL?=
 =?utf-8?B?anlFdUU4UkNVTEIySzlndEQxY0JwZ0JxNlZqVmJaem85QnNXd0t5UWllSnp4?=
 =?utf-8?B?M0pNei83WXE2OUY2YUx4Rk5rWTBiSC9pRi9DTVIraEVCcENqT2dCaFI0ZFpV?=
 =?utf-8?B?NGlWVU56RVlKRXp4V3J6QkRKTzFNNTQ1VmF2S1hlRGhsWXMzYUNGRjVRYngr?=
 =?utf-8?B?a0tONndwNFNOVGVUTE9rbDBYUTZzcDVHdEl0V3hlRTRscWhETlFiNVdGZldL?=
 =?utf-8?B?NkpGUUVKOTJHT2Z3SVVjTU5iczUzNHNkQVFsekcxSFNMMnF2L3BveXd1SWZ3?=
 =?utf-8?B?QXhJWU5KbExrRTdtUHVaeEVvMHUrK3VQTTZVZ2NSdTlyNDYrK1RCYnA3STlG?=
 =?utf-8?B?UDl1eGpGZHpSUkhYZXJLV01FZXkyVEVrakl5ekE3TjlpbmMySFlkRVZ5aDNN?=
 =?utf-8?B?ZStGWUUzSlpycDBHdGM1UWNDL3l1V3VlRGc1YUIwblRWazRxNm1mWkxXbXRP?=
 =?utf-8?B?OW1DZU90dzJUa1FWSnBQSWZrakRrSU9rUDVtWW43ZFFNUFFOZjBRUnNQY2pn?=
 =?utf-8?B?MjE5aVl6cGplajZuK2JscitJNjN5WVU1bU4wM3VCQVpNY2pqRXQ2ODJ5dzAw?=
 =?utf-8?B?eXcxM1pjV1JiM1hiMEdOQXJkVFEra29GT2VNRVdKUElWNmYrNEF0S2hQb3ZN?=
 =?utf-8?B?UlVlWnRUSjdTdWJXbWxxMTEwT3VJK2hiSUIyL2VtM0lEU2g5UjJ1ZFlEM0Ju?=
 =?utf-8?B?SHNOTmdRR1VtcXNuZGVoTGNZVUd2dE52cytva3h2Wmd3NWpCd1ZJY2R5blMz?=
 =?utf-8?Q?/r3El0R3uEA7v1ZfXSYOkJ0xh?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7a4624cc-a501-423d-c4c4-08dc39748926
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Feb 2024 22:19:46.1329
 (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: 15RVhTM12XmAiMBP9e2e+afD8Ccol6peS5t00jP1MPAt9NQEvgYddFMT6cdD6yGGNjN2omI+ye9KrhTdvwDw4w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7317
X-OriginatorOrg: intel.com


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 some common place like in 
kvm_mmu_page_fault()?

Otherwise,

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