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

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

[Richard Weinberger]

Am 29.01.2016 um 00:56 schrieb Nicolai Stange:
> Commit 16da306849d0 ("um: kill pfn_t")
> introduced a compile warning for defconfig:
> 
>   arch/um/kernel/skas/mmu.c:38:206: warning: right shift count >= width of type
>                                     [-Wshift-count-overflow]
> 
> Aforementioned patch changes the definition of the phys_to_pfn() macro from
> 
>   ((pfn_t) ((p) >> PAGE_SHIFT))
> 
> to
> 
>   ((p) >> PAGE_SHIFT)
> 
> This effectively changes the phys_to_pfn() expansion's type from
> unsigned long long to unsigned long.
> 
> Through the callchain init_stub_pte()->mk_pte(), the expansion of
> phys_to_pfn() is (indirectly) fed into the 'phys' argument of the
> pte_set_val(pte, phys, prot) macro, eventually leading to
> 
>   (pte).pte_high = (phys) >> 32;
> 
> This results in the warning from above.
> 
> Since UML only deals with 32 bit addresses, the upper 32 bits from 'phys'
> used to be zero anyway.
> 
> Zero out the pte value's high part in pte_set_val() in order to get rid
> of the offending shift.
> 
> Fixes: 16da306849d0 ("um: kill pfn_t")
> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
> ---
>  arch/um/include/asm/page.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/um/include/asm/page.h b/arch/um/include/asm/page.h
> index e13d41c..61e235f 100644
> --- a/arch/um/include/asm/page.h
> +++ b/arch/um/include/asm/page.h
> @@ -46,8 +46,8 @@ typedef struct { unsigned long pgd; } pgd_t;
>  			      smp_wmb(); \
>  			      (to).pte_low = (from).pte_low; })
>  #define pte_is_zero(pte) (!((pte).pte_low & ~_PAGE_NEWPAGE) && !(pte).pte_high)
> -#define pte_set_val(pte, phys, prot) \
> -	({ (pte).pte_high = (phys) >> 32; \
> +#define pte_set_val(pte, phys, prot)				\
> +	({ (pte).pte_high = 0;					\
>  	   (pte).pte_low = (phys) | pgprot_val(prot); })

I think we can completely kill .pte_high.

Thanks,
//richard

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

[Nicolai Stange]

Richard Weinberger <richard@nod.at> writes:

> Am 29.01.2016 um 00:56 schrieb Nicolai Stange:
>> Commit 16da306849d0 ("um: kill pfn_t")
>> introduced a compile warning for defconfig:
>> 
>>   arch/um/kernel/skas/mmu.c:38:206: warning: right shift count >= width of type
>>                                     [-Wshift-count-overflow]
>> 
>> Aforementioned patch changes the definition of the phys_to_pfn() macro from
>> 
>>   ((pfn_t) ((p) >> PAGE_SHIFT))
>> 
>> to
>> 
>>   ((p) >> PAGE_SHIFT)
>> 
>> This effectively changes the phys_to_pfn() expansion's type from
>> unsigned long long to unsigned long.
>> 
>> Through the callchain init_stub_pte()->mk_pte(), the expansion of
>> phys_to_pfn() is (indirectly) fed into the 'phys' argument of the
>> pte_set_val(pte, phys, prot) macro, eventually leading to
>> 
>>   (pte).pte_high = (phys) >> 32;
>> 
>> This results in the warning from above.
>> 
>> Since UML only deals with 32 bit addresses, the upper 32 bits from 'phys'
>> used to be zero anyway.
>> 
>> Zero out the pte value's high part in pte_set_val() in order to get rid
>> of the offending shift.
>> 
>> Fixes: 16da306849d0 ("um: kill pfn_t")
>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>> ---
>>  arch/um/include/asm/page.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/um/include/asm/page.h b/arch/um/include/asm/page.h
>> index e13d41c..61e235f 100644
>> --- a/arch/um/include/asm/page.h
>> +++ b/arch/um/include/asm/page.h
>> @@ -46,8 +46,8 @@ typedef struct { unsigned long pgd; } pgd_t;
>>  			      smp_wmb(); \
>>  			      (to).pte_low = (from).pte_low; })
>>  #define pte_is_zero(pte) (!((pte).pte_low & ~_PAGE_NEWPAGE) && !(pte).pte_high)
>> -#define pte_set_val(pte, phys, prot) \
>> -	({ (pte).pte_high = (phys) >> 32; \
>> +#define pte_set_val(pte, phys, prot)				\
>> +	({ (pte).pte_high = 0;					\
>>  	   (pte).pte_low = (phys) | pgprot_val(prot); })
>
> I think we can completely kill .pte_high.
>

I did a quick test with ->pte_high purged and this doesn't introduce any
new warnings.

Booting w/o a rootfs works up to mount_root.


Note that an implication of getting rid of ->pte_high would be that the
type of pte_val() would get changed from unsigned long long to unsigned
long. However, outside of arch/um, pte_val() is only used here:
- drivers/gpu/drm/drm_vm.c
- include/trace/events/xen.h
- mm/gup.c
- mm/memory.c
All these uses and the ones in arch/um itself look compatible with this
change (if relevant at all for UML).


I'll post a follow up patch for this tomorrow.

Question 1: now that ->pte_high will be gone, do you want to have
            ->pte_low renamed to e.g. ->pte_val?

Question 2: what is the smp_wmb() in pte_copy() paired with/good for?


Thank you!

Nicolai

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

[Richard Weinberger]

Am 29.01.2016 um 02:32 schrieb Nicolai Stange:
> Richard Weinberger <richard@nod.at> writes:
> 
>> Am 29.01.2016 um 00:56 schrieb Nicolai Stange:
>>> Commit 16da306849d0 ("um: kill pfn_t")
>>> introduced a compile warning for defconfig:
>>>
>>>   arch/um/kernel/skas/mmu.c:38:206: warning: right shift count >= width of type
>>>                                     [-Wshift-count-overflow]
>>>
>>> Aforementioned patch changes the definition of the phys_to_pfn() macro from
>>>
>>>   ((pfn_t) ((p) >> PAGE_SHIFT))
>>>
>>> to
>>>
>>>   ((p) >> PAGE_SHIFT)
>>>
>>> This effectively changes the phys_to_pfn() expansion's type from
>>> unsigned long long to unsigned long.
>>>
>>> Through the callchain init_stub_pte()->mk_pte(), the expansion of
>>> phys_to_pfn() is (indirectly) fed into the 'phys' argument of the
>>> pte_set_val(pte, phys, prot) macro, eventually leading to
>>>
>>>   (pte).pte_high = (phys) >> 32;
>>>
>>> This results in the warning from above.
>>>
>>> Since UML only deals with 32 bit addresses, the upper 32 bits from 'phys'
>>> used to be zero anyway.
>>>
>>> Zero out the pte value's high part in pte_set_val() in order to get rid
>>> of the offending shift.
>>>
>>> Fixes: 16da306849d0 ("um: kill pfn_t")
>>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>>> ---
>>>  arch/um/include/asm/page.h | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/um/include/asm/page.h b/arch/um/include/asm/page.h
>>> index e13d41c..61e235f 100644
>>> --- a/arch/um/include/asm/page.h
>>> +++ b/arch/um/include/asm/page.h
>>> @@ -46,8 +46,8 @@ typedef struct { unsigned long pgd; } pgd_t;
>>>  			      smp_wmb(); \
>>>  			      (to).pte_low = (from).pte_low; })
>>>  #define pte_is_zero(pte) (!((pte).pte_low & ~_PAGE_NEWPAGE) && !(pte).pte_high)
>>> -#define pte_set_val(pte, phys, prot) \
>>> -	({ (pte).pte_high = (phys) >> 32; \
>>> +#define pte_set_val(pte, phys, prot)				\
>>> +	({ (pte).pte_high = 0;					\
>>>  	   (pte).pte_low = (phys) | pgprot_val(prot); })
>>
>> I think we can completely kill .pte_high.
>>
> 
> I did a quick test with ->pte_high purged and this doesn't introduce any
> new warnings.
> 
> Booting w/o a rootfs works up to mount_root.
> 
> 
> Note that an implication of getting rid of ->pte_high would be that the
> type of pte_val() would get changed from unsigned long long to unsigned
> long. However, outside of arch/um, pte_val() is only used here:
> - drivers/gpu/drm/drm_vm.c
> - include/trace/events/xen.h
> - mm/gup.c
> - mm/memory.c
> All these uses and the ones in arch/um itself look compatible with this
> change (if relevant at all for UML).
> 
> 
> I'll post a follow up patch for this tomorrow.
> 
> 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? :)

> Question 2: what is the smp_wmb() in pte_copy() paired with/good for?

AFACT to make sure that a write to pte_high is complete before we write pte_low.
100% copy&pasted from arch/i386 15 years ago. ;-)

