| Message ID | 20250227000705.3199706-3-seanjc@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: VMX: Clean up EPT_VIOLATIONS_xxx #defines | expand |
On 27.02.25 г. 2:07 ч., Sean Christopherson wrote: > Define independent macros for the RWX protection bits that are enumerated > via EXIT_QUALIFICATION for EPT Violations, and tie them to the RWX bits in > EPT entries via compile-time asserts. Piggybacking the EPTE defines works > for now, but it creates holes in the EPT_VIOLATION_xxx macros and will > cause headaches if/when KVM emulates Mode-Based Execution (MBEC), or any > other features that introduces additional protection information. > > Opportunistically rename EPT_VIOLATION_RWX_MASK to EPT_VIOLATION_PROT_MASK > so that it doesn't become stale if/when MBEC support is added. > > No functional change intended. > > Cc: Jon Kohler <jon@nutanix.com> > Cc: Nikolay Borisov <nik.borisov@suse.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
> On Feb 27, 2025, at 1:52 AM, Nikolay Borisov <nik.borisov@suse.com> wrote: > > !-------------------------------------------------------------------| > CAUTION: External Email > > |-------------------------------------------------------------------! > > > > On 27.02.25 г. 2:07 ч., Sean Christopherson wrote: >> Define independent macros for the RWX protection bits that are enumerated >> via EXIT_QUALIFICATION for EPT Violations, and tie them to the RWX bits in >> EPT entries via compile-time asserts. Piggybacking the EPTE defines works >> for now, but it creates holes in the EPT_VIOLATION_xxx macros and will >> cause headaches if/when KVM emulates Mode-Based Execution (MBEC), or any >> other features that introduces additional protection information. >> Opportunistically rename EPT_VIOLATION_RWX_MASK to EPT_VIOLATION_PROT_MASK >> so that it doesn't become stale if/when MBEC support is added. >> No functional change intended. >> Cc: Jon Kohler <jon@nutanix.com> >> Cc: Nikolay Borisov <nik.borisov@suse.com> >> Signed-off-by: Sean Christopherson <seanjc@google.com> > > Reviewed-by: Nikolay Borisov <nik.borisov@suse.com> LGTM, but any chance we could hold this until I get the MBEC RFC out? My apologies on the delay, I caught a terrible chest cold after we met about it, followed by a secondary case of strep! Just getting back into the grind now, so I need to rebase and send those out. For anyone curious, the drafts are here: https://github.com/JonKohler/linux/tree/mbec-rfc-v1-6.12 https://github.com/JonKohler/qemu/tree/mbec-rfc-v1 I need to incorporate some early off-list review comments and send it out properly, but in reference to this specific change, you can see how I approached it here: https://github.com/JonKohler/linux/commit/0d2e82704ed3eb28c105967c8acd7907523ded5b
On Thu, Feb 27, 2025, Jon Kohler wrote: > > On Feb 27, 2025, at 1:52 AM, Nikolay Borisov <nik.borisov@suse.com> wrote: > > > > !-------------------------------------------------------------------| > > CAUTION: External Email Noted. :-D > > |-------------------------------------------------------------------! > > > > On 27.02.25 г. 2:07 ч., Sean Christopherson wrote: > >> Define independent macros for the RWX protection bits that are enumerated > >> via EXIT_QUALIFICATION for EPT Violations, and tie them to the RWX bits in > >> EPT entries via compile-time asserts. Piggybacking the EPTE defines works > >> for now, but it creates holes in the EPT_VIOLATION_xxx macros and will > >> cause headaches if/when KVM emulates Mode-Based Execution (MBEC), or any > >> other features that introduces additional protection information. > >> Opportunistically rename EPT_VIOLATION_RWX_MASK to EPT_VIOLATION_PROT_MASK > >> so that it doesn't become stale if/when MBEC support is added. > >> No functional change intended. > >> Cc: Jon Kohler <jon@nutanix.com> > >> Cc: Nikolay Borisov <nik.borisov@suse.com> > >> Signed-off-by: Sean Christopherson <seanjc@google.com> > > > > Reviewed-by: Nikolay Borisov <nik.borisov@suse.com> > > LGTM, but any chance we could hold this until I get the MBEC RFC out? No? It's definitely landing before the MBEC support, and IOM it works quite nicely with the MBEC support (my diff at the bottom). I don't see any reason to delay or change this cleanup. > My apologies on the delay, I caught a terrible chest cold after we met about > it, followed by a secondary case of strep! Ow. Don't rush on behalf of upstream, KVM has lived without MBEC for a long time, it's not going anywhere.o --- arch/x86/include/asm/vmx.h | 4 +++- arch/x86/kvm/mmu/paging_tmpl.h | 9 +++++++-- arch/x86/kvm/vmx/vmx.c | 7 +++++++ 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h index d7ab0ad63be6..61e31e915e46 100644 --- a/arch/x86/include/asm/vmx.h +++ b/arch/x86/include/asm/vmx.h @@ -587,9 +587,11 @@ enum vm_entry_failure_code { #define EPT_VIOLATION_PROT_READ BIT(3) #define EPT_VIOLATION_PROT_WRITE BIT(4) #define EPT_VIOLATION_PROT_EXEC BIT(5) +#define EPT_VIOLATION_PROT_USER_EXEC BIT(6) #define EPT_VIOLATION_PROT_MASK (EPT_VIOLATION_PROT_READ | \ EPT_VIOLATION_PROT_WRITE | \ - EPT_VIOLATION_PROT_EXEC) + EPT_VIOLATION_PROT_EXEC | \ + EPT_VIOLATION_PROT_USER_EXEC) #define EPT_VIOLATION_GVA_IS_VALID BIT(7) #define EPT_VIOLATION_GVA_TRANSLATED BIT(8) diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h index 68e323568e95..ede8207bf4d7 100644 --- a/arch/x86/kvm/mmu/paging_tmpl.h +++ b/arch/x86/kvm/mmu/paging_tmpl.h @@ -181,8 +181,9 @@ static inline unsigned FNAME(gpte_access)(u64 gpte) unsigned access; #if PTTYPE == PTTYPE_EPT access = ((gpte & VMX_EPT_WRITABLE_MASK) ? ACC_WRITE_MASK : 0) | - ((gpte & VMX_EPT_EXECUTABLE_MASK) ? ACC_EXEC_MASK : 0) | - ((gpte & VMX_EPT_READABLE_MASK) ? ACC_USER_MASK : 0); + ((gpte & VMX_EPT_EXECUTABLE_MASK) ? ACC_EXEC_MASK : 0) | + ((gpte & VMX_EPT_USER_EXECUTABLE_MASK) ? ACC_USER_EXEC_MASK : 0) | + ((gpte & VMX_EPT_READABLE_MASK) ? ACC_USER_MASK : 0); #else BUILD_BUG_ON(ACC_EXEC_MASK != PT_PRESENT_MASK); BUILD_BUG_ON(ACC_EXEC_MASK != 1); @@ -511,6 +512,10 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker, * ACC_*_MASK flags! */ walker->fault.exit_qualification |= EPT_VIOLATION_RWX_TO_PROT(pte_access); + /* This is also wrong.*/ + if (vcpu->arch.pt_guest_exec_control && + (pte_access & VMX_EPT_USER_EXECUTABLE_MASK)) + walker->fault.exit_qualification |= EPT_VIOLATION_PROT_USER_EXEC; } #endif walker->fault.address = addr; diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 0db64f4adf2a..4684647ef063 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -5806,6 +5806,13 @@ static int handle_ept_violation(struct kvm_vcpu *vcpu) exit_qualification = vmx_get_exit_qual(vcpu); + /* + * The USER_EXEC flag is undefined if MBEC is disabled. + * Note, this is wrong, MBEC should be a property of the MMU. + */ + if (!vcpu->arch.pt_guest_exec_control) + exit_qualification &= ~EPT_VIOLATION_PROT_USER_EXEC; + /* * EPT violation happened while executing iret from NMI, * "blocked by NMI" bit has to be set before next VM entry. base-commit: 67983df09fc3f96d0d6107fe1a99d29460bab481 --
> On Feb 27, 2025, at 2:34 PM, Sean Christopherson <seanjc@google.com> wrote: > > On Thu, Feb 27, 2025, Jon Kohler wrote: >>> On Feb 27, 2025, at 1:52 AM, Nikolay Borisov <nik.borisov@suse.com> wrote: >>> >>> !-------------------------------------------------------------------| >>> CAUTION: External Email > > Noted. :-D Silly IT ! > >>> |-------------------------------------------------------------------! >>> >>> On 27.02.25 г. 2:07 ч., Sean Christopherson wrote: >>>> Define independent macros for the RWX protection bits that are enumerated >>>> via EXIT_QUALIFICATION for EPT Violations, and tie them to the RWX bits in >>>> EPT entries via compile-time asserts. Piggybacking the EPTE defines works >>>> for now, but it creates holes in the EPT_VIOLATION_xxx macros and will >>>> cause headaches if/when KVM emulates Mode-Based Execution (MBEC), or any >>>> other features that introduces additional protection information. >>>> Opportunistically rename EPT_VIOLATION_RWX_MASK to EPT_VIOLATION_PROT_MASK >>>> so that it doesn't become stale if/when MBEC support is added. >>>> No functional change intended. >>>> Cc: Jon Kohler <jon@nutanix.com> >>>> Cc: Nikolay Borisov <nik.borisov@suse.com> >>>> Signed-off-by: Sean Christopherson <seanjc@google.com> >>> >>> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com> >> >> LGTM, but any chance we could hold this until I get the MBEC RFC out? > > No? It's definitely landing before the MBEC support, and IOM it works quite nicely > with the MBEC support (my diff at the bottom). I don't see any reason to delay > or change this cleanup. Ok no problem at all, happy to rebase on top of this when it lands. Thanks for the suggestions on the diff, will give it a poke > >> My apologies on the delay, I caught a terrible chest cold after we met about >> it, followed by a secondary case of strep! > > Ow. Don't rush on behalf of upstream, KVM has lived without MBEC for a long time, > it's not going anywhere.o > > --- > arch/x86/include/asm/vmx.h | 4 +++- > arch/x86/kvm/mmu/paging_tmpl.h | 9 +++++++-- > arch/x86/kvm/vmx/vmx.c | 7 +++++++ > 3 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h > index d7ab0ad63be6..61e31e915e46 100644 > --- a/arch/x86/include/asm/vmx.h > +++ b/arch/x86/include/asm/vmx.h > @@ -587,9 +587,11 @@ enum vm_entry_failure_code { > #define EPT_VIOLATION_PROT_READ BIT(3) > #define EPT_VIOLATION_PROT_WRITE BIT(4) > #define EPT_VIOLATION_PROT_EXEC BIT(5) > +#define EPT_VIOLATION_PROT_USER_EXEC BIT(6) > #define EPT_VIOLATION_PROT_MASK (EPT_VIOLATION_PROT_READ | \ > EPT_VIOLATION_PROT_WRITE | \ > - EPT_VIOLATION_PROT_EXEC) > + EPT_VIOLATION_PROT_EXEC | \ > + EPT_VIOLATION_PROT_USER_EXEC) > #define EPT_VIOLATION_GVA_IS_VALID BIT(7) > #define EPT_VIOLATION_GVA_TRANSLATED BIT(8) > > diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h > index 68e323568e95..ede8207bf4d7 100644 > --- a/arch/x86/kvm/mmu/paging_tmpl.h > +++ b/arch/x86/kvm/mmu/paging_tmpl.h > @@ -181,8 +181,9 @@ static inline unsigned FNAME(gpte_access)(u64 gpte) > unsigned access; > #if PTTYPE == PTTYPE_EPT > access = ((gpte & VMX_EPT_WRITABLE_MASK) ? ACC_WRITE_MASK : 0) | > - ((gpte & VMX_EPT_EXECUTABLE_MASK) ? ACC_EXEC_MASK : 0) | > - ((gpte & VMX_EPT_READABLE_MASK) ? ACC_USER_MASK : 0); > + ((gpte & VMX_EPT_EXECUTABLE_MASK) ? ACC_EXEC_MASK : 0) | > + ((gpte & VMX_EPT_USER_EXECUTABLE_MASK) ? ACC_USER_EXEC_MASK : 0) | > + ((gpte & VMX_EPT_READABLE_MASK) ? ACC_USER_MASK : 0); > #else > BUILD_BUG_ON(ACC_EXEC_MASK != PT_PRESENT_MASK); > BUILD_BUG_ON(ACC_EXEC_MASK != 1); > @@ -511,6 +512,10 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker, > * ACC_*_MASK flags! > */ > walker->fault.exit_qualification |= EPT_VIOLATION_RWX_TO_PROT(pte_access); > + /* This is also wrong.*/ > + if (vcpu->arch.pt_guest_exec_control && > + (pte_access & VMX_EPT_USER_EXECUTABLE_MASK)) > + walker->fault.exit_qualification |= EPT_VIOLATION_PROT_USER_EXEC; > } > #endif > walker->fault.address = addr; > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 0db64f4adf2a..4684647ef063 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -5806,6 +5806,13 @@ static int handle_ept_violation(struct kvm_vcpu *vcpu) > > exit_qualification = vmx_get_exit_qual(vcpu); > > + /* > + * The USER_EXEC flag is undefined if MBEC is disabled. > + * Note, this is wrong, MBEC should be a property of the MMU. > + */ > + if (!vcpu->arch.pt_guest_exec_control) > + exit_qualification &= ~EPT_VIOLATION_PROT_USER_EXEC; > + > /* > * EPT violation happened while executing iret from NMI, > * "blocked by NMI" bit has to be set before next VM entry. > > base-commit: 67983df09fc3f96d0d6107fe1a99d29460bab481 > -- >
On Thu, Feb 27, 2025, Jon Kohler wrote: > > On Feb 27, 2025, at 2:34 PM, Sean Christopherson <seanjc@google.com> wrote: > >> LGTM, but any chance we could hold this until I get the MBEC RFC out? > > > > No? It's definitely landing before the MBEC support, and IOM it works quite nicely > > with the MBEC support (my diff at the bottom). I don't see any reason to delay > > or change this cleanup. > > Ok no problem at all, happy to rebase on top of this when it lands. FWIW, you don't have to wait for this to land to send your RFC. You could send your RFC as-is; obviously I'd point out the conflict, but (a) it's an RFC and (b) generally it's not your responsibility to anticipate conflicts. Alternatively, and probably better in this case, would be include these patches in your RFC, with a short message in the cover letter explaining their existence. That said, I'm guessing I'll beat you to the punch and get this landed in kvm-x86 next before you send the RFC :-)
diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h index aabc223c6498..8707361b24da 100644 --- a/arch/x86/include/asm/vmx.h +++ b/arch/x86/include/asm/vmx.h @@ -580,14 +580,23 @@ enum vm_entry_failure_code { /* * Exit Qualifications for EPT Violations */ -#define EPT_VIOLATION_RWX_SHIFT 3 #define EPT_VIOLATION_ACC_READ BIT(0) #define EPT_VIOLATION_ACC_WRITE BIT(1) #define EPT_VIOLATION_ACC_INSTR BIT(2) -#define EPT_VIOLATION_RWX_MASK (VMX_EPT_RWX_MASK << EPT_VIOLATION_RWX_SHIFT) +#define EPT_VIOLATION_PROT_READ BIT(3) +#define EPT_VIOLATION_PROT_WRITE BIT(4) +#define EPT_VIOLATION_PROT_EXEC BIT(5) +#define EPT_VIOLATION_PROT_MASK (EPT_VIOLATION_PROT_READ | \ + EPT_VIOLATION_PROT_WRITE | \ + EPT_VIOLATION_PROT_EXEC) #define EPT_VIOLATION_GVA_IS_VALID BIT(7) #define EPT_VIOLATION_GVA_TRANSLATED BIT(8) +#define EPT_VIOLATION_RWX_TO_PROT(__epte) (((__epte) & VMX_EPT_RWX_MASK) << 3) + +static_assert(EPT_VIOLATION_RWX_TO_PROT(VMX_EPT_RWX_MASK) == + (EPT_VIOLATION_PROT_READ | EPT_VIOLATION_PROT_WRITE | EPT_VIOLATION_PROT_EXEC)); + /* * Exit Qualifications for NOTIFY VM EXIT */ diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h index f4711674c47b..68e323568e95 100644 --- a/arch/x86/kvm/mmu/paging_tmpl.h +++ b/arch/x86/kvm/mmu/paging_tmpl.h @@ -510,8 +510,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker, * Note, pte_access holds the raw RWX bits from the EPTE, not * ACC_*_MASK flags! */ - walker->fault.exit_qualification |= (pte_access & VMX_EPT_RWX_MASK) << - EPT_VIOLATION_RWX_SHIFT; + walker->fault.exit_qualification |= EPT_VIOLATION_RWX_TO_PROT(pte_access); } #endif walker->fault.address = addr; diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index b71392989609..049f28f1b2bc 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -5821,7 +5821,7 @@ static int handle_ept_violation(struct kvm_vcpu *vcpu) error_code |= (exit_qualification & EPT_VIOLATION_ACC_INSTR) ? PFERR_FETCH_MASK : 0; /* ept page table entry is present? */ - error_code |= (exit_qualification & EPT_VIOLATION_RWX_MASK) + error_code |= (exit_qualification & EPT_VIOLATION_PROT_MASK) ? PFERR_PRESENT_MASK : 0; if (error_code & EPT_VIOLATION_GVA_IS_VALID)
Define independent macros for the RWX protection bits that are enumerated via EXIT_QUALIFICATION for EPT Violations, and tie them to the RWX bits in EPT entries via compile-time asserts. Piggybacking the EPTE defines works for now, but it creates holes in the EPT_VIOLATION_xxx macros and will cause headaches if/when KVM emulates Mode-Based Execution (MBEC), or any other features that introduces additional protection information. Opportunistically rename EPT_VIOLATION_RWX_MASK to EPT_VIOLATION_PROT_MASK so that it doesn't become stale if/when MBEC support is added. No functional change intended. Cc: Jon Kohler <jon@nutanix.com> Cc: Nikolay Borisov <nik.borisov@suse.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/include/asm/vmx.h | 13 +++++++++++-- arch/x86/kvm/mmu/paging_tmpl.h | 3 +-- arch/x86/kvm/vmx/vmx.c | 2 +- 3 files changed, 13 insertions(+), 5 deletions(-)