Re: [RFC PATCH 2/2] powerpc/mm: Replace _PAGE_USER with _PAGE_PRIV

On Sat, Mar 05, 2016 at 01:59:02PM +0530, Aneesh Kumar K.V wrote:

Paul Mackerras [off-list ref] writes:

quoted

[ text/plain ]
On Fri, Feb 26, 2016 at 08:50:50AM +0530, Aneesh Kumar K.V wrote:

quoted

_PAGE_PRIV means the page can be accessed only by kernel. This is done
to keep pte bits similar to PowerISA 3.0 radix PTE format. User
pages are now makred by clearing _PAGE_PRIV bit.

.....

quoted

quoted

@@ -40,6 +40,11 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
 		if (unlikely(access & ~old_pte))
 			return 1;

This check is going to do a different thing now as far as
_PAGE_USER/_PAGE_PRIV is concerned: previously it would prevent a
non-privileged access to a privileged page from creating a HPTE, now
it prevents a privileged access to a non-privileged page from creating
a HPTE.  A privileged access means an access by the kernel to a high
address, and arguably we would never have a non-privileged PTE at a
high (i.e. kernel) address, but it's still a semantic change that
should have been flagged in the patch description.

We don't set _PAGE_PRIVILGED when we have a privilged acess to a non
privilged page. We set it as below (with updated comments)

	/*
	 * We set _PAGE_PRIVILEGED only when
	 * kernel mode access kernel space.
	 *
	 * _PAGE_PRIVILGED is NOT set
	 * 1) when kernel mode access user space
	 * 2) user space access kernel space.
	 */
	if (!(msr & MSR_PR) && !(REGION_ID(ea) == USER_REGION_ID))
		access |= _PAGE_PRIVILEGED;

You're confusing a page in the user part of the address space with a
non-privileged page.  Now, we would certainly expect that all pages in
the user part of the address space would be non-privileged pages and
all pages in the kernel part would be privileged pages, but nothing
actually enforces that.  The semantic change is that if we did somehow
happen to have a non-privileged page (one without _PAGE_PRIVILEGED
set) in the kernel part of the address space, we can no longer access
it from the kernel.  Now you can argue that we never have
non-privileged pages in the kernel part of the address space, and so
the change doesn't matter.  That is probably a good argument, but you
do need to mention the change and make that argument in your patch
description.

Paul.