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@xxxxxxxxxx>
> > ---
> > 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.
> some common place like in kvm_mmu_page_fault()?
Because again, the logic being enforced is very specific to intercepted #PFs.
