[PATCH 0/4] apply page shift to PFN instead of VA in pfn_to_virt

[PATCH 0/4] apply page shift to PFN instead of VA in pfn_to_virt

[Yan Zhao]

This is a tiny fix to pfn_to_virt() for some platforms.

The original implementaion of pfn_to_virt() takes PFN instead of PA as the
input to macro __va, with PAGE_SHIFT applying to the converted VA, which
is not right under most conditions, especially when there's an offset in
__va.


Yan Zhao (4):
  asm-generic/page.h: apply page shift to PFN instead of VA in
    pfn_to_virt
  csky: apply page shift to PFN instead of VA in pfn_to_virt
  Hexagon: apply page shift to PFN instead of VA in pfn_to_virt
  openrisc: apply page shift to PFN instead of VA in pfn_to_virt

 arch/csky/include/asm/page.h     | 2 +-
 arch/hexagon/include/asm/page.h  | 2 +-
 arch/openrisc/include/asm/page.h | 2 +-
 include/asm-generic/page.h       | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)


base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
-- 
2.17.1

[PATCH 1/4] asm-generic/page.h: apply page shift to PFN instead of VA in pfn_to_virt

[Yan Zhao]

Apply the page shift to PFN to get physical address for final VA.
The macro __va should take physical address instead of PFN as input.

Fixes: 2d78057f0dd4 ("asm-generic/page.h: Make pfn accessors static inlines")
Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
---
 include/asm-generic/page.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/page.h b/include/asm-generic/page.h
index 9773582fd96e..4f1265207b9a 100644
--- a/include/asm-generic/page.h
+++ b/include/asm-generic/page.h
@@ -81,7 +81,7 @@ static inline unsigned long virt_to_pfn(const void *kaddr)
 #define virt_to_pfn virt_to_pfn
 static inline void *pfn_to_virt(unsigned long pfn)
 {
-	return __va(pfn) << PAGE_SHIFT;
+	return __va(pfn << PAGE_SHIFT);
 }
 #define pfn_to_virt pfn_to_virt
 
-- 
2.17.1

Re: [PATCH 1/4] asm-generic/page.h: apply page shift to PFN instead of VA in pfn_to_virt

[Guo Ren]

On Wed, Jan 31, 2024 at 2:27 PM Yan Zhao <yan.y.zhao@intel.com> wrote:
>
> Apply the page shift to PFN to get physical address for final VA.
> The macro __va should take physical address instead of PFN as input.
>
> Fixes: 2d78057f0dd4 ("asm-generic/page.h: Make pfn accessors static inlines")
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> ---
>  include/asm-generic/page.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/asm-generic/page.h b/include/asm-generic/page.h
> index 9773582fd96e..4f1265207b9a 100644
> --- a/include/asm-generic/page.h
> +++ b/include/asm-generic/page.h
> @@ -81,7 +81,7 @@ static inline unsigned long virt_to_pfn(const void *kaddr)
>  #define virt_to_pfn virt_to_pfn
>  static inline void *pfn_to_virt(unsigned long pfn)
>  {
> -       return __va(pfn) << PAGE_SHIFT;
> +       return __va(pfn << PAGE_SHIFT);
Oh, that's a terrible bug; Thx for fixing it.

Reviewed-by: Guo Ren <guoren@kernel.org>

>  }
>  #define pfn_to_virt pfn_to_virt
>
> --
> 2.17.1
>


-- 
Best Regards
 Guo Ren

Re: [PATCH 1/4] asm-generic/page.h: apply page shift to PFN instead of VA in pfn_to_virt

[Linus Walleij]

On Wed, Jan 31, 2024 at 7:27 AM Yan Zhao <yan.y.zhao@intel.com> wrote:

> Apply the page shift to PFN to get physical address for final VA.
> The macro __va should take physical address instead of PFN as input.
>
> Fixes: 2d78057f0dd4 ("asm-generic/page.h: Make pfn accessors static inlines")
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>