Thanks,
//richard

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

[Nicolai Stange]

Richard Weinberger <richard@nod.at> writes:

> Am 29.01.2016 um 02:32 schrieb Nicolai Stange:
>> Richard Weinberger <richard@nod.at> writes:
>> 
>>> Am 29.01.2016 um 00:56 schrieb Nicolai Stange:
>>>> Commit 16da306849d0 ("um: kill pfn_t")
>>>> introduced a compile warning for defconfig:
>>>>
>>>>   arch/um/kernel/skas/mmu.c:38:206: warning: right shift count >= width of type
>>>>                                     [-Wshift-count-overflow]
>>>>
>>>> Aforementioned patch changes the definition of the phys_to_pfn() macro from
>>>>
>>>>   ((pfn_t) ((p) >> PAGE_SHIFT))
>>>>
>>>> to
>>>>
>>>>   ((p) >> PAGE_SHIFT)
>>>>
>>>> This effectively changes the phys_to_pfn() expansion's type from
>>>> unsigned long long to unsigned long.
>>>>
>>>> Through the callchain init_stub_pte()->mk_pte(), the expansion of
>>>> phys_to_pfn() is (indirectly) fed into the 'phys' argument of the
>>>> pte_set_val(pte, phys, prot) macro, eventually leading to
>>>>
>>>>   (pte).pte_high = (phys) >> 32;
>>>>
>>>> This results in the warning from above.
>>>>
>>>> Since UML only deals with 32 bit addresses, the upper 32 bits from 'phys'
>>>> used to be zero anyway.
>>>>
>>>> Zero out the pte value's high part in pte_set_val() in order to get rid
>>>> of the offending shift.
>>>>
>>>> Fixes: 16da306849d0 ("um: kill pfn_t")
>>>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>>>> ---
>>>>  arch/um/include/asm/page.h | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/um/include/asm/page.h b/arch/um/include/asm/page.h
>>>> index e13d41c..61e235f 100644
>>>> --- a/arch/um/include/asm/page.h
>>>> +++ b/arch/um/include/asm/page.h
>>>> @@ -46,8 +46,8 @@ typedef struct { unsigned long pgd; } pgd_t;
>>>>  			      smp_wmb(); \
>>>>  			      (to).pte_low = (from).pte_low; })
>>>>  #define pte_is_zero(pte) (!((pte).pte_low & ~_PAGE_NEWPAGE) && !(pte).pte_high)
>>>> -#define pte_set_val(pte, phys, prot) \
>>>> -	({ (pte).pte_high = (phys) >> 32; \
>>>> +#define pte_set_val(pte, phys, prot)				\
>>>> +	({ (pte).pte_high = 0;					\
>>>>  	   (pte).pte_low = (phys) | pgprot_val(prot); })
>>>
>>> I think we can completely kill .pte_high.
>>>
>> 
>> I did a quick test with ->pte_high purged and this doesn't introduce any
>> new warnings.
>> 
>> Booting w/o a rootfs works up to mount_root.
>> 
>> 
>> Note that an implication of getting rid of ->pte_high would be that the
>> type of pte_val() would get changed from unsigned long long to unsigned
>> long. However, outside of arch/um, pte_val() is only used here:
>> - drivers/gpu/drm/drm_vm.c
>> - include/trace/events/xen.h
>> - mm/gup.c
>> - mm/memory.c
>> All these uses and the ones in arch/um itself look compatible with this
>> change (if relevant at all for UML).
>> 
>> 
>> I'll post a follow up patch for this tomorrow.
>> 
>> 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).


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.

What about pgd_t and pmd_t?


>> Question 2: what is the smp_wmb() in pte_copy() paired with/good for?
>
> AFACT to make sure that a write to pte_high is complete before we write pte_low.
> 100% copy&pasted from arch/i386 15 years ago. ;-)
>
> Thanks,
> //richard

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

