diff mbox

[PATCHv3,07/17] x86/mm: Preserve KeyID on pte_modify() and pgprot_modify()

Message ID 20180612143915.68065-8-kirill.shutemov@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kirill A. Shutemov June 12, 2018, 2:39 p.m. UTC
Encrypted VMA will have KeyID stored in vma->vm_page_prot. This way we
don't need to do anything special to setup encrypted page table entries
and don't need to reserve space for KeyID in a VMA.

This patch changes _PAGE_CHG_MASK to include KeyID bits. Otherwise they
are going to be stripped from vm_page_prot on the first pgprot_modify().

Define PTE_PFN_MASK_MAX similar to PTE_PFN_MASK but based on
__PHYSICAL_MASK_SHIFT. This way we include whole range of bits
architecturally available for PFN without referencing physical_mask and
mktme_keyid_mask variables.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/x86/include/asm/pgtable_types.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Dave Hansen June 13, 2018, 6:13 p.m. UTC | #1
On 06/12/2018 07:39 AM, Kirill A. Shutemov wrote:
> Encrypted VMA will have KeyID stored in vma->vm_page_prot. This way we

"An encrypted VMA..."

> don't need to do anything special to setup encrypted page table entries
> and don't need to reserve space for KeyID in a VMA.
> 
> This patch changes _PAGE_CHG_MASK to include KeyID bits. Otherwise they
> are going to be stripped from vm_page_prot on the first pgprot_modify().
> 
> Define PTE_PFN_MASK_MAX similar to PTE_PFN_MASK but based on
> __PHYSICAL_MASK_SHIFT. This way we include whole range of bits
> architecturally available for PFN without referencing physical_mask and
> mktme_keyid_mask variables.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/include/asm/pgtable_types.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
> index 1e5a40673953..e8ebe760b88d 100644
> --- a/arch/x86/include/asm/pgtable_types.h
> +++ b/arch/x86/include/asm/pgtable_types.h
> @@ -121,8 +121,13 @@
>   * protection key is treated like _PAGE_RW, for
>   * instance, and is *not* included in this mask since
>   * pte_modify() does modify it.
> + *
> + * It includes full range of PFN bits regardless if they were claimed for KeyID
> + * or not: we want to preserve KeyID on pte_modify() and pgprot_modify().
>   */
> -#define _PAGE_CHG_MASK	(PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT |		\
> +#define PTE_PFN_MASK_MAX \
> +	(((signed long)PAGE_MASK) & ((1ULL << __PHYSICAL_MASK_SHIFT) - 1))

"signed long" is really unusual to see.  Was that intentional?

> +#define _PAGE_CHG_MASK	(PTE_PFN_MASK_MAX | _PAGE_PCD | _PAGE_PWT |		\
>  			 _PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY |	\
>  			 _PAGE_SOFT_DIRTY)
>  #define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE)

This makes me a bit nervous.  We have some places (here) where we
pretend that the KeyID is part of the paddr and then other places like
pte_pfn() where it's not.

Seems like something that will come back to bite us.
Kirill A. Shutemov June 15, 2018, 12:57 p.m. UTC | #2
On Wed, Jun 13, 2018 at 06:13:03PM +0000, Dave Hansen wrote:
> On 06/12/2018 07:39 AM, Kirill A. Shutemov wrote:
> > Encrypted VMA will have KeyID stored in vma->vm_page_prot. This way we
> 
> "An encrypted VMA..."
> 
> > don't need to do anything special to setup encrypted page table entries
> > and don't need to reserve space for KeyID in a VMA.
> > 
> > This patch changes _PAGE_CHG_MASK to include KeyID bits. Otherwise they
> > are going to be stripped from vm_page_prot on the first pgprot_modify().
> > 
> > Define PTE_PFN_MASK_MAX similar to PTE_PFN_MASK but based on
> > __PHYSICAL_MASK_SHIFT. This way we include whole range of bits
> > architecturally available for PFN without referencing physical_mask and
> > mktme_keyid_mask variables.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  arch/x86/include/asm/pgtable_types.h | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
> > index 1e5a40673953..e8ebe760b88d 100644
> > --- a/arch/x86/include/asm/pgtable_types.h
> > +++ b/arch/x86/include/asm/pgtable_types.h
> > @@ -121,8 +121,13 @@
> >   * protection key is treated like _PAGE_RW, for
> >   * instance, and is *not* included in this mask since
> >   * pte_modify() does modify it.
> > + *
> > + * It includes full range of PFN bits regardless if they were claimed for KeyID
> > + * or not: we want to preserve KeyID on pte_modify() and pgprot_modify().
> >   */
> > -#define _PAGE_CHG_MASK	(PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT |		\
> > +#define PTE_PFN_MASK_MAX \
> > +	(((signed long)PAGE_MASK) & ((1ULL << __PHYSICAL_MASK_SHIFT) - 1))
> 
> "signed long" is really unusual to see.  Was that intentional?