My bug, obviously. :(
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I thought this was already applied with the other fixes, but maybe it
was missed?

Yours,
Linus Walleij

Re: [PATCH 1/4] asm-generic/page.h: apply page shift to PFN instead of VA in pfn_to_virt

[Yan Zhao]

On Thu, Feb 29, 2024 at 02:34:35PM +0100, Linus Walleij wrote:
> On Wed, Jan 31, 2024 at 7:27 AM Yan Zhao <yan.y.zhao@intel.com> wrote:
> 
> > Apply the page shift to PFN to get physical address for final VA.
> > The macro __va should take physical address instead of PFN as input.
> >
> > Fixes: 2d78057f0dd4 ("asm-generic/page.h: Make pfn accessors static inlines")
> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> 
> My bug, obviously. :(
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> I thought this was already applied with the other fixes, but maybe it
> was missed?
>
Hi Linus,
The other 3 in csky/hexagon/openrisc should have been applied in
https://lore.kernel.org/all/20240202140550.9886-1-yan.y.zhao@intel.com/.

This one in asm-generic is not, because Arnd said he is going to remove
the header as a whole soon. 

I explained it in the change log :)
"The pfn_to_virt() in asm-generic/page.h is not touched in this patch as
it's referenced by page_to_virt() in that header while the whole header is
going to be removed as a whole due to no one including it."

Thanks
Yan

[PATCH 2/4] csky: apply page shift to PFN instead of VA in pfn_to_virt

[Yan Zhao]

Apply the page shift to PFN to get physical address for final VA.
The macro __va should take physical address instead of PFN as input.

Fixes: c1884e1e1164 ("csky: Make pfn accessors static inlines")
Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
---
 arch/csky/include/asm/page.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/csky/include/asm/page.h b/arch/csky/include/asm/page.h
index 4a0502e324a6..2c4cc7825a7b 100644
--- a/arch/csky/include/asm/page.h
+++ b/arch/csky/include/asm/page.h
@@ -84,7 +84,7 @@ static inline unsigned long virt_to_pfn(const void *kaddr)
 
 static inline void * pfn_to_virt(unsigned long pfn)
 {
-	return (void *)((unsigned long)__va(pfn) << PAGE_SHIFT);
+	return __va(pfn << PAGE_SHIFT);
 }
 
 #define MAP_NR(x)	PFN_DOWN((unsigned long)(x) - PAGE_OFFSET - \
-- 
2.17.1

Re: [PATCH 2/4] csky: apply page shift to PFN instead of VA in pfn_to_virt

[Guo Ren]

On Wed, Jan 31, 2024 at 2:28 PM Yan Zhao <yan.y.zhao@intel.com> wrote:
>
> Apply the page shift to PFN to get physical address for final VA.
> The macro __va should take physical address instead of PFN as input.
>
> Fixes: c1884e1e1164 ("csky: Make pfn accessors static inlines")
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> ---
>  arch/csky/include/asm/page.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/csky/include/asm/page.h b/arch/csky/include/asm/page.h
> index 4a0502e324a6..2c4cc7825a7b 100644
> --- a/arch/csky/include/asm/page.h
> +++ b/arch/csky/include/asm/page.h
> @@ -84,7 +84,7 @@ static inline unsigned long virt_to_pfn(const void *kaddr)
>
>  static inline void * pfn_to_virt(unsigned long pfn)
>  {
> -       return (void *)((unsigned long)__va(pfn) << PAGE_SHIFT);
> +       return __va(pfn << PAGE_SHIFT);
>  }
Reviewed-by: Guo Ren <guoren@kernel.org>

>
>  #define MAP_NR(x)      PFN_DOWN((unsigned long)(x) - PAGE_OFFSET - \
> --
> 2.17.1
>


-- 
Best Regards
 Guo Ren

[PATCH 3/4] Hexagon: apply page shift to PFN instead of VA in pfn_to_virt

[Yan Zhao]

Apply the page shift to PFN to get physical address for final VA.
The macro __va should take physical address instead of PFN as input.

Fixes: d6e81532b10d ("Hexagon: Make pfn accessors statics inlines")
Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
---
 arch/hexagon/include/asm/page.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/hexagon/include/asm/page.h b/arch/hexagon/include/asm/page.h
index 10f1bc07423c..1d341590791a 100644
--- a/arch/hexagon/include/asm/page.h
+++ b/arch/hexagon/include/asm/page.h
@@ -135,7 +135,7 @@ static inline unsigned long virt_to_pfn(const void *kaddr)
 
 static inline void *pfn_to_virt(unsigned long pfn)
 {
-	return (void *)((unsigned long)__va(pfn) << PAGE_SHIFT);
+	return __va(pfn << PAGE_SHIFT);
 }
 
 
-- 
2.17.1

[PATCH 4/4] openrisc: apply page shift to PFN instead of VA in pfn_to_virt

[Yan Zhao]

Apply the page shift to PFN to get physical address for final VA.
The macro __va should take physical address instead of PFN as input.

Fixes: 232ba1630c66 ("openrisc: Make pfn accessors statics inlines")
Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
---
 arch/openrisc/include/asm/page.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/openrisc/include/asm/page.h b/arch/openrisc/include/asm/page.h
index 44fc1fd56717..55c66f6cb1bd 100644
--- a/arch/openrisc/include/asm/page.h
+++ b/arch/openrisc/include/asm/page.h
@@ -79,7 +79,7 @@ static inline unsigned long virt_to_pfn(const void *kaddr)
 
 static inline void * pfn_to_virt(unsigned long pfn)
 {
-	return (void *)((unsigned long)__va(pfn) << PAGE_SHIFT);
+	return __va(pfn << PAGE_SHIFT);
 }
 
 #define virt_to_page(addr) \
-- 
2.17.1

Re: [PATCH 0/4] apply page shift to PFN instead of VA in pfn_to_virt

[Linus Walleij]

On Wed, Jan 31, 2024 at 7:25 AM Yan Zhao <yan.y.zhao@intel.com> wrote:

> This is a tiny fix to pfn_to_virt() for some platforms.
>
> The original implementaion of pfn_to_virt() takes PFN instead of PA as the
> input to macro __va, with PAGE_SHIFT applying to the converted VA, which
> is not right under most conditions, especially when there's an offset in
> __va.

Ooops that's right, I wonder why I got it wrong.
Arithmetic made it not regress :/
Thank you so much for fixing this Yan!

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Arnd: I think you can take most of them through the arch tree.

Yours,
Linus Walleij

Re: [PATCH 0/4] apply page shift to PFN instead of VA in pfn_to_virt

[Arnd Bergmann]

On Wed, Jan 31, 2024, at 06:51, Yan Zhao wrote:
> This is a tiny fix to pfn_to_virt() for some platforms.
>
> The original implementaion of pfn_to_virt() takes PFN instead of PA as the
> input to macro __va, with PAGE_SHIFT applying to the converted VA, which
> is not right under most conditions, especially when there's an offset in
> __va.
>
>
> Yan Zhao (4):
>   asm-generic/page.h: apply page shift to PFN instead of VA in
>     pfn_to_virt
>   csky: apply page shift to PFN instead of VA in pfn_to_virt
>   Hexagon: apply page shift to PFN instead of VA in pfn_to_virt
>   openrisc: apply page shift to PFN instead of VA in pfn_to_virt

Nice catch, this is clearly a correct fix, and I can take
the series through the asm-generic tree if we want to take
this approach.

I made a couple of interesting observations looking at this patch
though:

- this function is only used in architecture specific code on
  m68k, riscv and s390, though a couple of other architectures
  have the same definition.

- There is another function that does the same thing called
  pfn_to_kaddr(), which is defined on arm, arm64, csky,
  loongarch, mips, nios2, powerpc, s390, sh, sparc and x86,
  as well as yet another pfn_va() on parisc.

- the asm-generic/page.h file used to be included by h8300, c6x
  and blackfin, all of which are now gone. It has no users left
  and can just as well get removed, unless we find a new use
  for it.

Since it looks like the four broken functions you fix
don't have a single caller, maybe it would be better to
just remove them all?

How exactly did you notice the function being wrong,
did you try to add a user somewhere, or just read through
the code?

    Arnd

Re: [PATCH 0/4] apply page shift to PFN instead of VA in pfn_to_virt

[Yan Zhao]

On Wed, Jan 31, 2024 at 12:48:38PM +0100, Arnd Bergmann wrote:
> On Wed, Jan 31, 2024, at 06:51, Yan Zhao wrote:
> > This is a tiny fix to pfn_to_virt() for some platforms.
> >
> > The original implementaion of pfn_to_virt() takes PFN instead of PA as the
> > input to macro __va, with PAGE_SHIFT applying to the converted VA, which
> > is not right under most conditions, especially when there's an offset in
> > __va.
> >
> >
> > Yan Zhao (4):
> >   asm-generic/page.h: apply page shift to PFN instead of VA in
> >     pfn_to_virt
> >   csky: apply page shift to PFN instead of VA in pfn_to_virt
> >   Hexagon: apply page shift to PFN instead of VA in pfn_to_virt
> >   openrisc: apply page shift to PFN instead of VA in pfn_to_virt
> 
> Nice catch, this is clearly a correct fix, and I can take
> the series through the asm-generic tree if we want to take
> this approach.
> 
> I made a couple of interesting observations looking at this patch
> though:
> 
> - this function is only used in architecture specific code on
>   m68k, riscv and s390, though a couple of other architectures
>   have the same definition.
> 
> - There is another function that does the same thing called
>   pfn_to_kaddr(), which is defined on arm, arm64, csky,
>   loongarch, mips, nios2, powerpc, s390, sh, sparc and x86,
>   as well as yet another pfn_va() on parisc.
> 
> - the asm-generic/page.h file used to be included by h8300, c6x
>   and blackfin, all of which are now gone. It has no users left
>   and can just as well get removed, unless we find a new use
>   for it.
> 
> Since it looks like the four broken functions you fix
> don't have a single caller, maybe it would be better to
> just remove them all?
> 
> How exactly did you notice the function being wrong,
> did you try to add a user somewhere, or just read through
> the code?
I came across them when I was debugging an unexpected kernel page fault
on x86, and I was not sure whether page_to_virt() was compiled in
asm-generic/page.h or linux/mm.h.
Though finally, it turned out that the one in linux/mm.h was used, which
yielded the right result and the unexpected kernel page fault in my case
was not related to page_to_virt(), it did lead me to noticing that the
pfn_to_virt() in asm-generic/page.h and other 3 archs did not look right.

Yes, unlike virt_to_pfn() which still has a caller in openrisc (among
csky, Hexagon, openrisc), pfn_to_virt() now does not have a caller in
the 3 archs. Though both virt_to_pfn() and pfn_to_virt() are referenced
in asm-generic/page.h, I also not sure if we need to remove the
asm-generic/page.h which may serve as a template to future archs ?

So, either way looks good to me :)

