| Message ID | 20220707215237.1730283-1-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 40c0f5016391b85ecfc7db01974978b5cd1968e8 |
| Headers | show |
| Series | MIPS: Fixed __debug_virt_addr_valid() | expand |
On Thu, Jul 07, 2022 at 02:52:36PM -0700, Florian Fainelli wrote: > It is permissible for kernel code to call virt_to_phys() against virtual > addresses that are in KSEG0 or KSEG1 and we need to be dealing with both > types. Add a final condition that ensures that the virtual address is > below KSEG2. > > Fixes: dfad83cb7193 ("MIPS: Add support for CONFIG_DEBUG_VIRTUAL") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > arch/mips/mm/physaddr.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/mips/mm/physaddr.c b/arch/mips/mm/physaddr.c > index a1ced5e44951..a82f8f57a652 100644 > --- a/arch/mips/mm/physaddr.c > +++ b/arch/mips/mm/physaddr.c > @@ -5,6 +5,7 @@ > #include <linux/mmdebug.h> > #include <linux/mm.h> > > +#include <asm/addrspace.h> > #include <asm/sections.h> > #include <asm/io.h> > #include <asm/page.h> > @@ -30,7 +31,7 @@ static inline bool __debug_virt_addr_valid(unsigned long x) > if (x == MAX_DMA_ADDRESS) > return true; > > - return false; > + return KSEGX(x) < KSEG2; With this do we really need the high_memory-based conditionals in this method? If the line above is the only way to take the uncached segment into account then can we reduce the whole method to: static inline bool __debug_virt_addr_valid { return x >= PAGE_OFFSET && KSEGX(x) < KSEG2; } ? Though this still may be invalid for EVA systems, like malta (see arch/mips/include/asm/mach-malta/spaces.h). Note AFAICS if EVA is enabled, highmem is implied to be disabled (see the CPU_MIPS32_3_5_EVA config utilization and HIGHMEM config dependencies). Thus all the memory is supposed to be linearly mapped in that case. -Sergey > } > > phys_addr_t __virt_to_phys(volatile const void *x) > -- > 2.25.1 >
On Thu, Jul 07, 2022 at 02:52:36PM -0700, Florian Fainelli wrote: > It is permissible for kernel code to call virt_to_phys() against virtual > addresses that are in KSEG0 or KSEG1 and we need to be dealing with both > types. Add a final condition that ensures that the virtual address is > below KSEG2. > > Fixes: dfad83cb7193 ("MIPS: Add support for CONFIG_DEBUG_VIRTUAL") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > arch/mips/mm/physaddr.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/mips/mm/physaddr.c b/arch/mips/mm/physaddr.c > index a1ced5e44951..a82f8f57a652 100644 > --- a/arch/mips/mm/physaddr.c > +++ b/arch/mips/mm/physaddr.c > @@ -5,6 +5,7 @@ > #include <linux/mmdebug.h> > #include <linux/mm.h> > > +#include <asm/addrspace.h> > #include <asm/sections.h> > #include <asm/io.h> > #include <asm/page.h> > @@ -30,7 +31,7 @@ static inline bool __debug_virt_addr_valid(unsigned long x) > if (x == MAX_DMA_ADDRESS) > return true; > > - return false; > + return KSEGX(x) < KSEG2; > } > > phys_addr_t __virt_to_phys(volatile const void *x) > -- > 2.25.1 applied to mips-next. Thomas.
On Mon, Jul 11, 2022 at 10:38:48AM +0200, Thomas Bogendoerfer wrote: > On Thu, Jul 07, 2022 at 02:52:36PM -0700, Florian Fainelli wrote: > > It is permissible for kernel code to call virt_to_phys() against virtual > > addresses that are in KSEG0 or KSEG1 and we need to be dealing with both > > types. Add a final condition that ensures that the virtual address is > > below KSEG2. > > > > Fixes: dfad83cb7193 ("MIPS: Add support for CONFIG_DEBUG_VIRTUAL") > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > --- > > arch/mips/mm/physaddr.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/mips/mm/physaddr.c b/arch/mips/mm/physaddr.c > > index a1ced5e44951..a82f8f57a652 100644 > > --- a/arch/mips/mm/physaddr.c > > +++ b/arch/mips/mm/physaddr.c > > @@ -5,6 +5,7 @@ > > #include <linux/mmdebug.h> > > #include <linux/mm.h> > > > > +#include <asm/addrspace.h> > > #include <asm/sections.h> > > #include <asm/io.h> > > #include <asm/page.h> > > @@ -30,7 +31,7 @@ static inline bool __debug_virt_addr_valid(unsigned long x) > > if (x == MAX_DMA_ADDRESS) > > return true; > > > > - return false; > > + return KSEGX(x) < KSEG2; > > } > > > > phys_addr_t __virt_to_phys(volatile const void *x) > > -- > > 2.25.1 > > applied to mips-next. Are you sure it was ready to be applied? Link: https://lore.kernel.org/linux-mips/20220708115851.ejsooiilxcopkoei@mobilestation/ -Sergey > > Thomas. > > -- > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a > good idea. [ RFC1925, 2.3 ]
On Mon, Jul 11, 2022 at 01:40:52PM +0300, Serge Semin wrote: > On Mon, Jul 11, 2022 at 10:38:48AM +0200, Thomas Bogendoerfer wrote: > > On Thu, Jul 07, 2022 at 02:52:36PM -0700, Florian Fainelli wrote: > > > It is permissible for kernel code to call virt_to_phys() against virtual > > > addresses that are in KSEG0 or KSEG1 and we need to be dealing with both > > > types. Add a final condition that ensures that the virtual address is > > > below KSEG2. > > > > > > Fixes: dfad83cb7193 ("MIPS: Add support for CONFIG_DEBUG_VIRTUAL") > > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > > --- > > > arch/mips/mm/physaddr.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/mips/mm/physaddr.c b/arch/mips/mm/physaddr.c > > > index a1ced5e44951..a82f8f57a652 100644 > > > --- a/arch/mips/mm/physaddr.c > > > +++ b/arch/mips/mm/physaddr.c > > > @@ -5,6 +5,7 @@ > > > #include <linux/mmdebug.h> > > > #include <linux/mm.h> > > > > > > +#include <asm/addrspace.h> > > > #include <asm/sections.h> > > > #include <asm/io.h> > > > #include <asm/page.h> > > > @@ -30,7 +31,7 @@ static inline bool __debug_virt_addr_valid(unsigned long x) > > > if (x == MAX_DMA_ADDRESS) > > > return true; > > > > > > - return false; > > > + return KSEGX(x) < KSEG2; > > > } > > > > > > phys_addr_t __virt_to_phys(volatile const void *x) > > > -- > > > 2.25.1 > > > > > applied to mips-next. > > Are you sure it was ready to be applied? > Link: https://lore.kernel.org/linux-mips/20220708115851.ejsooiilxcopkoei@mobilestation/ your comment sounded like optimizing, which can be done later on, so I assumed it ready. Thomas.
On Mon, Jul 11, 2022 at 01:27:40PM +0200, Thomas Bogendoerfer wrote: > On Mon, Jul 11, 2022 at 01:40:52PM +0300, Serge Semin wrote: > > On Mon, Jul 11, 2022 at 10:38:48AM +0200, Thomas Bogendoerfer wrote: > > > On Thu, Jul 07, 2022 at 02:52:36PM -0700, Florian Fainelli wrote: > > > > It is permissible for kernel code to call virt_to_phys() against virtual > > > > addresses that are in KSEG0 or KSEG1 and we need to be dealing with both > > > > types. Add a final condition that ensures that the virtual address is > > > > below KSEG2. > > > > > > > > Fixes: dfad83cb7193 ("MIPS: Add support for CONFIG_DEBUG_VIRTUAL") > > > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > > > --- > > > > arch/mips/mm/physaddr.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/mips/mm/physaddr.c b/arch/mips/mm/physaddr.c > > > > index a1ced5e44951..a82f8f57a652 100644 > > > > --- a/arch/mips/mm/physaddr.c > > > > +++ b/arch/mips/mm/physaddr.c > > > > @@ -5,6 +5,7 @@ > > > > #include <linux/mmdebug.h> > > > > #include <linux/mm.h> > > > > > > > > +#include <asm/addrspace.h> > > > > #include <asm/sections.h> > > > > #include <asm/io.h> > > > > #include <asm/page.h> > > > > @@ -30,7 +31,7 @@ static inline bool __debug_virt_addr_valid(unsigned long x) > > > > if (x == MAX_DMA_ADDRESS) > > > > return true; > > > > > > > > - return false; > > > > + return KSEGX(x) < KSEG2; > > > > } > > > > > > > > phys_addr_t __virt_to_phys(volatile const void *x) > > > > -- > > > > 2.25.1 > > > > > > > > applied to mips-next. > > > > Are you sure it was ready to be applied? > > Link: https://lore.kernel.org/linux-mips/20220708115851.ejsooiilxcopkoei@mobilestation/ > > your comment sounded like optimizing, which can be done later on, so > I assumed it ready. What about Malta and EVA? -Sergey > > Thomas. > > -- > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a > good idea. [ RFC1925, 2.3 ]
On 7/11/2022 4:29 AM, Serge Semin wrote: > On Mon, Jul 11, 2022 at 01:27:40PM +0200, Thomas Bogendoerfer wrote: >> On Mon, Jul 11, 2022 at 01:40:52PM +0300, Serge Semin wrote: >>> On Mon, Jul 11, 2022 at 10:38:48AM +0200, Thomas Bogendoerfer wrote: >>>> On Thu, Jul 07, 2022 at 02:52:36PM -0700, Florian Fainelli wrote: >>>>> It is permissible for kernel code to call virt_to_phys() against virtual >>>>> addresses that are in KSEG0 or KSEG1 and we need to be dealing with both >>>>> types. Add a final condition that ensures that the virtual address is >>>>> below KSEG2. >>>>> >>>>> Fixes: dfad83cb7193 ("MIPS: Add support for CONFIG_DEBUG_VIRTUAL") >>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>>>> --- >>>>> arch/mips/mm/physaddr.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/arch/mips/mm/physaddr.c b/arch/mips/mm/physaddr.c >>>>> index a1ced5e44951..a82f8f57a652 100644 >>>>> --- a/arch/mips/mm/physaddr.c >>>>> +++ b/arch/mips/mm/physaddr.c >>>>> @@ -5,6 +5,7 @@ >>>>> #include <linux/mmdebug.h> >>>>> #include <linux/mm.h> >>>>> >>>>> +#include <asm/addrspace.h> >>>>> #include <asm/sections.h> >>>>> #include <asm/io.h> >>>>> #include <asm/page.h> >>>>> @@ -30,7 +31,7 @@ static inline bool __debug_virt_addr_valid(unsigned long x) >>>>> if (x == MAX_DMA_ADDRESS) >>>>> return true; >>>>> >>>>> - return false; >>>>> + return KSEGX(x) < KSEG2; >>>>> } >>>>> >>>>> phys_addr_t __virt_to_phys(volatile const void *x) >>>>> -- >>>>> 2.25.1 >>>> >>> >>>> applied to mips-next. >>> >>> Are you sure it was ready to be applied? >>> Link: https://lore.kernel.org/linux-mips/20220708115851.ejsooiilxcopkoei@mobilestation/ >> > >> your comment sounded like optimizing, which can be done later on, so >> I assumed it ready. > > What about Malta and EVA? Sergey's comment definitively need to be addressed, and I would not see the point in making an incremental change to a wrong fix. Can you drop that patch for now? Thanks!
On Mon, Jul 11, 2022 at 05:02:51PM -0700, Florian Fainelli wrote: > On 7/11/2022 4:29 AM, Serge Semin wrote: > > On Mon, Jul 11, 2022 at 01:27:40PM +0200, Thomas Bogendoerfer wrote: > > > On Mon, Jul 11, 2022 at 01:40:52PM +0300, Serge Semin wrote: > > > > On Mon, Jul 11, 2022 at 10:38:48AM +0200, Thomas Bogendoerfer wrote: > > > > > On Thu, Jul 07, 2022 at 02:52:36PM -0700, Florian Fainelli wrote: > > > > > > It is permissible for kernel code to call virt_to_phys() against virtual > > > > > > addresses that are in KSEG0 or KSEG1 and we need to be dealing with both > > > > > > types. Add a final condition that ensures that the virtual address is > > > > > > below KSEG2. > > > > > > > > > > > > Fixes: dfad83cb7193 ("MIPS: Add support for CONFIG_DEBUG_VIRTUAL") > > > > > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > > > > > --- > > > > > > arch/mips/mm/physaddr.c | 3 ++- > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/arch/mips/mm/physaddr.c b/arch/mips/mm/physaddr.c > > > > > > index a1ced5e44951..a82f8f57a652 100644 > > > > > > --- a/arch/mips/mm/physaddr.c > > > > > > +++ b/arch/mips/mm/physaddr.c > > > > > > @@ -5,6 +5,7 @@ > > > > > > #include <linux/mmdebug.h> > > > > > > #include <linux/mm.h> > > > > > > +#include <asm/addrspace.h> > > > > > > #include <asm/sections.h> > > > > > > #include <asm/io.h> > > > > > > #include <asm/page.h> > > > > > > @@ -30,7 +31,7 @@ static inline bool __debug_virt_addr_valid(unsigned long x) > > > > > > if (x == MAX_DMA_ADDRESS) > > > > > > return true; > > > > > > - return false; > > > > > > + return KSEGX(x) < KSEG2; > > > > > > } > > > > > > phys_addr_t __virt_to_phys(volatile const void *x) > > > > > > -- > > > > > > 2.25.1 > > > > > > > > > > > > > > applied to mips-next. > > > > > > > > Are you sure it was ready to be applied? > > > > Link: https://lore.kernel.org/linux-mips/20220708115851.ejsooiilxcopkoei@mobilestation/ > > > > > > > > your comment sounded like optimizing, which can be done later on, so > > > I assumed it ready. > > > > What about Malta and EVA? > > Sergey's comment definitively need to be addressed, and I would not see the > point in making an incremental change to a wrong fix. Can you drop that > patch for now? Thanks! dropped. Thomas.
On 7/8/22 04:58, Serge Semin wrote: > On Thu, Jul 07, 2022 at 02:52:36PM -0700, Florian Fainelli wrote: >> It is permissible for kernel code to call virt_to_phys() against virtual >> addresses that are in KSEG0 or KSEG1 and we need to be dealing with both >> types. Add a final condition that ensures that the virtual address is >> below KSEG2. >> >> Fixes: dfad83cb7193 ("MIPS: Add support for CONFIG_DEBUG_VIRTUAL") >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> arch/mips/mm/physaddr.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/mips/mm/physaddr.c b/arch/mips/mm/physaddr.c >> index a1ced5e44951..a82f8f57a652 100644 >> --- a/arch/mips/mm/physaddr.c >> +++ b/arch/mips/mm/physaddr.c >> @@ -5,6 +5,7 @@ >> #include <linux/mmdebug.h> >> #include <linux/mm.h> >> >> +#include <asm/addrspace.h> >> #include <asm/sections.h> >> #include <asm/io.h> >> #include <asm/page.h> >> @@ -30,7 +31,7 @@ static inline bool __debug_virt_addr_valid(unsigned long x) >> if (x == MAX_DMA_ADDRESS) >> return true; >> > >> - return false; >> + return KSEGX(x) < KSEG2; > > With this do we really need the high_memory-based conditionals in this > method? > > If the line above is the only way to take the uncached segment into > account then can we reduce the whole method to: > static inline bool __debug_virt_addr_valid { > return x >= PAGE_OFFSET && KSEGX(x) < KSEG2; > } > ? > > Though this still may be invalid for EVA systems, like malta (see > arch/mips/include/asm/mach-malta/spaces.h). > > Note AFAICS if EVA is enabled, highmem is implied to be disabled (see > the CPU_MIPS32_3_5_EVA config utilization and HIGHMEM config > dependencies). Thus all the memory is supposed to be linearly mapped > in that case. OK, so if all of the memory is linearly mapped, then I am not too sure what we will be able to check, which is in essence pretty similar to what happens on MIPS64, right? Maybe DEBUG_VIRTUAL should depend on !EVA as well?
On Tue, Jul 12, 2022 at 09:28:02AM -0700, Florian Fainelli wrote: > On 7/8/22 04:58, Serge Semin wrote: > > On Thu, Jul 07, 2022 at 02:52:36PM -0700, Florian Fainelli wrote: > > > It is permissible for kernel code to call virt_to_phys() against virtual > > > addresses that are in KSEG0 or KSEG1 and we need to be dealing with both > > > types. Add a final condition that ensures that the virtual address is > > > below KSEG2. > > > > > > Fixes: dfad83cb7193 ("MIPS: Add support for CONFIG_DEBUG_VIRTUAL") > > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > > --- > > > arch/mips/mm/physaddr.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/mips/mm/physaddr.c b/arch/mips/mm/physaddr.c > > > index a1ced5e44951..a82f8f57a652 100644 > > > --- a/arch/mips/mm/physaddr.c > > > +++ b/arch/mips/mm/physaddr.c > > > @@ -5,6 +5,7 @@ > > > #include <linux/mmdebug.h> > > > #include <linux/mm.h> > > > +#include <asm/addrspace.h> > > > #include <asm/sections.h> > > > #include <asm/io.h> > > > #include <asm/page.h> > > > @@ -30,7 +31,7 @@ static inline bool __debug_virt_addr_valid(unsigned long x) > > > if (x == MAX_DMA_ADDRESS) > > > return true; > > > > > - return false; > > > + return KSEGX(x) < KSEG2; > > > > With this do we really need the high_memory-based conditionals in this > > method? > > > > If the line above is the only way to take the uncached segment into > > account then can we reduce the whole method to: > > static inline bool __debug_virt_addr_valid { > > return x >= PAGE_OFFSET && KSEGX(x) < KSEG2; > > } > > ? > > > > Though this still may be invalid for EVA systems, like malta (see > > arch/mips/include/asm/mach-malta/spaces.h). > > > > Note AFAICS if EVA is enabled, highmem is implied to be disabled (see > > the CPU_MIPS32_3_5_EVA config utilization and HIGHMEM config > > dependencies). Thus all the memory is supposed to be linearly mapped > > in that case. > > OK, so if all of the memory is linearly mapped, then I am not too sure what > we will be able to check, which is in essence pretty similar to what happens > on MIPS64, right? Essence is right, but in general situation is more complicated. Basically EVA (seems like a Bible reference...) provides a way to change the traditional MIPS memory address space to pretty much any within 4GB virtual-to-physical address mapping. Most importantly it can be used to eliminate the limitation of having just 512MB of the directly mapped memory in kernel. Anyway neither MIPS HIGHMEM kernel config nor the Malta's EVA mode don't imply having high-memory enabled in case of EVAs. Most likely the constraint has been set due to the MIPS arch code too much relying on the traditional address space mapping. So the high-memory part just wasn't fixed to be properly working in that case, while the CPU MMU-type segments can still be defined for EVA. As the comment in the Malta spaces.h header file says HIGHMEM_START is preserved just for the correct high-mem macros arithmetics. Just to note. Lack of high-memory in case of EVA is a big drawback because some physical memory gets to be unavailable. At the very least 512MB segment must be preserved for the uncached kernel region for MMIOs. Not to say that XPA won't work without high-memory. So it's either XPA (greater than 4GB physical memory) or EVA (up to 3.5GB directly mapped kernel memory). So annoying. > > Maybe DEBUG_VIRTUAL should depend on !EVA as well? In that case the debug-version of __phys_addr_symbol() will be unavailable too. I would rather suggest to fix the __debug_virt_addr_valid() method implementation to at least returning always true in case of EVA or !HIGHMEM. -Sergey > -- > Florian
diff --git a/arch/mips/mm/physaddr.c b/arch/mips/mm/physaddr.c index a1ced5e44951..a82f8f57a652 100644 --- a/arch/mips/mm/physaddr.c +++ b/arch/mips/mm/physaddr.c @@ -5,6 +5,7 @@ #include <linux/mmdebug.h> #include <linux/mm.h> +#include <asm/addrspace.h> #include <asm/sections.h> #include <asm/io.h> #include <asm/page.h> @@ -30,7 +31,7 @@ static inline bool __debug_virt_addr_valid(unsigned long x) if (x == MAX_DMA_ADDRESS) return true; - return false; + return KSEGX(x) < KSEG2; } phys_addr_t __virt_to_phys(volatile const void *x)
It is permissible for kernel code to call virt_to_phys() against virtual addresses that are in KSEG0 or KSEG1 and we need to be dealing with both types. Add a final condition that ensures that the virtual address is below KSEG2. Fixes: dfad83cb7193 ("MIPS: Add support for CONFIG_DEBUG_VIRTUAL") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- arch/mips/mm/physaddr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)