Yes. That's trick with sign-extension, borrowed from PHYSICAL_PAGE_MASK
definition. It helps on 32-bit with PAE properly expand the PAGE_MASK to
64-bit.

I'll add comment.

> > +#define _PAGE_CHG_MASK	(PTE_PFN_MASK_MAX | _PAGE_PCD | _PAGE_PWT |		\
> >  			 _PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY |	\
> >  			 _PAGE_SOFT_DIRTY)
> >  #define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE)
> 
> This makes me a bit nervous.  We have some places (here) where we
> pretend that the KeyID is part of the paddr and then other places like
> pte_pfn() where it's not.

Other option is to include KeyID mask into _PAGE_CHG_MASK. But it means
_PAGE_CHG_MASK would need to reference *two* variables: physical_mask and
mktme_keyid_mask. I mentioned this in the commit message.

This is more efficient way to achieve the same compile-time without
referencing any variables.

> Seems like something that will come back to bite us.

Any suggestions?
Dave Hansen June 15, 2018, 1:43 p.m. UTC | #3
On 06/15/2018 05:57 AM, Kirill A. Shutemov wrote:
>>> +#define _PAGE_CHG_MASK	(PTE_PFN_MASK_MAX | _PAGE_PCD | _PAGE_PWT |		\
>>>  			 _PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY |	\
>>>  			 _PAGE_SOFT_DIRTY)
>>>  #define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE)
>> This makes me a bit nervous.  We have some places (here) where we
>> pretend that the KeyID is part of the paddr and then other places like
>> pte_pfn() where it's not.
> Other option is to include KeyID mask into _PAGE_CHG_MASK. But it means
> _PAGE_CHG_MASK would need to reference *two* variables: physical_mask and
> mktme_keyid_mask. I mentioned this in the commit message.

Why can't it be one variable with a different name that's populated by
OR'ing physical_mask and mktme_keyid_mask together?

My issue here is that it this approach adds confusion around the logical
separation between physical address and the bits immediately above the
physical address in the PTE that are stolen for the keyID.

Whatever you come up with will probably fine, as long as things that are
called "PFN" or physical address don't also get used for keyID bits.
Kirill A. Shutemov June 15, 2018, 3:27 p.m. UTC | #4
On Fri, Jun 15, 2018 at 01:43:03PM +0000, Dave Hansen wrote:
> On 06/15/2018 05:57 AM, Kirill A. Shutemov wrote:
> >>> +#define _PAGE_CHG_MASK	(PTE_PFN_MASK_MAX | _PAGE_PCD | _PAGE_PWT |		\
> >>>  			 _PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY |	\
> >>>  			 _PAGE_SOFT_DIRTY)
> >>>  #define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE)
> >> This makes me a bit nervous.  We have some places (here) where we
> >> pretend that the KeyID is part of the paddr and then other places like
> >> pte_pfn() where it's not.
> > Other option is to include KeyID mask into _PAGE_CHG_MASK. But it means
> > _PAGE_CHG_MASK would need to reference *two* variables: physical_mask and
> > mktme_keyid_mask. I mentioned this in the commit message.
> 
> Why can't it be one variable with a different name that's populated by
> OR'ing physical_mask and mktme_keyid_mask together?

My point is that we don't need variables at all here.

Architecture defines range of bits in PTE used for PFN. MKTME reduces the
number of bits for PFN. PTE_PFN_MASK_MAX represents the original
architectural range, before MKTME stole these bits.

PTE_PFN_MASK_MAX is constant -- on x86-64 bits 51:12 -- regardless of
MKTME support.

> My issue here is that it this approach adds confusion around the logical
> separation between physical address and the bits immediately above the
> physical address in the PTE that are stolen for the keyID.

Well, yes, with MKTME the meaning of PFN/physical address is not that clear
cut. This comes from hardware design. I don't think we will be able to get
rid of ambiguity completely.