Re: [PATCH 0/4] apply page shift to PFN instead of VA in pfn_to_virt

[Arnd Bergmann]

On Thu, Feb 1, 2024, at 01:01, Yan Zhao wrote:
> On Wed, Jan 31, 2024 at 12:48:38PM +0100, Arnd Bergmann wrote:
>> On Wed, Jan 31, 2024, at 06:51, Yan Zhao wrote:
>> 
>> How exactly did you notice the function being wrong,
>> did you try to add a user somewhere, or just read through
>> the code?
> I came across them when I was debugging an unexpected kernel page fault
> on x86, and I was not sure whether page_to_virt() was compiled in
> asm-generic/page.h or linux/mm.h.
> Though finally, it turned out that the one in linux/mm.h was used, which
> yielded the right result and the unexpected kernel page fault in my case
> was not related to page_to_virt(), it did lead me to noticing that the
> pfn_to_virt() in asm-generic/page.h and other 3 archs did not look right.
>
> Yes, unlike virt_to_pfn() which still has a caller in openrisc (among
> csky, Hexagon, openrisc), pfn_to_virt() now does not have a caller in
> the 3 archs. Though both virt_to_pfn() and pfn_to_virt() are referenced
> in asm-generic/page.h, I also not sure if we need to remove the
> asm-generic/page.h which may serve as a template to future archs ?
>
> So, either way looks good to me :)