[Richard Weinberger]

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

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

[PATCH v2] um: asm/page.h: remove the pte_high member from struct pte_t

[Nicolai Stange]

Commit 16da306849d0 ("um: kill pfn_t")
introduced a compile warning for defconfig (SUBARCH=i386):

  arch/um/kernel/skas/mmu.c:38:206:
      warning: right shift count >= width of type [-Wshift-count-overflow]

Aforementioned patch changes the definition of the phys_to_pfn() macro from

  ((pfn_t) ((p) >> PAGE_SHIFT))

to

  ((p) >> PAGE_SHIFT)

This effectively changes the phys_to_pfn() expansion's type from
unsigned long long to unsigned long.

Through the callchain init_stub_pte() => mk_pte(), the expansion of
phys_to_pfn() is (indirectly) fed into the 'phys' argument of the
pte_set_val(pte, phys, prot) macro, eventually leading to

  (pte).pte_high = (phys) >> 32;

This results in the warning from above.

Since UML only deals with 32 bit addresses, the upper 32 bits from 'phys'
used to be always zero anyway. Also, all page protection flags defined by
UML don't use any bits beyond bit 9. Since the contents of a PTE are
defined within architecture scope only, the ->pte_high member can be safely
removed.

Remove the ->pte_high member from struct pte_t.
Rename ->pte_low to ->pte.
Adapt the pte helper macros in arch/um/include/asm/page.h.

