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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
> ---
> 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.
--
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 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > > ---
> > > 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.
--
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: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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
>>>>> ---
>>>>> 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!
--
Florian
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 <[email protected]>
> > > > > > ---
> > > > > > 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.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]
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 <[email protected]>
>> ---
>> 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?
--
Florian
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 <[email protected]>
> > > ---
> > > 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