I think it's fair to assume we won't need asm-generic/page.h any
more, as we likely won't be adding new NOMMU architectures.
I can have a look myself at removing any such unused headers in
include/asm-generic/, it's probably not the only one.

Can you just send a patch to remove the unused pfn_to_virt()
functions?

     Arnd

Re: [PATCH 0/4] apply page shift to PFN instead of VA in pfn_to_virt

[Geert Uytterhoeven]

Hi Arnd,

On Thu, Feb 1, 2024 at 11:11 AM Arnd Bergmann <arnd@arndb.de> wrote:
> I think it's fair to assume we won't need asm-generic/page.h any
> more, as we likely won't be adding new NOMMU architectures.

So you think riscv-nommu (k210) was the last one we will ever see?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] apply page shift to PFN instead of VA in pfn_to_virt

[Arnd Bergmann]

On Thu, Feb 1, 2024, at 11:46, Geert Uytterhoeven wrote:
> Hi Arnd,
>
> On Thu, Feb 1, 2024 at 11:11 AM Arnd Bergmann <arnd@arndb.de> wrote:
>> I think it's fair to assume we won't need asm-generic/page.h any
>> more, as we likely won't be adding new NOMMU architectures.
>
> So you think riscv-nommu (k210) was the last one we will ever see?

Yes. We've already removed half of the nommu architectures
(blackfin, avr32, h8300, m32r, microblaze-nommu)  over
the past couple of years, and the remaining ones are pretty
much only there to support existing users.

