[PATCH] x86: add additional EPT bit definitions

[PATCH] x86: add additional EPT bit definitions

[Ayush Ranjan]

From: Michael Pratt <mpratt@google.com>

Used in gvisor for EPT support.

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)
+#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 |       \
-- 
2.35.0.rc0.227.g00780c9af4-goog

Re: [PATCH] x86: add additional EPT bit definitions

[Sean Christopherson]

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
> 

Re: [PATCH] x86: add additional EPT bit definitions

[Ayush Ranjan]

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
> >