Re: [uml-user] [PATCH] um: asm/page.h: zero out a pte's high value in set_pte_val()

[Richard W. <ri...@no...>]

Am 29.01.2016 um 15:31 schrieb Nicolai Stange:
>>> Question 1: now that ->pte_high will be gone, do you want to have
>>>             ->pte_low renamed to e.g. ->pte_val?
>>
>> So, with a freshly booted brain the story looks a bit different.
>> All this code needs a cleanup and we need to check what other archs do
>> before we change pte_val(). Are you ready for some research? :)
> 
> So this is what arch/x86 does:
> 
> 1.) typedef a pteval_t to a type matching the underlying hardware's
>     native PTE size.
>     Examples:
>     - x86: arch/x86/include/asm/pgtable-2level_types.h -- unsigned long
>     - x86(PAE): arch/x86/include/asm/pgtable-3level_types.h -- u64
>     - x86_64: arch/x86/include/asm/pgtable_64_types.h -- unsigned long
> 
> 2.) pte_t is typedefed to either a struct or union like this:
> 
>       typedef struct { pteval_t pte; } pte_t;
> 
>     In the case of a union (x86 and x86 w/ PAE), an additional member
>     'pte_low' is introduced, aliasing the low half of ->pte.
> 
> Now, all three x86-arch cases define typedef a pgprotval_t matching their
> respective pteval_t type and have a common then
> 
>   typedef struct pgprot { pgprotval_t pgprot; } pgprot_t;
> 
> Basically, mk_pte(page, pgprot) shifts the page's physical address to
> some architecturally defined point (PAGE_SHIFT) within pteval_t and ors
> the architecturally defined protection flags (_PAGE_*) in.
> 
> Of course, the protection flags are defined such that hardware
> eventually finds them at the expected place within the final PTE
> (c.f. arch/x86/include/asm/pgtable_types.h).
> 
> 
> Summarizing:
> The content of pteval_t is completely architecture dependent.  The only
> semantics on pte values defined for out-of-arch users, e.g. mm/gup.c
> seems to be equality on a pte_val(pte).
> 

Thanks for doing the research!

> Finally, the page protection flags defined for UML do not have any bit
> at a position greater than 9 assigned to them
> (c.g. arch/um/include/asm/pgtable.h). (If that had been the case, we had
> been in trouble already because protection flags are only or'ed into
> ->pte_low).
> 
> 
> Thus, under the assumption that with UML, physical addresses are always
> 32 bits, I would say that it is safe to change pte_t.
> 
> 
> Proposal:
> Introduce pteval_t and pgprotval_t like x86 does and do
> 
>   typedef struct { pteval_t pte; } pte_t;
>   typedef struct pgprot { pgprotval_t pgprot; } pgprot_t;
> 
> Change the pte macros accordingly.

Makes sense.

> What about pgd_t and pmd_t?

What do you mean? AFAICT we can keep them as-is.

So, please redo your patch such that we can merge it as soon as possible
to have the build warning fixed.

Thanks,
//richard