The only platform one that I see getting real work is
esp32 [1], but that is not a new architecture.

     Arnd

[1] https://github.com/jcmvbkbc/linux-xtensa/tree/xtensa-6.8-rc2-esp32

Re: [PATCH 0/4] apply page shift to PFN instead of VA in pfn_to_virt

[Yan Zhao]

On Thu, Feb 01, 2024 at 06:46:46AM +0100, Arnd Bergmann wrote:
> On Thu, Feb 1, 2024, at 01:01, Yan Zhao wrote:
> > On Wed, Jan 31, 2024 at 12:48:38PM +0100, Arnd Bergmann wrote:
> >> On Wed, Jan 31, 2024, at 06:51, Yan Zhao wrote:
> >> 
> >> How exactly did you notice the function being wrong,
> >> did you try to add a user somewhere, or just read through
> >> the code?
> > I came across them when I was debugging an unexpected kernel page fault
> > on x86, and I was not sure whether page_to_virt() was compiled in
> > asm-generic/page.h or linux/mm.h.
> > Though finally, it turned out that the one in linux/mm.h was used, which
> > yielded the right result and the unexpected kernel page fault in my case
> > was not related to page_to_virt(), it did lead me to noticing that the
> > pfn_to_virt() in asm-generic/page.h and other 3 archs did not look right.
> >
> > Yes, unlike virt_to_pfn() which still has a caller in openrisc (among
> > csky, Hexagon, openrisc), pfn_to_virt() now does not have a caller in
> > the 3 archs. Though both virt_to_pfn() and pfn_to_virt() are referenced
> > in asm-generic/page.h, I also not sure if we need to remove the
> > asm-generic/page.h which may serve as a template to future archs ?
> >
> > So, either way looks good to me :)
> 
> I think it's fair to assume we won't need asm-generic/page.h any
> more, as we likely won't be adding new NOMMU architectures.
> I can have a look myself at removing any such unused headers in
> include/asm-generic/, it's probably not the only one.
> 
> Can you just send a patch to remove the unused pfn_to_virt()
> functions?
Ok. I'll do it!
BTW: do you think it's also good to keep this fixing series though we'll
remove the unused function later?
So if people want to revert the removal some day, they can get right one.

