| Message ID | 20240228024147.41573-8-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86/mmu: Page fault and MMIO cleanups | expand |
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>
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. > some common place like in kvm_mmu_page_fault()? Because again, the logic being enforced is very specific to intercepted #PFs.
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>
On 2/28/2024 10:41 AM, 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 "Violatation" -> "Violation" > 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)); > + > 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, "sythentic" -> "synthetic" Two typos. Others, Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com> > + * 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) ?
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)); + 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) ?
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(-)