| Message ID | 20190724084638.24982-4-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4dd7554a6456d124c85e0a4ad156625b71390b5c |
| Headers | show |
| Series | [1/5] powerpc/64s/radix: Fix memory hotplug section page table creation | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (f3365d1a959d5c6527efe3d38276acc9b58e3f3f) |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 2 checks, 21 lines checked |
Nicholas Piggin <npiggin@gmail.com> writes: > Ensure __va is given a physical address below PAGE_OFFSET, and __pa is > given a virtual address above PAGE_OFFSET. > Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > arch/powerpc/include/asm/page.h | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h > index 0d52f57fca04..c8bb14ff4713 100644 > --- a/arch/powerpc/include/asm/page.h > +++ b/arch/powerpc/include/asm/page.h > @@ -215,9 +215,19 @@ static inline bool pfn_valid(unsigned long pfn) > /* > * gcc miscompiles (unsigned long)(&static_var) - PAGE_OFFSET > * with -mcmodel=medium, so we use & and | instead of - and + on 64-bit. > + * This also results in better code generation. > */ > -#define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET)) > -#define __pa(x) ((unsigned long)(x) & 0x0fffffffffffffffUL) > +#define __va(x) \ > +({ \ > + VIRTUAL_BUG_ON((unsigned long)(x) >= PAGE_OFFSET); \ > + (void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET); \ > +}) Can we make that static inline? Is there a need for __pa and __va to be #define like we do #ifndef anywhere? > + > +#define __pa(x) \ > +({ \ > + VIRTUAL_BUG_ON((unsigned long)(x) < PAGE_OFFSET); \ > + (unsigned long)(x) & 0x0fffffffffffffffUL; \ > +}) > > #else /* 32-bit, non book E */ > #define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) + PAGE_OFFSET - MEMORY_START)) > -- > 2.22.0
Le 24/07/2019 à 10:46, Nicholas Piggin a écrit : > Ensure __va is given a physical address below PAGE_OFFSET, and __pa is > given a virtual address above PAGE_OFFSET. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > arch/powerpc/include/asm/page.h | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h > index 0d52f57fca04..c8bb14ff4713 100644 > --- a/arch/powerpc/include/asm/page.h > +++ b/arch/powerpc/include/asm/page.h > @@ -215,9 +215,19 @@ static inline bool pfn_valid(unsigned long pfn) > /* > * gcc miscompiles (unsigned long)(&static_var) - PAGE_OFFSET > * with -mcmodel=medium, so we use & and | instead of - and + on 64-bit. > + * This also results in better code generation. > */ > -#define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET)) > -#define __pa(x) ((unsigned long)(x) & 0x0fffffffffffffffUL) > +#define __va(x) \ > +({ \ > + VIRTUAL_BUG_ON((unsigned long)(x) >= PAGE_OFFSET); \ Do we really want to add a BUG_ON here ? Can't we just add a WARN_ON, like in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/powerpc/include/asm/io.h?id=6bf752daca07c85c181159f75dcf65b12056883b ? > + (void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET); \ > +}) > + > +#define __pa(x) \ > +({ \ > + VIRTUAL_BUG_ON((unsigned long)(x) < PAGE_OFFSET); \ Same > + (unsigned long)(x) & 0x0fffffffffffffffUL; \ > +}) > > #else /* 32-bit, non book E */ > #define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) + PAGE_OFFSET - MEMORY_START)) > Would it be possible to change those macros into static inlines ? Christophe
Hi Nic, Le 24/07/2019 à 10:46, Nicholas Piggin a écrit : > Ensure __va is given a physical address below PAGE_OFFSET, and __pa is > given a virtual address above PAGE_OFFSET. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > arch/powerpc/include/asm/page.h | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h > index 0d52f57fca04..c8bb14ff4713 100644 > --- a/arch/powerpc/include/asm/page.h > +++ b/arch/powerpc/include/asm/page.h > @@ -215,9 +215,19 @@ static inline bool pfn_valid(unsigned long pfn) > /* > * gcc miscompiles (unsigned long)(&static_var) - PAGE_OFFSET > * with -mcmodel=medium, so we use & and | instead of - and + on 64-bit. > + * This also results in better code generation. > */ > -#define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET)) > -#define __pa(x) ((unsigned long)(x) & 0x0fffffffffffffffUL) > +#define __va(x) \ > +({ \ > + VIRTUAL_BUG_ON((unsigned long)(x) >= PAGE_OFFSET); \ > + (void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET); \ > +}) > + > +#define __pa(x) \ > +({ \ > + VIRTUAL_BUG_ON((unsigned long)(x) < PAGE_OFFSET); \ With this, it is likely that virt_addr_valid() BUGs on a non valid address. I think the purpose of virt_addr_valid() is to check addresses seamlessly, see check_heap_object() > + (unsigned long)(x) & 0x0fffffffffffffffUL; \ > +}) > > #else /* 32-bit, non book E */ > #define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) + PAGE_OFFSET - MEMORY_START))
Excerpts from Christophe Leroy's message of December 24, 2021 11:24 pm: > Hi Nic, > > Le 24/07/2019 à 10:46, Nicholas Piggin a écrit : >> Ensure __va is given a physical address below PAGE_OFFSET, and __pa is >> given a virtual address above PAGE_OFFSET. >> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- >> arch/powerpc/include/asm/page.h | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h >> index 0d52f57fca04..c8bb14ff4713 100644 >> --- a/arch/powerpc/include/asm/page.h >> +++ b/arch/powerpc/include/asm/page.h >> @@ -215,9 +215,19 @@ static inline bool pfn_valid(unsigned long pfn) >> /* >> * gcc miscompiles (unsigned long)(&static_var) - PAGE_OFFSET >> * with -mcmodel=medium, so we use & and | instead of - and + on 64-bit. >> + * This also results in better code generation. >> */ >> -#define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET)) >> -#define __pa(x) ((unsigned long)(x) & 0x0fffffffffffffffUL) >> +#define __va(x) \ >> +({ \ >> + VIRTUAL_BUG_ON((unsigned long)(x) >= PAGE_OFFSET); \ >> + (void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET); \ >> +}) >> + >> +#define __pa(x) \ >> +({ \ >> + VIRTUAL_BUG_ON((unsigned long)(x) < PAGE_OFFSET); \ > > With this, it is likely that virt_addr_valid() BUGs on a non valid address. > > I think the purpose of virt_addr_valid() is to check addresses > seamlessly, see check_heap_object() Looks like you're right. How did you catch that? We could change virt_addr_valid() to make that test first. x86 and arm64 both do checking rather than relying on !pfn_valid for < PAGE_OFFSET addresses. Thanks, Nick
Le 25/12/2021 à 11:10, Nicholas Piggin a écrit : > Excerpts from Christophe Leroy's message of December 24, 2021 11:24 pm: >> Hi Nic, >> >> Le 24/07/2019 à 10:46, Nicholas Piggin a écrit : >>> Ensure __va is given a physical address below PAGE_OFFSET, and __pa is >>> given a virtual address above PAGE_OFFSET. >>> >>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >>> --- >>> arch/powerpc/include/asm/page.h | 14 ++++++++++++-- >>> 1 file changed, 12 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h >>> index 0d52f57fca04..c8bb14ff4713 100644 >>> --- a/arch/powerpc/include/asm/page.h >>> +++ b/arch/powerpc/include/asm/page.h >>> @@ -215,9 +215,19 @@ static inline bool pfn_valid(unsigned long pfn) >>> /* >>> * gcc miscompiles (unsigned long)(&static_var) - PAGE_OFFSET >>> * with -mcmodel=medium, so we use & and | instead of - and + on 64-bit. >>> + * This also results in better code generation. >>> */ >>> -#define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET)) >>> -#define __pa(x) ((unsigned long)(x) & 0x0fffffffffffffffUL) >>> +#define __va(x) \ >>> +({ \ >>> + VIRTUAL_BUG_ON((unsigned long)(x) >= PAGE_OFFSET); \ >>> + (void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET); \ >>> +}) >>> + >>> +#define __pa(x) \ >>> +({ \ >>> + VIRTUAL_BUG_ON((unsigned long)(x) < PAGE_OFFSET); \ >> >> With this, it is likely that virt_addr_valid() BUGs on a non valid address. >> >> I think the purpose of virt_addr_valid() is to check addresses >> seamlessly, see check_heap_object() > > Looks like you're right. How did you catch that? I caught that while looking at the problem reported by Kefeng where he says that virt_addr_valid() reports true on vmalloced memory on book3e/64 > > We could change virt_addr_valid() to make that test first. x86 and arm64 > both do checking rather than relying on !pfn_valid for < PAGE_OFFSET > addresses. That should work. Maybe also we should implement a __pa_nodebug() like x86 and arm64 ? Christophe
diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h index 0d52f57fca04..c8bb14ff4713 100644 --- a/arch/powerpc/include/asm/page.h +++ b/arch/powerpc/include/asm/page.h @@ -215,9 +215,19 @@ static inline bool pfn_valid(unsigned long pfn) /* * gcc miscompiles (unsigned long)(&static_var) - PAGE_OFFSET * with -mcmodel=medium, so we use & and | instead of - and + on 64-bit. + * This also results in better code generation. */ -#define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET)) -#define __pa(x) ((unsigned long)(x) & 0x0fffffffffffffffUL) +#define __va(x) \ +({ \ + VIRTUAL_BUG_ON((unsigned long)(x) >= PAGE_OFFSET); \ + (void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET); \ +}) + +#define __pa(x) \ +({ \ + VIRTUAL_BUG_ON((unsigned long)(x) < PAGE_OFFSET); \ + (unsigned long)(x) & 0x0fffffffffffffffUL; \ +}) #else /* 32-bit, non book E */ #define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) + PAGE_OFFSET - MEMORY_START))
Ensure __va is given a physical address below PAGE_OFFSET, and __pa is given a virtual address above PAGE_OFFSET. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- arch/powerpc/include/asm/page.h | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)