[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f345b6d8-0065-4ca8-9526-543a601c3e90@intel.com>
Date: Fri, 1 Mar 2024 12:14:46 +1300
From: "Huang, Kai" <kai.huang@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: Paolo Bonzini <pbonzini@...hat.com>, <kvm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, Yan Zhao <yan.y.zhao@...el.com>, "Isaku
Yamahata" <isaku.yamahata@...el.com>, Michael Roth <michael.roth@....com>,
"Yu Zhang" <yu.c.zhang@...ux.intel.com>, Chao Peng
<chao.p.peng@...ux.intel.com>, Fuad Tabba <tabba@...gle.com>, David Matlack
<dmatlack@...gle.com>
Subject: Re: [PATCH 07/16] KVM: x86: Move synthetic PFERR_* sanity checks to
SVM's #NPF handler
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@...gle.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@...el.com>
Powered by blists - more mailing lists