| Message ID | 20220406145802.538416-5-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/6] powerpc: Fix virt_addr_valid() for 64-bit Book3E & 32-bit | expand |
Le 06/04/2022 à 16:58, Michael Ellerman a écrit : > We added checks to __pa() / __va() to ensure they're only called with > appropriate addresses. But using BUG_ON() is too strong, it means > virt_addr_valid() will BUG when DEBUG_VIRTUAL is enabled. > > Instead switch them to warnings, arm64 does the same. > > Fixes: 4dd7554a6456 ("powerpc/64: Add VIRTUAL_BUG_ON checks for __va and __pa addresses") > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > --- > arch/powerpc/include/asm/page.h | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h > index f2c5c26869f1..40a27a56ee40 100644 > --- a/arch/powerpc/include/asm/page.h > +++ b/arch/powerpc/include/asm/page.h > @@ -216,6 +216,12 @@ static inline bool pfn_valid(unsigned long pfn) > #define __pa(x) ((phys_addr_t)(unsigned long)(x) - VIRT_PHYS_OFFSET) > #else > #ifdef CONFIG_PPC64 > + > +#ifdef CONFIG_DEBUG_VIRTUAL > +#define VIRTUAL_WARN_ON(x) WARN_ON(x) > +#else > +#define VIRTUAL_WARN_ON(x) > +#endif Could be: #define VIRTUAL_WARN_ON(x) WARN_ON(IS_ENABLED(CONFIG_DEBUG_VIRTUAL) && (x)) > /* > * gcc miscompiles (unsigned long)(&static_var) - PAGE_OFFSET > * with -mcmodel=medium, so we use & and | instead of - and + on 64-bit. > @@ -223,13 +229,13 @@ static inline bool pfn_valid(unsigned long pfn) > */ > #define __va(x) \ > ({ \ > - VIRTUAL_BUG_ON((unsigned long)(x) >= PAGE_OFFSET); \ > + VIRTUAL_WARN_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); \ > + VIRTUAL_WARN_ON((unsigned long)(x) < PAGE_OFFSET); \ > (unsigned long)(x) & 0x0fffffffffffffffUL; \ > }) > Isn't it dangerous to WARN (or BUG) here ? __pa() can be used very early during boot, like in prom_init.c Some other architectures have a __pa_nodebug(). The __pa() does the WARN() then calls __pa_nodebug(). Early users call __pa_nodebug() directly. Christophe
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Le 06/04/2022 à 16:58, Michael Ellerman a écrit : >> We added checks to __pa() / __va() to ensure they're only called with >> appropriate addresses. But using BUG_ON() is too strong, it means >> virt_addr_valid() will BUG when DEBUG_VIRTUAL is enabled. >> >> Instead switch them to warnings, arm64 does the same. >> >> Fixes: 4dd7554a6456 ("powerpc/64: Add VIRTUAL_BUG_ON checks for __va and __pa addresses") >> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> >> --- >> arch/powerpc/include/asm/page.h | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h >> index f2c5c26869f1..40a27a56ee40 100644 >> --- a/arch/powerpc/include/asm/page.h >> +++ b/arch/powerpc/include/asm/page.h >> @@ -216,6 +216,12 @@ static inline bool pfn_valid(unsigned long pfn) >> #define __pa(x) ((phys_addr_t)(unsigned long)(x) - VIRT_PHYS_OFFSET) >> #else >> #ifdef CONFIG_PPC64 >> + >> +#ifdef CONFIG_DEBUG_VIRTUAL >> +#define VIRTUAL_WARN_ON(x) WARN_ON(x) >> +#else >> +#define VIRTUAL_WARN_ON(x) >> +#endif > > Could be: > > #define VIRTUAL_WARN_ON(x) WARN_ON(IS_ENABLED(CONFIG_DEBUG_VIRTUAL) && (x)) > >> /* >> * gcc miscompiles (unsigned long)(&static_var) - PAGE_OFFSET >> * with -mcmodel=medium, so we use & and | instead of - and + on 64-bit. >> @@ -223,13 +229,13 @@ static inline bool pfn_valid(unsigned long pfn) >> */ >> #define __va(x) \ >> ({ \ >> - VIRTUAL_BUG_ON((unsigned long)(x) >= PAGE_OFFSET); \ >> + VIRTUAL_WARN_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); \ >> + VIRTUAL_WARN_ON((unsigned long)(x) < PAGE_OFFSET); \ >> (unsigned long)(x) & 0x0fffffffffffffffUL; \ >> }) >> > > Isn't it dangerous to WARN (or BUG) here ? __pa() can be used very early > during boot, like in prom_init.c Yes. WARN is a bit less dangerous though :) > Some other architectures have a __pa_nodebug(). The __pa() does the > WARN() then calls __pa_nodebug(). Early users call __pa_nodebug() directly. Yeah I saw that, we could go that way. I think possibly the better option is for __pa() to have no checks, instead the checks go in the higher level routines like virt_to_phys() and phys_to_virt(). And then we can check uses of __pa() and any that are *not* early boot or low level stuff can be converted to virt_to_phys(). cheers
diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h index f2c5c26869f1..40a27a56ee40 100644 --- a/arch/powerpc/include/asm/page.h +++ b/arch/powerpc/include/asm/page.h @@ -216,6 +216,12 @@ static inline bool pfn_valid(unsigned long pfn) #define __pa(x) ((phys_addr_t)(unsigned long)(x) - VIRT_PHYS_OFFSET) #else #ifdef CONFIG_PPC64 + +#ifdef CONFIG_DEBUG_VIRTUAL +#define VIRTUAL_WARN_ON(x) WARN_ON(x) +#else +#define VIRTUAL_WARN_ON(x) +#endif /* * gcc miscompiles (unsigned long)(&static_var) - PAGE_OFFSET * with -mcmodel=medium, so we use & and | instead of - and + on 64-bit. @@ -223,13 +229,13 @@ static inline bool pfn_valid(unsigned long pfn) */ #define __va(x) \ ({ \ - VIRTUAL_BUG_ON((unsigned long)(x) >= PAGE_OFFSET); \ + VIRTUAL_WARN_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); \ + VIRTUAL_WARN_ON((unsigned long)(x) < PAGE_OFFSET); \ (unsigned long)(x) & 0x0fffffffffffffffUL; \ })
We added checks to __pa() / __va() to ensure they're only called with appropriate addresses. But using BUG_ON() is too strong, it means virt_addr_valid() will BUG when DEBUG_VIRTUAL is enabled. Instead switch them to warnings, arm64 does the same. Fixes: 4dd7554a6456 ("powerpc/64: Add VIRTUAL_BUG_ON checks for __va and __pa addresses") Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- arch/powerpc/include/asm/page.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)