| Message ID | 20220123195239.509528-1-ayushranjan@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: add additional EPT bit definitions | expand |
On Sun, Jan 23, 2022, Ayush Ranjan wrote: > From: Michael Pratt <mpratt@google.com> > > Used in gvisor for EPT support. As you may have surmised from the other patch, the changelogs from patches carried in our internal kernels rarely meet the criteria for acceptance upstream. E.g. this doesn't provide sufficient justification since there's obviously no in-kernel gvisor that's consuming this. Submitting patches that we carry internally is perfectly ok, but there needs to be sufficient justfication, and the patch needs to follow the rules laid out by Documentation/process/submitting-patches.rst. > Tested: Builds cleanly > Signed-off-by: Ayush Ranjan <ayushranjan@google.com> > Signed-off-by: Michael Pratt <mpratt@google.com> > --- > arch/x86/include/asm/vmx.h | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h > index 0ffaa3156a4e..c77ad687cdf7 100644 > --- a/arch/x86/include/asm/vmx.h > +++ b/arch/x86/include/asm/vmx.h > @@ -496,7 +496,9 @@ enum vmcs_field { > #define VMX_EPT_WRITABLE_MASK 0x2ull > #define VMX_EPT_EXECUTABLE_MASK 0x4ull > #define VMX_EPT_IPAT_BIT (1ull << 6) > -#define VMX_EPT_ACCESS_BIT (1ull << 8) > +#define VMX_EPT_PSE_BIT (1ull << 7) I'm not a fan of "PSE", it's unnecessarily terse and "PSE" has different meaning in IA32 paging. VMX_EPT_PAGE_SIZE_BIT would be choice. As for justification, something that has been mentioned once or thrice is the lack of build-time assertions that the PT_* bits in mmu.h that are reused for EPT entries do indeed match the EPT definitions. I can throw together a patch/series to add that and do the below cleanup. > +#define VMX_EPT_ACCESS_SHIFT 8 I'd prefer we don't define the "shifts" for EPT (or PTE) bits, they really shouldn't be used as doing things like test_and_clear_bit() via a shift value can generate unnecessary lock instructions. arch/x86/kvm/mmu.h could use a bit of spring cleaning in this regard. > +#define VMX_EPT_ACCESS_BIT (1ull << VMX_EPT_ACCESS_SHIFT) > #define VMX_EPT_DIRTY_BIT (1ull << 9) > #define VMX_EPT_RWX_MASK (VMX_EPT_READABLE_MASK | \ > VMX_EPT_WRITABLE_MASK | \ > -- > 2.35.0.rc0.227.g00780c9af4-goog >
Abandoning this patch. On Mon, Jan 24, 2022 at 9:24 AM Sean Christopherson <seanjc@google.com> wrote: > > On Sun, Jan 23, 2022, Ayush Ranjan wrote: > > From: Michael Pratt <mpratt@google.com> > > > > Used in gvisor for EPT support. > > As you may have surmised from the other patch, the changelogs from patches carried > in our internal kernels rarely meet the criteria for acceptance upstream. E.g. this > doesn't provide sufficient justification since there's obviously no in-kernel gvisor > that's consuming this. > > Submitting patches that we carry internally is perfectly ok, but there needs to be > sufficient justfication, and the patch needs to follow the rules laid out by > Documentation/process/submitting-patches.rst. > > > Tested: Builds cleanly > > Signed-off-by: Ayush Ranjan <ayushranjan@google.com> > > Signed-off-by: Michael Pratt <mpratt@google.com> > > --- > > arch/x86/include/asm/vmx.h | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h > > index 0ffaa3156a4e..c77ad687cdf7 100644 > > --- a/arch/x86/include/asm/vmx.h > > +++ b/arch/x86/include/asm/vmx.h > > @@ -496,7 +496,9 @@ enum vmcs_field { > > #define VMX_EPT_WRITABLE_MASK 0x2ull > > #define VMX_EPT_EXECUTABLE_MASK 0x4ull > > #define VMX_EPT_IPAT_BIT (1ull << 6) > > -#define VMX_EPT_ACCESS_BIT (1ull << 8) > > +#define VMX_EPT_PSE_BIT (1ull << 7) > > I'm not a fan of "PSE", it's unnecessarily terse and "PSE" has different meaning > in IA32 paging. VMX_EPT_PAGE_SIZE_BIT would be choice. > > As for justification, something that has been mentioned once or thrice is the lack > of build-time assertions that the PT_* bits in mmu.h that are reused for EPT entries > do indeed match the EPT definitions. I can throw together a patch/series to add > that and do the below cleanup. > > > +#define VMX_EPT_ACCESS_SHIFT 8 > > I'd prefer we don't define the "shifts" for EPT (or PTE) bits, they really shouldn't > be used as doing things like test_and_clear_bit() via a shift value can generate > unnecessary lock instructions. arch/x86/kvm/mmu.h could use a bit of spring cleaning > in this regard. > > > +#define VMX_EPT_ACCESS_BIT (1ull << VMX_EPT_ACCESS_SHIFT) > > #define VMX_EPT_DIRTY_BIT (1ull << 9) > > #define VMX_EPT_RWX_MASK (VMX_EPT_READABLE_MASK | \ > > VMX_EPT_WRITABLE_MASK | \ > > -- > > 2.35.0.rc0.227.g00780c9af4-goog > >
diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h index 0ffaa3156a4e..c77ad687cdf7 100644 --- a/arch/x86/include/asm/vmx.h +++ b/arch/x86/include/asm/vmx.h @@ -496,7 +496,9 @@ enum vmcs_field { #define VMX_EPT_WRITABLE_MASK 0x2ull #define VMX_EPT_EXECUTABLE_MASK 0x4ull #define VMX_EPT_IPAT_BIT (1ull << 6) -#define VMX_EPT_ACCESS_BIT (1ull << 8) +#define VMX_EPT_PSE_BIT (1ull << 7) +#define VMX_EPT_ACCESS_SHIFT 8 +#define VMX_EPT_ACCESS_BIT (1ull << VMX_EPT_ACCESS_SHIFT) #define VMX_EPT_DIRTY_BIT (1ull << 9) #define VMX_EPT_RWX_MASK (VMX_EPT_READABLE_MASK | \ VMX_EPT_WRITABLE_MASK | \