If you have suggestions on how to make it clearer, I would be glad to
rework it.

> Whatever you come up with will probably fine, as long as things that are
> called "PFN" or physical address don't also get used for keyID bits.

We are arguing about macros used exactly once. Is it really so confusing?
Dave Hansen June 15, 2018, 3:31 p.m. UTC | #5
On 06/15/2018 08:27 AM, Kirill A. Shutemov wrote:
> On Fri, Jun 15, 2018 at 01:43:03PM +0000, Dave Hansen wrote:
>> On 06/15/2018 05:57 AM, Kirill A. Shutemov wrote:
>>>>> +#define _PAGE_CHG_MASK	(PTE_PFN_MASK_MAX | _PAGE_PCD | _PAGE_PWT |		\
>>>>>  			 _PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY |	\
>>>>>  			 _PAGE_SOFT_DIRTY)
>>>>>  #define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE)
>>>> This makes me a bit nervous.  We have some places (here) where we
>>>> pretend that the KeyID is part of the paddr and then other places like
>>>> pte_pfn() where it's not.
>>> Other option is to include KeyID mask into _PAGE_CHG_MASK. But it means
>>> _PAGE_CHG_MASK would need to reference *two* variables: physical_mask and
>>> mktme_keyid_mask. I mentioned this in the commit message.
>>
>> Why can't it be one variable with a different name that's populated by
>> OR'ing physical_mask and mktme_keyid_mask together?
> 
> My point is that we don't need variables at all here.
> 
> Architecture defines range of bits in PTE used for PFN. MKTME reduces the
> number of bits for PFN. PTE_PFN_MASK_MAX represents the original
> architectural range, before MKTME stole these bits.
> 
> PTE_PFN_MASK_MAX is constant -- on x86-64 bits 51:12 -- regardless of
> MKTME support.

Then please just rename the make PTE_<SOMETHING>_MASK where <SOMETHING>
includes both the concept of a physical address and a MKTME keyID.  Just
don't call it a pfn if it is not used in physical addressing.

>> Whatever you come up with will probably fine, as long as things that are
>> called "PFN" or physical address don't also get used for keyID bits.
> 
> We are arguing about macros used exactly once. Is it really so confusing?

Yes.
Kirill A. Shutemov June 15, 2018, 4:06 p.m. UTC | #6
On Fri, Jun 15, 2018 at 03:31:57PM +0000, Dave Hansen wrote:
> On 06/15/2018 08:27 AM, Kirill A. Shutemov wrote:
> > On Fri, Jun 15, 2018 at 01:43:03PM +0000, Dave Hansen wrote:
> >> On 06/15/2018 05:57 AM, Kirill A. Shutemov wrote:
> >>>>> +#define _PAGE_CHG_MASK	(PTE_PFN_MASK_MAX | _PAGE_PCD | _PAGE_PWT |		\
> >>>>>  			 _PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY |	\
> >>>>>  			 _PAGE_SOFT_DIRTY)
> >>>>>  #define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE)
> >>>> This makes me a bit nervous.  We have some places (here) where we
> >>>> pretend that the KeyID is part of the paddr and then other places like
> >>>> pte_pfn() where it's not.
> >>> Other option is to include KeyID mask into _PAGE_CHG_MASK. But it means
> >>> _PAGE_CHG_MASK would need to reference *two* variables: physical_mask and
> >>> mktme_keyid_mask. I mentioned this in the commit message.
> >>
> >> Why can't it be one variable with a different name that's populated by
> >> OR'ing physical_mask and mktme_keyid_mask together?
> > 
> > My point is that we don't need variables at all here.
> > 
> > Architecture defines range of bits in PTE used for PFN. MKTME reduces the
> > number of bits for PFN. PTE_PFN_MASK_MAX represents the original
> > architectural range, before MKTME stole these bits.
> > 
> > PTE_PFN_MASK_MAX is constant -- on x86-64 bits 51:12 -- regardless of
> > MKTME support.
> 
> Then please just rename the make PTE_<SOMETHING>_MASK where <SOMETHING>
> includes both the concept of a physical address and a MKTME keyID.  Just
> don't call it a pfn if it is not used in physical addressing.

I have no idea what such concept should be called. I cannot come with
anything better than PTE_PFN_MASK_MAX. Do you?
Dave Hansen June 15, 2018, 4:58 p.m. UTC | #7
On 06/15/2018 09:06 AM, Kirill A. Shutemov wrote:
> I have no idea what such concept should be called. I cannot come with
> anything better than PTE_PFN_MASK_MAX. Do you?

