Re: [PATCHv5 18/19] x86/mm: Handle encrypted memory in page_to_virt() and __pa()

[Kirill A. Shutemov]

On Thu, Jul 19, 2018 at 12:21:44AM +0200, Thomas Gleixner wrote:
> On Tue, 17 Jul 2018, Kirill A. Shutemov wrote:
> 
> > Per-KeyID direct mappings require changes into how we find the right
> > virtual address for a page and virt-to-phys address translations.
> > 
> > page_to_virt() definition overwrites default macros provided by
> > <linux/mm.h>. We only overwrite the macros if MTKME is enabled
> > compile-time.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> > ---
> >  arch/x86/include/asm/mktme.h   | 3 +++
> >  arch/x86/include/asm/page_64.h | 2 +-
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/include/asm/mktme.h b/arch/x86/include/asm/mktme.h
> > index ba83fba4f9b3..dbfbd955da98 100644
> > --- a/arch/x86/include/asm/mktme.h
> > +++ b/arch/x86/include/asm/mktme.h
> > @@ -29,6 +29,9 @@ void arch_free_page(struct page *page, int order);
> >  
> >  int sync_direct_mapping(void);
> >  
> > +#define page_to_virt(x) \
> > +	(__va(PFN_PHYS(page_to_pfn(x))) + page_keyid(x) * direct_mapping_size)
> 
> This really does not belong into the mktme header.
> 
> Please make this the unconditional x86 page_to_virt() implementation in
> asm/page.h, which is the canonical and obvious place for it.

Okay. (and I owe Dave a beer on this :P)

> The page_keyid() name is quite generic as well. Can this please have some
> kind of reference to the underlying mechanism, i.e. mktme?

Hm. I intentially get the name generic. It used outside arch/x86.

We can have an alias (mktme_page_keyid() ?) to be used in arch/x86 to
indicate undelying mechanism.

Is it what you want to see?

> Please hide the multiplication with direct_mapping_size in the mktme header
> as well. It's non interesting for the !MKTME case. Something like:
> 
> #define page_to_virt(x) \
> 	(__va(PFN_PHYS(page_to_pfn(x))) + mktme_page_to_virt_offset(x))
> 
> makes it immediately clear where to look and also makes it clear that the
> offset will be 0 for a !MKTME enabled kernel and (hopefully) for all !MKTME
> enabled processors as well.
> 
> And then have a proper implementation of mktme_page_to_virt_offset() with a
> proper comment what on earth this is doing. It might be all obvious to you
> now, but it's completely non obvious for the casual reader and you will
> have to twist your brain around it 6 month from now as well.

Sure.

> >  #else
> >  #define mktme_keyid_mask	((phys_addr_t)0)
> >  #define mktme_nr_keyids		0
> > diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h
> > index f57fc3cc2246..a4f394e3471d 100644
> > --- a/arch/x86/include/asm/page_64.h
> > +++ b/arch/x86/include/asm/page_64.h
> > @@ -24,7 +24,7 @@ static inline unsigned long __phys_addr_nodebug(unsigned long x)
> >  	/* use the carry flag to determine if x was < __START_KERNEL_map */
> >  	x = y + ((x > y) ? phys_base : (__START_KERNEL_map - PAGE_OFFSET));
> >  
> > -	return x;
> > +	return x & direct_mapping_mask;
> 
> This hunk also lacks any explanation both in the changelog and in form of a
> comment.

I'll fix it.

> > Per-KeyID direct mappings require changes into how we find the right
> > virtual address for a page and virt-to-phys address translations.
> 
> That's pretty useless as it does just tell about 'changes', but not at all
> about what kind of changes and why these changes are required. It's really
> not helpful to assume that everyone stumbling over this will know the whole
> story especially not 6 month after this has been merged and then someone
> ends up with a bisect on that change.
> 
> While at it, please get rid of the 'we'. We are neither CPUs nor code.

Okay. I'll rewrite this.

-- 
 Kirill A. Shutemov