Re: [PATCH] x86/mm/cpa: set PAGE_KERNEL in __set_pages_p()

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Tue, 2022-05-10 at 20:50 +0900, Hyeonggon Yoo wrote:
> Thanks for review!
> 
> IIUC __pgprot_mask() already clears _PAGE_GLOBAL from PAGE_KERNEL
> when PTI is used.
> 
> #define __pgprot_mask(x)        __pgprot((x) &
> __default_kernel_pte_mask)
> #define PAGE_KERNEL             __pgprot_mask(__PAGE_KERNEL | _ENC)
> 
> But yeah, it seems PAGE_KERNEL is too much for this.

Oh, yep, you're right.

> 
> > Maybe something like this?
> > (_PAGE_PRESENT | _PAGE_RW | _PAGE_GLOBAL) &
> > __default_kernel_pte_mask
> > 
> 
> What about __pgprot_mask(_PAGE_PRESENT | _PAGE_RW | _PAGE_GLOBAL)?
> 
> > That would add back in a little of the "default global" behavior
> > that
> > was removed in d1440b2, but I think it should be ok because it is
> > limited to the direct map. 
> > Otherwise, I wonder if the existing global
> > clearing logic is really needed.
> 
> I think it's still needed. pte_present() returning true due to
> _PAGE_PROTNONE
> after __set_pages_np() simply doesn't make sense. No?

Yea, I just meant we don't need PROTNONE in the kernel PTEs (I think),
so the kernel is forced to do weird stuff in CPA so that it can share
the pte helpers with userspace.

I have not thought this all the way through, but say we declared that
PROT_NONE doesn't exist in kernel pte's, then pte_present() could be
like:

static inline int pte_present(pte_t a)
{
	pteval_t val = pte_flags(a)
	bool kernel_p = val & (_PAGE_USER | _PAGE_PRESENT) ==
			_PAGE_PRESENT;
	bool user_p = (val & _PAGE_USER) &&
		      (val & (_PAGE_PRESENT | _PAGE_PROTNONE));

	/*
	 * PROT_NONE does not exist for kernel PTEs, but global and
	 * not present can appear in other cases. Check each
	 * differently depending on the user bit.
	 */
	return kernel_p || user_p;
}

Not sure where else it ties in. I think that is what Dave was getting
at in the other thread - to figure out all the places that need to
change to remove the meaning from kernel PTEs.

Re-setting global seems valid too.