PTE_PRESERVE_MASK

Or maybe:

PTE_MODIFY_PRESERVE_MASK

Maybe a comment to go along with it:

/*
 * These are the bits that must be preserved during when doing a
 * PTE permission modification operation, like taking a PTE from
 * R/W->R/O.  They include the physical address and the memory
 * encryption keyID.  The paddr and the keyID never occupy the same
 * bits at the same time.  But, a given bit might be used for the keyID
 * on one system and used for the physical address on another.  As an
 * optimization, we manage them in one unit here since their combination
 * always occupies the same hardware bits.
 */
Kirill A. Shutemov June 15, 2018, 8:45 p.m. UTC | #8
On Fri, Jun 15, 2018 at 04:58:24PM +0000, Dave Hansen wrote:
> On 06/15/2018 09:06 AM, Kirill A. Shutemov wrote:
> > I have no idea what such concept should be called. I cannot come with
> > anything better than PTE_PFN_MASK_MAX. Do you?
> 
> PTE_PRESERVE_MASK
> 
> Or maybe:
> 
> PTE_MODIFY_PRESERVE_MASK

It just replacing one confusion with another. Preserve what? How does it
differ from _PAGE_CHG_MASK?

I frankly think my name proposal convey more meaning.

> Maybe a comment to go along with it:
> 
> /*
>  * These are the bits that must be preserved during when doing a
>  * PTE permission modification operation, like taking a PTE from
>  * R/W->R/O.  They include the physical address and the memory
>  * encryption keyID.  The paddr and the keyID never occupy the same
>  * bits at the same time.  But, a given bit might be used for the keyID
>  * on one system and used for the physical address on another.  As an
>  * optimization, we manage them in one unit here since their combination
>  * always occupies the same hardware bits.
>  */

Thanks, this is useful.
Dave Hansen June 15, 2018, 8:45 p.m. UTC | #9
On 06/15/2018 01:45 PM, Kirill A. Shutemov wrote:
> On Fri, Jun 15, 2018 at 04:58:24PM +0000, Dave Hansen wrote:
>> On 06/15/2018 09:06 AM, Kirill A. Shutemov wrote:
>>> I have no idea what such concept should be called. I cannot come with
>>> anything better than PTE_PFN_MASK_MAX. Do you?
>> PTE_PRESERVE_MASK
>>
>> Or maybe:
>>
>> PTE_MODIFY_PRESERVE_MASK
> It just replacing one confusion with another. Preserve what?

"Preserve this mask when modifying pte permission bits"
Kirill A. Shutemov June 15, 2018, 8:55 p.m. UTC | #10
On Fri, Jun 15, 2018 at 08:45:21PM +0000, Dave Hansen wrote:
> On 06/15/2018 01:45 PM, Kirill A. Shutemov wrote:
> > On Fri, Jun 15, 2018 at 04:58:24PM +0000, Dave Hansen wrote:
> >> On 06/15/2018 09:06 AM, Kirill A. Shutemov wrote:
> >>> I have no idea what such concept should be called. I cannot come with
> >>> anything better than PTE_PFN_MASK_MAX. Do you?
> >> PTE_PRESERVE_MASK
> >>
> >> Or maybe:
> >>
> >> PTE_MODIFY_PRESERVE_MASK
> > It just replacing one confusion with another. Preserve what?
> 
> "Preserve this mask when modifying pte permission bits"

We preserve more than these bits. See _PAGE_CHG_MASK.
diff mbox

Patch

diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 1e5a40673953..e8ebe760b88d 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -121,8 +121,13 @@ 
  * protection key is treated like _PAGE_RW, for
  * instance, and is *not* included in this mask since
  * pte_modify() does modify it.
+ *
+ * It includes full range of PFN bits regardless if they were claimed for KeyID
+ * or not: we want to preserve KeyID on pte_modify() and pgprot_modify().
  */
-#define _PAGE_CHG_MASK	(PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT |		\
+#define PTE_PFN_MASK_MAX \
+	(((signed long)PAGE_MASK) & ((1ULL << __PHYSICAL_MASK_SHIFT) - 1))
+#define _PAGE_CHG_MASK	(PTE_PFN_MASK_MAX | _PAGE_PCD | _PAGE_PWT |		\
 			 _PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY |	\
 			 _PAGE_SOFT_DIRTY)
 #define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE)