| Message ID | 1343775898-28345-4-git-send-email-cyril@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 31 Jul 2012, Cyril Chemparathy wrote: > This patch fixes up the types used when converting back and forth between > physical and virtual addresses. > > Signed-off-by: Vitaly Andrianov <vitalya@ti.com> > Signed-off-by: Cyril Chemparathy <cyril@ti.com> Did you verify that this didn't introduce any compilation warning when compiling for non LPAE? If so and there were no warnings then... Acked-by: Nicolas Pitre <nico@linaro.org> > --- > arch/arm/include/asm/memory.h | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h > index 01c710d..4a0108f 100644 > --- a/arch/arm/include/asm/memory.h > +++ b/arch/arm/include/asm/memory.h > @@ -157,22 +157,27 @@ extern unsigned long __pv_phys_offset; > > extern unsigned long __pv_offset; > > -static inline unsigned long __virt_to_phys(unsigned long x) > +static inline phys_addr_t __virt_to_phys(unsigned long x) > { > unsigned long t; > early_patch_imm8(x, t, "add", __pv_offset); > return t; > } > > -static inline unsigned long __phys_to_virt(unsigned long x) > +static inline unsigned long __phys_to_virt(phys_addr_t x) > { > unsigned long t; > early_patch_imm8(x, t, "sub", __pv_offset); > return t; > } > #else > -#define __virt_to_phys(x) ((x) - PAGE_OFFSET + PHYS_OFFSET) > -#define __phys_to_virt(x) ((x) - PHYS_OFFSET + PAGE_OFFSET) > + > +#define __virt_to_phys(x) \ > + ((phys_addr_t)(x) - PAGE_OFFSET + PHYS_OFFSET) > + > +#define __phys_to_virt(x) \ > + ((unsigned long)((phys_addr_t)(x) - PHYS_OFFSET + PAGE_OFFSET)) > + > #endif > #endif > > @@ -207,14 +212,14 @@ static inline phys_addr_t virt_to_phys(const volatile void *x) > > static inline void *phys_to_virt(phys_addr_t x) > { > - return (void *)(__phys_to_virt((unsigned long)(x))); > + return (void *)__phys_to_virt(x); > } > > /* > * Drivers should NOT use these either. > */ > #define __pa(x) __virt_to_phys((unsigned long)(x)) > -#define __va(x) ((void *)__phys_to_virt((unsigned long)(x))) > +#define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x))) > #define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT) > > /* > -- > 1.7.9.5 >
On 8/4/2012 2:24 AM, Nicolas Pitre wrote: > On Tue, 31 Jul 2012, Cyril Chemparathy wrote: > >> This patch fixes up the types used when converting back and forth between >> physical and virtual addresses. >> >> Signed-off-by: Vitaly Andrianov <vitalya@ti.com> >> Signed-off-by: Cyril Chemparathy <cyril@ti.com> > > Did you verify that this didn't introduce any compilation warning when > compiling for non LPAE? If so and there were no warnings then... > Yes. This series has been tested on vanilla ARMv7 Cortex-A8 non-LPAE hardware as well. > Acked-by: Nicolas Pitre <nico@linaro.org> > > >> --- >> arch/arm/include/asm/memory.h | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h >> index 01c710d..4a0108f 100644 >> --- a/arch/arm/include/asm/memory.h >> +++ b/arch/arm/include/asm/memory.h >> @@ -157,22 +157,27 @@ extern unsigned long __pv_phys_offset; >> >> extern unsigned long __pv_offset; >> >> -static inline unsigned long __virt_to_phys(unsigned long x) >> +static inline phys_addr_t __virt_to_phys(unsigned long x) >> { >> unsigned long t; >> early_patch_imm8(x, t, "add", __pv_offset); >> return t; >> } >> >> -static inline unsigned long __phys_to_virt(unsigned long x) >> +static inline unsigned long __phys_to_virt(phys_addr_t x) >> { >> unsigned long t; >> early_patch_imm8(x, t, "sub", __pv_offset); >> return t; >> } >> #else >> -#define __virt_to_phys(x) ((x) - PAGE_OFFSET + PHYS_OFFSET) >> -#define __phys_to_virt(x) ((x) - PHYS_OFFSET + PAGE_OFFSET) >> + >> +#define __virt_to_phys(x) \ >> + ((phys_addr_t)(x) - PAGE_OFFSET + PHYS_OFFSET) >> + >> +#define __phys_to_virt(x) \ >> + ((unsigned long)((phys_addr_t)(x) - PHYS_OFFSET + PAGE_OFFSET)) >> + >> #endif >> #endif >> >> @@ -207,14 +212,14 @@ static inline phys_addr_t virt_to_phys(const volatile void *x) >> >> static inline void *phys_to_virt(phys_addr_t x) >> { >> - return (void *)(__phys_to_virt((unsigned long)(x))); >> + return (void *)__phys_to_virt(x); >> } >> >> /* >> * Drivers should NOT use these either. >> */ >> #define __pa(x) __virt_to_phys((unsigned long)(x)) >> -#define __va(x) ((void *)__phys_to_virt((unsigned long)(x))) >> +#define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x))) >> #define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT) >> >> /* >> -- >> 1.7.9.5 >>
On Tue, Jul 31, 2012 at 07:04:39PM -0400, Cyril Chemparathy wrote: > This patch fixes up the types used when converting back and forth between > physical and virtual addresses. > > Signed-off-by: Vitaly Andrianov <vitalya@ti.com> > Signed-off-by: Cyril Chemparathy <cyril@ti.com> > --- > arch/arm/include/asm/memory.h | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h > index 01c710d..4a0108f 100644 > --- a/arch/arm/include/asm/memory.h > +++ b/arch/arm/include/asm/memory.h > @@ -157,22 +157,27 @@ extern unsigned long __pv_phys_offset; > > extern unsigned long __pv_offset; > > -static inline unsigned long __virt_to_phys(unsigned long x) > +static inline phys_addr_t __virt_to_phys(unsigned long x) > { > unsigned long t; > early_patch_imm8(x, t, "add", __pv_offset); > return t; > } > > -static inline unsigned long __phys_to_virt(unsigned long x) > +static inline unsigned long __phys_to_virt(phys_addr_t x) > { > unsigned long t; > early_patch_imm8(x, t, "sub", __pv_offset); > return t; > } > #else > -#define __virt_to_phys(x) ((x) - PAGE_OFFSET + PHYS_OFFSET) > -#define __phys_to_virt(x) ((x) - PHYS_OFFSET + PAGE_OFFSET) > + > +#define __virt_to_phys(x) \ > + ((phys_addr_t)(x) - PAGE_OFFSET + PHYS_OFFSET) > + > +#define __phys_to_virt(x) \ > + ((unsigned long)((phys_addr_t)(x) - PHYS_OFFSET + PAGE_OFFSET)) > + > #endif > #endif > > @@ -207,14 +212,14 @@ static inline phys_addr_t virt_to_phys(const volatile void *x) > > static inline void *phys_to_virt(phys_addr_t x) > { > - return (void *)(__phys_to_virt((unsigned long)(x))); > + return (void *)__phys_to_virt(x); > } > > /* > * Drivers should NOT use these either. > */ > #define __pa(x) __virt_to_phys((unsigned long)(x)) > -#define __va(x) ((void *)__phys_to_virt((unsigned long)(x))) > +#define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x))) > #define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT) This as a whole does not fill me with a great amount of enthusiasm, because it breaks some of the typechecking that we have here. The whole point of __phys_to_virt() and __virt_to_phys() is that they work on integer types, and warn if you dare to use them with pointers. Adding a cast into them breaks that. The whole point is that the typecasting is explicitly inside phys_to_virt() and virt_to_phys() and not their macro counterparts. Secondly, are you sure that this patch is correct on its own? You're passing a u64 into assembly only expecting a 32-bit register. Have you checked it does the right thing with a 64-bit phys_addr_t on both LE and BE?
On 8/6/2012 7:14 AM, Russell King - ARM Linux wrote: > On Tue, Jul 31, 2012 at 07:04:39PM -0400, Cyril Chemparathy wrote: >> This patch fixes up the types used when converting back and forth between >> physical and virtual addresses. >> >> Signed-off-by: Vitaly Andrianov <vitalya@ti.com> >> Signed-off-by: Cyril Chemparathy <cyril@ti.com> >> --- >> arch/arm/include/asm/memory.h | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h >> index 01c710d..4a0108f 100644 >> --- a/arch/arm/include/asm/memory.h >> +++ b/arch/arm/include/asm/memory.h >> @@ -157,22 +157,27 @@ extern unsigned long __pv_phys_offset; >> >> extern unsigned long __pv_offset; >> >> -static inline unsigned long __virt_to_phys(unsigned long x) >> +static inline phys_addr_t __virt_to_phys(unsigned long x) >> { >> unsigned long t; >> early_patch_imm8(x, t, "add", __pv_offset); >> return t; >> } >> >> -static inline unsigned long __phys_to_virt(unsigned long x) >> +static inline unsigned long __phys_to_virt(phys_addr_t x) >> { >> unsigned long t; >> early_patch_imm8(x, t, "sub", __pv_offset); >> return t; >> } >> #else >> -#define __virt_to_phys(x) ((x) - PAGE_OFFSET + PHYS_OFFSET) >> -#define __phys_to_virt(x) ((x) - PHYS_OFFSET + PAGE_OFFSET) >> + >> +#define __virt_to_phys(x) \ >> + ((phys_addr_t)(x) - PAGE_OFFSET + PHYS_OFFSET) >> + >> +#define __phys_to_virt(x) \ >> + ((unsigned long)((phys_addr_t)(x) - PHYS_OFFSET + PAGE_OFFSET)) >> + >> #endif >> #endif >> >> @@ -207,14 +212,14 @@ static inline phys_addr_t virt_to_phys(const volatile void *x) >> >> static inline void *phys_to_virt(phys_addr_t x) >> { >> - return (void *)(__phys_to_virt((unsigned long)(x))); >> + return (void *)__phys_to_virt(x); >> } >> >> /* >> * Drivers should NOT use these either. >> */ >> #define __pa(x) __virt_to_phys((unsigned long)(x)) >> -#define __va(x) ((void *)__phys_to_virt((unsigned long)(x))) >> +#define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x))) >> #define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT) > > This as a whole does not fill me with a great amount of enthusiasm, > because it breaks some of the typechecking that we have here. > > The whole point of __phys_to_virt() and __virt_to_phys() is that they work > on integer types, and warn if you dare to use them with pointers. Adding > a cast into them breaks that. > Understood. Thanks. The casts were needed to upgrade to 64-bit before arithmetic. We should convert the non-patch __phys_to_virt and __virt_to_phys to inlines to keep the typechecking intact. > The whole point is that the typecasting is explicitly inside phys_to_virt() > and virt_to_phys() and not their macro counterparts. > > Secondly, are you sure that this patch is correct on its own? You're > passing a u64 into assembly only expecting a 32-bit register. Have you > checked it does the right thing with a 64-bit phys_addr_t on both LE > and BE? > We should explicitly pass in the lower order bits here, at least until the next patch in the series fixes things up for 64-bit. Thanks. We've tested with 64-bit and 32-bit phys_addr_t, but only on LE. Thanks for pointing this out, we'll figure out a way to run BE as well.
Hi Russell, On 8/6/2012 7:14 AM, Russell King - ARM Linux wrote: > On Tue, Jul 31, 2012 at 07:04:39PM -0400, Cyril Chemparathy wrote: >> This patch fixes up the types used when converting back and forth between >> physical and virtual addresses. >> >> Signed-off-by: Vitaly Andrianov <vitalya@ti.com> >> Signed-off-by: Cyril Chemparathy <cyril@ti.com> >> --- >> arch/arm/include/asm/memory.h | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h >> index 01c710d..4a0108f 100644 >> --- a/arch/arm/include/asm/memory.h >> +++ b/arch/arm/include/asm/memory.h >> @@ -157,22 +157,27 @@ extern unsigned long __pv_phys_offset; >> >> extern unsigned long __pv_offset; >> >> -static inline unsigned long __virt_to_phys(unsigned long x) >> +static inline phys_addr_t __virt_to_phys(unsigned long x) >> { >> unsigned long t; >> early_patch_imm8(x, t, "add", __pv_offset); >> return t; >> } >> >> -static inline unsigned long __phys_to_virt(unsigned long x) >> +static inline unsigned long __phys_to_virt(phys_addr_t x) >> { >> unsigned long t; >> early_patch_imm8(x, t, "sub", __pv_offset); >> return t; >> } >> #else >> -#define __virt_to_phys(x) ((x) - PAGE_OFFSET + PHYS_OFFSET) >> -#define __phys_to_virt(x) ((x) - PHYS_OFFSET + PAGE_OFFSET) >> + >> +#define __virt_to_phys(x) \ >> + ((phys_addr_t)(x) - PAGE_OFFSET + PHYS_OFFSET) >> + >> +#define __phys_to_virt(x) \ >> + ((unsigned long)((phys_addr_t)(x) - PHYS_OFFSET + PAGE_OFFSET)) >> + >> #endif >> #endif >> >> @@ -207,14 +212,14 @@ static inline phys_addr_t virt_to_phys(const volatile void *x) >> >> static inline void *phys_to_virt(phys_addr_t x) >> { >> - return (void *)(__phys_to_virt((unsigned long)(x))); >> + return (void *)__phys_to_virt(x); >> } >> >> /* >> * Drivers should NOT use these either. >> */ >> #define __pa(x) __virt_to_phys((unsigned long)(x)) >> -#define __va(x) ((void *)__phys_to_virt((unsigned long)(x))) >> +#define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x))) >> #define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT) > > This as a whole does not fill me with a great amount of enthusiasm, > because it breaks some of the typechecking that we have here. > > The whole point of __phys_to_virt() and __virt_to_phys() is that they work > on integer types, and warn if you dare to use them with pointers. Adding > a cast into them breaks that. > > The whole point is that the typecasting is explicitly inside phys_to_virt() > and virt_to_phys() and not their macro counterparts. > The casts in __phys_to_virt() and __virt_to_phys() were necessary to widen integer types in case of LPAE without phys/virt patching. I assume that this specifically is the typecasting that you are concerned about. Would it be better then to convert these to inlines then? That way we could get the typechecking, with proper widening as needed.
diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h index 01c710d..4a0108f 100644 --- a/arch/arm/include/asm/memory.h +++ b/arch/arm/include/asm/memory.h @@ -157,22 +157,27 @@ extern unsigned long __pv_phys_offset; extern unsigned long __pv_offset; -static inline unsigned long __virt_to_phys(unsigned long x) +static inline phys_addr_t __virt_to_phys(unsigned long x) { unsigned long t; early_patch_imm8(x, t, "add", __pv_offset); return t; } -static inline unsigned long __phys_to_virt(unsigned long x) +static inline unsigned long __phys_to_virt(phys_addr_t x) { unsigned long t; early_patch_imm8(x, t, "sub", __pv_offset); return t; } #else -#define __virt_to_phys(x) ((x) - PAGE_OFFSET + PHYS_OFFSET) -#define __phys_to_virt(x) ((x) - PHYS_OFFSET + PAGE_OFFSET) + +#define __virt_to_phys(x) \ + ((phys_addr_t)(x) - PAGE_OFFSET + PHYS_OFFSET) + +#define __phys_to_virt(x) \ + ((unsigned long)((phys_addr_t)(x) - PHYS_OFFSET + PAGE_OFFSET)) + #endif #endif @@ -207,14 +212,14 @@ static inline phys_addr_t virt_to_phys(const volatile void *x) static inline void *phys_to_virt(phys_addr_t x) { - return (void *)(__phys_to_virt((unsigned long)(x))); + return (void *)__phys_to_virt(x); } /* * Drivers should NOT use these either. */ #define __pa(x) __virt_to_phys((unsigned long)(x)) -#define __va(x) ((void *)__phys_to_virt((unsigned long)(x))) +#define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x))) #define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT) /*