Noteworthy is the pte_copy() macro where a smp_wmb() gets dropped.
This write barrier doesn't seem to be paired with any read barrier
though and thus, was useless anyway.

Fixes: 16da306849d0 ("um: kill pfn_t")
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 v1 was "um: asm/page.h: zero out a pte's high value in set_pte_val()"

 Changes to v1: do not zero out ->pte_high in pte_set_val(), but remove
                ->pte_high alltogether.

 Although I said that I'm going to introduce a
   typedef unsigned long pteval_t;
 I did not do so in this patch because that would really have been two
 changes in one patch.
 What's more, introduction of pteval_t should probably come along with
 pgdval_t, pmdval_t and pgprotval_t also.

 arch/um/include/asm/page.h | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/arch/um/include/asm/page.h b/arch/um/include/asm/page.h
index e13d41c..f878bec 100644
--- a/arch/um/include/asm/page.h
+++ b/arch/um/include/asm/page.h
@@ -34,21 +34,18 @@ struct page;
 
 #if defined(CONFIG_3_LEVEL_PGTABLES) && !defined(CONFIG_64BIT)
 
-typedef struct { unsigned long pte_low, pte_high; } pte_t;
+typedef struct { unsigned long pte; } pte_t;
 typedef struct { unsigned long pmd; } pmd_t;
 typedef struct { unsigned long pgd; } pgd_t;
-#define pte_val(x) ((x).pte_low | ((unsigned long long) (x).pte_high << 32))
-
-#define pte_get_bits(pte, bits) ((pte).pte_low & (bits))
-#define pte_set_bits(pte, bits) ((pte).pte_low |= (bits))
-#define pte_clear_bits(pte, bits) ((pte).pte_low &= ~(bits))
-#define pte_copy(to, from) ({ (to).pte_high = (from).pte_high; \
-			      smp_wmb(); \
-			      (to).pte_low = (from).pte_low; })
-#define pte_is_zero(pte) (!((pte).pte_low & ~_PAGE_NEWPAGE) && !(pte).pte_high)
-#define pte_set_val(pte, phys, prot) \
-	({ (pte).pte_high = (phys) >> 32; \
-	   (pte).pte_low = (phys) | pgprot_val(prot); })
+#define pte_val(p) ((p).pte)
+
+#define pte_get_bits(p, bits) ((p).pte & (bits))
+#define pte_set_bits(p, bits) ((p).pte |= (bits))
+#define pte_clear_bits(p, bits) ((p).pte &= ~(bits))
+#define pte_copy(to, from) ({ (to).pte = (from).pte; })
+#define pte_is_zero(p) (!((p).pte & ~_PAGE_NEWPAGE))
+#define pte_set_val(p, phys, prot) \
+	({ (p).pte = (phys) | pgprot_val(prot); })
 
 #define pmd_val(x)	((x).pmd)
 #define __pmd(x) ((pmd_t) { (x) } )
-- 
2.7.0