Or maybe I'm just paranoid, and explanation about the fix in the commit
message of patch for function removal is enough.

What's your preference? :)

Re: [PATCH 0/4] apply page shift to PFN instead of VA in pfn_to_virt

[Arnd Bergmann]

On Fri, Feb 2, 2024, at 02:02, Yan Zhao wrote:
> On Thu, Feb 01, 2024 at 06:46:46AM +0100, Arnd Bergmann wrote:
>> 
>> I think it's fair to assume we won't need asm-generic/page.h any
>> more, as we likely won't be adding new NOMMU architectures.
>> I can have a look myself at removing any such unused headers in
>> include/asm-generic/, it's probably not the only one.
>> 
>> Can you just send a patch to remove the unused pfn_to_virt()
>> functions?
> Ok. I'll do it!
> BTW: do you think it's also good to keep this fixing series though we'll
> remove the unused function later?
> So if people want to revert the removal some day, they can get right one.
>
> Or maybe I'm just paranoid, and explanation about the fix in the commit
> message of patch for function removal is enough.
>
> What's your preference? :)

I would just remove it, there is no point in having both
pfn_to_kaddr() and pfn_to_virt() when they do the exact
same thing aside from this bug.

Just do a single patch for all architectures, no need to
have three or four identical ones when I'm going to merge
them all through the same tree anyway.

Just make sure you explain in the changelog what the bug was
and how you noticed it, in case anyone is ever tempted to
bring the function back.

    Arnd

Re: [PATCH 0/4] apply page shift to PFN instead of VA in pfn_to_virt

[Yan Zhao]

On Fri, Feb 02, 2024 at 08:04:34AM +0100, Arnd Bergmann wrote:
> On Fri, Feb 2, 2024, at 02:02, Yan Zhao wrote:
> > On Thu, Feb 01, 2024 at 06:46:46AM +0100, Arnd Bergmann wrote:
> >> 
> >> I think it's fair to assume we won't need asm-generic/page.h any
> >> more, as we likely won't be adding new NOMMU architectures.
> >> I can have a look myself at removing any such unused headers in
> >> include/asm-generic/, it's probably not the only one.
> >> 
> >> Can you just send a patch to remove the unused pfn_to_virt()
> >> functions?
> > Ok. I'll do it!
> > BTW: do you think it's also good to keep this fixing series though we'll
> > remove the unused function later?
> > So if people want to revert the removal some day, they can get right one.
> >
> > Or maybe I'm just paranoid, and explanation about the fix in the commit
> > message of patch for function removal is enough.
> >
> > What's your preference? :)
> 
> I would just remove it, there is no point in having both
> pfn_to_kaddr() and pfn_to_virt() when they do the exact
> same thing aside from this bug.
>
> Just do a single patch for all architectures, no need to
> have three or four identical ones when I'm going to merge
> them all through the same tree anyway.
> 
> Just make sure you explain in the changelog what the bug was
> and how you noticed it, in case anyone is ever tempted to
> bring the function back.
Done.
https://lore.kernel.org/all/20240202140550.9886-1-yan.y.zhao@intel.com
Thanks for you guidance :)