lkml.org 
[lkml]   [2016]   [Nov]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] ARC: mm: PAE40: Cast pfn to pte_t in pfn_pte() macro
Date
Hi Yuriy,

Really nice catch!
Though a couple of nitpicks below.

On Mon, 2016-11-28 at 07:07 +0300, Yuriy Kolerov wrote:
> Originally pfn_pte(pfn, prot) macro had this definition:
>
>     __pte(((pfn) << PAGE_SHIFT) | pgprot_val(prot))
>
> The value of pfn (Page Frame Number) is shifted to the left to get the
> value of pte (Page Table Entry). Usually a 4-byte value is passed to
> this macro as value of pfn. However if Linux is configured with support
> of PAE40 then value of pte has 8-byte type because it must contain
> additional 8 bits of the physical address. Thus if value of pfn
> represents a physical page frame above of 4GB boundary then
> shifting of pfn to the left by PAGE_SHIFT wipes most significant
> bits of the 40-bit physical address.
>
> As a result all physical addresses above of 4GB boundary in systems
> with PAE40 are mapped to virtual address incorrectly. An error may
> occur when the kernel tries to unmap such bad pages:
>
>     [ECR   ]: 0x00050100 => Invalid Read @ 0x41414144 by insn @ 0x801644c6
>     [EFA   ]: 0x41414144
>     [BLINK ]: unmap_page_range+0x134/0x700
>     [ERET  ]: unmap_page_range+0x17a/0x700
>     [STAT32]: 0x8008021e : IE K
>     BTA: 0x801644c6  SP: 0x901a5e84  FP: 0x5ff35de8
>     LPS: 0x8026462c LPE: 0x80264630 LPC: 0x00000000
>     r00: 0x8fcc4fc0 r01: 0x2fe68000 r02: 0x41414140
>     r03: 0x2c05c000 r04: 0x2fe6a000 r05: 0x0009ffff
>     r06: 0x901b6898 r07: 0x2fe68000 r08: 0x00000001
>     r09: 0x804a807c r10: 0x0000067e r11: 0xffffffff
>     r12: 0x80164480
>     Stack Trace:
>       unmap_page_range+0x17a/0x700
>       unmap_vmas+0x46/0x64
>       do_munmap+0x210/0x450
>       SyS_munmap+0x2c/0x50
>       EV_Trap+0xfc/0x100

This example makes not much sense in its current form.
I'd like to see how mentioned above problem leads to this
failure. I.e. pfn = 0xXXX gave pte = 0xYYY and at truncated to 0xYYY
address there's no data we expected thus reading from 0x41414144
end up in exception etc.

> So the value of pfn must be casted to pte_t before shifting to
> ensure that 40-bit address will not be truncated:
>
>     __pte(((pte_t) (pfn) << PAGE_SHIFT) | pgprot_val(prot))
>
> Signed-off-by: Yuriy Kolerov <yuriy.kolerov@synopsys.com>
> ---
>  arch/arc/include/asm/pgtable.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arc/include/asm/pgtable.h b/arch/arc/include/asm/pgtable.h
> index 89eeb37..77bc51c 100644
> --- a/arch/arc/include/asm/pgtable.h
> +++ b/arch/arc/include/asm/pgtable.h
> @@ -280,7 +280,8 @@ static inline void pmd_set(pmd_t *pmdp, pte_t *ptep)
>  
>  #define pte_page(pte) pfn_to_page(pte_pfn(pte))
>  #define mk_pte(page, prot) pfn_pte(page_to_pfn(page), prot)
> -#define pfn_pte(pfn, prot) __pte(((pfn) << PAGE_SHIFT) | pgprot_val(prot))
> +#define pfn_pte(pfn, prot) \
> + __pte(((pte_t) (pfn) << PAGE_SHIFT) | pgprot_val(prot))

I think it's better to split it in a bit different manner like:
--------------------------------->8-----------------------------
#define pfn_pte(pfn, prot) __pte(((pte_t) (pfn) << PAGE_SHIFT) | \
      pgprot_val(prot))
--------------------------------->8-----------------------------

Also see how this macro is implemented for example on ARM:
http://lxr.free-electrons.com/source/arch/arm/include/asm/pgtable.h#L211
-------------------->8------------------
#define pfn_pte(pfn,prot)       __pte(__pfn_to_phys(pfn) | pgprot_val(prot))
-------------------->8------------------

Where __pfn_to_phys() is:
http://lxr.free-electrons.com/source/include/asm-generic/memory_model.h#L78
-------------------->8------------------
#define __pfn_to_phys(pfn)      PFN_PHYS(pfn)
-------------------->8------------------

PFN_PHYS() is:
http://lxr.free-electrons.com/source/include/linux/pfn.h#L20
-------------------->8------------------
#define PFN_PHYS(x)     ((phys_addr_t)(x) << PAGE_SHIFT)
-------------------->8------------------

And finally phys_addr_t is:
http://lxr.free-electrons.com/source/include/linux/types.h#L161
-------------------->8------------------
#ifdef CONFIG_PHYS_ADDR_T_64BIT
typedef u64 phys_addr_t;
#else
typedef u32 phys_addr_t;
#endif
-------------------->8------------------

Not really sure though which implementation is better.
I like your approach because its simplicity instead of another
couple of layers of definitions but maybe there's a reason for this
kind of complication. Funny enough other arches take their own approaches
ranging from the same you did to this __pfn_to_phys() to casting to "long long".

-Alexey
\
 
 \ /
  Last update: 2016-11-28 12:43    [W:0.045 / U:0.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site