[PATCH] powerpc: Make virt_to_pfn() a static inline

[PATCH] powerpc: Make virt_to_pfn() a static inline

[Linus Walleij]

Making virt_to_pfn() a static inline taking a strongly typed
(const void *) makes the contract of a passing a pointer of that
type to the function explicit and exposes any misuse of the
macro virt_to_pfn() acting polymorphic and accepting many types
such as (void *), (unitptr_t) or (unsigned long) as arguments
without warnings.

Move the virt_to_pfn() and related functions below the
declaration of __pa() so it compiles.

For symmetry do the same with pfn_to_kaddr().

As the file is included right into the linker file, we need
to surround the functions with ifndef __ASSEMBLY__ so we
don't cause compilation errors.

The conversion moreover exposes the fact that pmd_page_vaddr()
was returning an unsigned long rather than a const void * as
could be expected, so all the sites defining pmd_page_vaddr()
had to be augmented as well.

Finally the KVM code in book3s_64_mmu_hv.c was passing an
unsigned int to virt_to_phys() so fix that up with a cast so the
result compiles.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 arch/powerpc/include/asm/nohash/32/pgtable.h |  2 +-
 arch/powerpc/include/asm/nohash/64/pgtable.h |  2 +-
 arch/powerpc/include/asm/page.h              | 30 ++++++++++++++++++----------
 arch/powerpc/include/asm/pgtable.h           |  4 ++--
 arch/powerpc/kvm/book3s_64_mmu_hv.c          |  2 +-
 5 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/include/asm/nohash/32/pgtable.h b/arch/powerpc/include/asm/nohash/32/pgtable.h
index fec56d965f00..d6201b5096b8 100644
--- a/arch/powerpc/include/asm/nohash/32/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/32/pgtable.h
@@ -355,7 +355,7 @@ static inline int pte_young(pte_t pte)
 #define pmd_pfn(pmd)		(pmd_val(pmd) >> PAGE_SHIFT)
 #else
 #define pmd_page_vaddr(pmd)	\
-	((unsigned long)(pmd_val(pmd) & ~(PTE_TABLE_SIZE - 1)))
+	((const void *)(pmd_val(pmd) & ~(PTE_TABLE_SIZE - 1)))
 #define pmd_pfn(pmd)		(__pa(pmd_val(pmd)) >> PAGE_SHIFT)
 #endif
 
diff --git a/arch/powerpc/include/asm/nohash/64/pgtable.h b/arch/powerpc/include/asm/nohash/64/pgtable.h
index 287e25864ffa..81c801880933 100644
--- a/arch/powerpc/include/asm/nohash/64/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/64/pgtable.h
@@ -127,7 +127,7 @@ static inline pte_t pmd_pte(pmd_t pmd)
 #define	pmd_bad(pmd)		(!is_kernel_addr(pmd_val(pmd)) \
 				 || (pmd_val(pmd) & PMD_BAD_BITS))
 #define	pmd_present(pmd)	(!pmd_none(pmd))
-#define pmd_page_vaddr(pmd)	(pmd_val(pmd) & ~PMD_MASKED_BITS)
+#define pmd_page_vaddr(pmd)	((const void *)(pmd_val(pmd) & ~PMD_MASKED_BITS))
 extern struct page *pmd_page(pmd_t pmd);
 #define pmd_pfn(pmd)		(page_to_pfn(pmd_page(pmd)))
 
diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
index f2b6bf5687d0..9ee4b6d4a82a 100644
--- a/arch/powerpc/include/asm/page.h
+++ b/arch/powerpc/include/asm/page.h
@@ -9,6 +9,7 @@
 #ifndef __ASSEMBLY__
 #include <linux/types.h>
 #include <linux/kernel.h>
+#include <linux/bug.h>
 #else
 #include <asm/types.h>
 #endif
@@ -119,16 +120,6 @@ extern long long virt_phys_offset;
 #define ARCH_PFN_OFFSET		((unsigned long)(MEMORY_START >> PAGE_SHIFT))
 #endif
 
-#define virt_to_pfn(kaddr)	(__pa(kaddr) >> PAGE_SHIFT)
-#define virt_to_page(kaddr)	pfn_to_page(virt_to_pfn(kaddr))
-#define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
-
-#define virt_addr_valid(vaddr)	({					\
-	unsigned long _addr = (unsigned long)vaddr;			\
-	_addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory &&	\
-	pfn_valid(virt_to_pfn(_addr));					\
-})
-
 /*
  * On Book-E parts we need __va to parse the device tree and we can't
  * determine MEMORY_START until then.  However we can determine PHYSICAL_START
@@ -233,6 +224,25 @@ extern long long virt_phys_offset;
 #endif
 #endif
 
+#ifndef __ASSEMBLY__
+static inline unsigned long virt_to_pfn(const void *kaddr)
+{
+	return __pa(kaddr) >> PAGE_SHIFT;
+}
+
+static inline const void *pfn_to_kaddr(unsigned long pfn)
+{
+	return (const void *)(((unsigned long)__va(pfn)) << PAGE_SHIFT);
+}
+#endif
+
+#define virt_to_page(kaddr)	pfn_to_page(virt_to_pfn(kaddr))
+#define virt_addr_valid(vaddr)	({					\
+	unsigned long _addr = (unsigned long)vaddr;			\
+	_addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory &&	\
+	pfn_valid(virt_to_pfn((void *)_addr));				\
+})
+
 /*
  * Unfortunately the PLT is in the BSS in the PPC32 ELF ABI,
  * and needs to be executable.  This means the whole heap ends
diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
index 6a88bfdaa69b..a9515d3d7831 100644
--- a/arch/powerpc/include/asm/pgtable.h
+++ b/arch/powerpc/include/asm/pgtable.h
@@ -60,9 +60,9 @@ static inline pgprot_t pte_pgprot(pte_t pte)
 }
 
 #ifndef pmd_page_vaddr
-static inline unsigned long pmd_page_vaddr(pmd_t pmd)
+static inline const void *pmd_page_vaddr(pmd_t pmd)
 {
-	return ((unsigned long)__va(pmd_val(pmd) & ~PMD_MASKED_BITS));
+	return (const void *)((unsigned long)__va(pmd_val(pmd) & ~PMD_MASKED_BITS));
 }
 #define pmd_page_vaddr pmd_page_vaddr
 #endif
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 7f765d5ad436..efd0ebf70a5e 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -182,7 +182,7 @@ void kvmppc_free_hpt(struct kvm_hpt_info *info)
 	vfree(info->rev);
 	info->rev = NULL;
 	if (info->cma)
-		kvm_free_hpt_cma(virt_to_page(info->virt),
+		kvm_free_hpt_cma(virt_to_page((void *)info->virt),
 				 1 << (info->order - PAGE_SHIFT));
 	else if (info->virt)
 		free_pages(info->virt, info->order - PAGE_SHIFT);

---
base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
change-id: 20230808-virt-to-phys-powerpc-634cf7acd39a

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

Re: [PATCH] powerpc: Make virt_to_pfn() a static inline

[Michael Ellerman]

Linus Walleij <linus.walleij@linaro.org> writes:
> Making virt_to_pfn() a static inline taking a strongly typed
> (const void *) makes the contract of a passing a pointer of that
> type to the function explicit and exposes any misuse of the
> macro virt_to_pfn() acting polymorphic and accepting many types
> such as (void *), (unitptr_t) or (unsigned long) as arguments
> without warnings.
...
> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
> index f2b6bf5687d0..9ee4b6d4a82a 100644
> --- a/arch/powerpc/include/asm/page.h
> +++ b/arch/powerpc/include/asm/page.h
> @@ -9,6 +9,7 @@
>  #ifndef __ASSEMBLY__
>  #include <linux/types.h>
>  #include <linux/kernel.h>
> +#include <linux/bug.h>
>  #else
>  #include <asm/types.h>
>  #endif
> @@ -119,16 +120,6 @@ extern long long virt_phys_offset;
>  #define ARCH_PFN_OFFSET		((unsigned long)(MEMORY_START >> PAGE_SHIFT))
>  #endif
>  
> -#define virt_to_pfn(kaddr)	(__pa(kaddr) >> PAGE_SHIFT)
> -#define virt_to_page(kaddr)	pfn_to_page(virt_to_pfn(kaddr))
> -#define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
> -
> -#define virt_addr_valid(vaddr)	({					\
> -	unsigned long _addr = (unsigned long)vaddr;			\
> -	_addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory &&	\
> -	pfn_valid(virt_to_pfn(_addr));					\
> -})
> -
>  /*
>   * On Book-E parts we need __va to parse the device tree and we can't
>   * determine MEMORY_START until then.  However we can determine PHYSICAL_START
> @@ -233,6 +224,25 @@ extern long long virt_phys_offset;
>  #endif
>  #endif
>  
> +#ifndef __ASSEMBLY__
> +static inline unsigned long virt_to_pfn(const void *kaddr)
> +{
> +	return __pa(kaddr) >> PAGE_SHIFT;
> +}
> +
> +static inline const void *pfn_to_kaddr(unsigned long pfn)
> +{
> +	return (const void *)(((unsigned long)__va(pfn)) << PAGE_SHIFT);

Any reason to do it this way rather than:

+       return __va(pfn << PAGE_SHIFT);

Seems to be equivalent and much cleaner?

cheers

Re: [PATCH] powerpc: Make virt_to_pfn() a static inline

[Christophe Leroy]

Le 14/08/2023 à 14:37, Michael Ellerman a écrit :
> Linus Walleij <linus.walleij@linaro.org> writes:
>> Making virt_to_pfn() a static inline taking a strongly typed
>> (const void *) makes the contract of a passing a pointer of that
>> type to the function explicit and exposes any misuse of the
>> macro virt_to_pfn() acting polymorphic and accepting many types
>> such as (void *), (unitptr_t) or (unsigned long) as arguments
>> without warnings.
> ...
>> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
>> index f2b6bf5687d0..9ee4b6d4a82a 100644
>> --- a/arch/powerpc/include/asm/page.h
>> +++ b/arch/powerpc/include/asm/page.h
>> @@ -9,6 +9,7 @@
>>   #ifndef __ASSEMBLY__
>>   #include <linux/types.h>
>>   #include <linux/kernel.h>
>> +#include <linux/bug.h>
>>   #else
>>   #include <asm/types.h>
>>   #endif
>> @@ -119,16 +120,6 @@ extern long long virt_phys_offset;
>>   #define ARCH_PFN_OFFSET		((unsigned long)(MEMORY_START >> PAGE_SHIFT))
>>   #endif
>>   
>> -#define virt_to_pfn(kaddr)	(__pa(kaddr) >> PAGE_SHIFT)
>> -#define virt_to_page(kaddr)	pfn_to_page(virt_to_pfn(kaddr))
>> -#define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
>> -
>> -#define virt_addr_valid(vaddr)	({					\
>> -	unsigned long _addr = (unsigned long)vaddr;			\
>> -	_addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory &&	\
>> -	pfn_valid(virt_to_pfn(_addr));					\
>> -})
>> -
>>   /*
>>    * On Book-E parts we need __va to parse the device tree and we can't
>>    * determine MEMORY_START until then.  However we can determine PHYSICAL_START
>> @@ -233,6 +224,25 @@ extern long long virt_phys_offset;
>>   #endif
>>   #endif
>>   
>> +#ifndef __ASSEMBLY__
>> +static inline unsigned long virt_to_pfn(const void *kaddr)
>> +{
>> +	return __pa(kaddr) >> PAGE_SHIFT;
>> +}
>> +
>> +static inline const void *pfn_to_kaddr(unsigned long pfn)
>> +{
>> +	return (const void *)(((unsigned long)__va(pfn)) << PAGE_SHIFT);
> 
> Any reason to do it this way rather than:
> 
> +       return __va(pfn << PAGE_SHIFT);

Even cleaner:

	return __va(PFN_PHYS(pfn));

> 
> Seems to be equivalent and much cleaner?
> 
> cheers

Re: [PATCH] powerpc: Make virt_to_pfn() a static inline

[Linus Walleij]

On Mon, Aug 14, 2023 at 2:37 PM Michael Ellerman <mpe@ellerman.id.au> wrote:

> > +static inline const void *pfn_to_kaddr(unsigned long pfn)
> > +{
> > +     return (const void *)(((unsigned long)__va(pfn)) << PAGE_SHIFT);
>
> Any reason to do it this way rather than:
>
> +       return __va(pfn << PAGE_SHIFT);
>
> Seems to be equivalent and much cleaner?

I was afraid of changing the semantic in the original macro
which converts to a virtual address before shifting, instead
of shifting first, but you're right, I'm too cautious. I'll propose
the elegant solution from you & Christophe instead!

Yours,
Linus Walleij

Re: [PATCH] powerpc: Make virt_to_pfn() a static inline

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 14/08/2023 à 14:37, Michael Ellerman a écrit :
>> Linus Walleij <linus.walleij@linaro.org> writes:
>>> Making virt_to_pfn() a static inline taking a strongly typed
>>> (const void *) makes the contract of a passing a pointer of that
>>> type to the function explicit and exposes any misuse of the
>>> macro virt_to_pfn() acting polymorphic and accepting many types
>>> such as (void *), (unitptr_t) or (unsigned long) as arguments
>>> without warnings.
>> ...
>>> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
>>> index f2b6bf5687d0..9ee4b6d4a82a 100644
>>> --- a/arch/powerpc/include/asm/page.h
>>> +++ b/arch/powerpc/include/asm/page.h
>>> @@ -233,6 +224,25 @@ extern long long virt_phys_offset;
>>>   #endif
>>>   #endif
>>>   
>>> +#ifndef __ASSEMBLY__
>>> +static inline unsigned long virt_to_pfn(const void *kaddr)
>>> +{
>>> +	return __pa(kaddr) >> PAGE_SHIFT;
>>> +}
>>> +
>>> +static inline const void *pfn_to_kaddr(unsigned long pfn)
>>> +{
>>> +	return (const void *)(((unsigned long)__va(pfn)) << PAGE_SHIFT);
>> 
>> Any reason to do it this way rather than:
>> 
>> +       return __va(pfn << PAGE_SHIFT);
>
> Even cleaner:
>
> 	return __va(PFN_PHYS(pfn));

PFN_PHYS() includes a cast to phys_addr_t before shifting, so it's not
entirely equivalent.

But if phys_addr_t is larger than unsinged long then that cast is
important. Which makes me wonder how/if pfn_to_kaddr() has been working
until now for CONFIG_PHYS_ADDR_T_64BIT=y.

cheers

Re: [PATCH] powerpc: Make virt_to_pfn() a static inline

[Michael Ellerman]

Linus Walleij <linus.walleij@linaro.org> writes:

> Making virt_to_pfn() a static inline taking a strongly typed
> (const void *) makes the contract of a passing a pointer of that
> type to the function explicit and exposes any misuse of the
> macro virt_to_pfn() acting polymorphic and accepting many types
> such as (void *), (unitptr_t) or (unsigned long) as arguments
> without warnings.
>
> Move the virt_to_pfn() and related functions below the
> declaration of __pa() so it compiles.
>
> For symmetry do the same with pfn_to_kaddr().
>
> As the file is included right into the linker file, we need
> to surround the functions with ifndef __ASSEMBLY__ so we
> don't cause compilation errors.
>
> The conversion moreover exposes the fact that pmd_page_vaddr()
> was returning an unsigned long rather than a const void * as
> could be expected, so all the sites defining pmd_page_vaddr()
> had to be augmented as well.
...
> diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
> index 6a88bfdaa69b..a9515d3d7831 100644
> --- a/arch/powerpc/include/asm/pgtable.h
> +++ b/arch/powerpc/include/asm/pgtable.h
> @@ -60,9 +60,9 @@ static inline pgprot_t pte_pgprot(pte_t pte)
>  }
>  
>  #ifndef pmd_page_vaddr
> -static inline unsigned long pmd_page_vaddr(pmd_t pmd)
> +static inline const void *pmd_page_vaddr(pmd_t pmd)
>  {
> -	return ((unsigned long)__va(pmd_val(pmd) & ~PMD_MASKED_BITS));
> +	return (const void *)((unsigned long)__va(pmd_val(pmd) & ~PMD_MASKED_BITS));

This can also just be:

	return __va(pmd_val(pmd) & ~PMD_MASKED_BITS);

I've squashed that in.

cheers

Re: [PATCH] powerpc: Make virt_to_pfn() a static inline

[Linus Walleij]

On Tue, Aug 15, 2023 at 9:30 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:

> > -     return ((unsigned long)__va(pmd_val(pmd) & ~PMD_MASKED_BITS));
> > +     return (const void *)((unsigned long)__va(pmd_val(pmd) & ~PMD_MASKED_BITS));
>
> This can also just be:
>
>         return __va(pmd_val(pmd) & ~PMD_MASKED_BITS);
>
> I've squashed that in.

Oh you applied it, then I don't need to send revised versions, thanks Michael!

Yours,
Linus Walleij

Re: [PATCH] powerpc: Make virt_to_pfn() a static inline

[Michael Ellerman]

Linus Walleij <linus.walleij@linaro.org> writes:
> On Tue, Aug 15, 2023 at 9:30 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Linus Walleij <linus.walleij@linaro.org> writes:
>
>> > -     return ((unsigned long)__va(pmd_val(pmd) & ~PMD_MASKED_BITS));
>> > +     return (const void *)((unsigned long)__va(pmd_val(pmd) & ~PMD_MASKED_BITS));
>>
>> This can also just be:
>>
>>         return __va(pmd_val(pmd) & ~PMD_MASKED_BITS);
>>
>> I've squashed that in.
>
> Oh you applied it, then I don't need to send revised versions, thanks Michael!

Yeah, it's in my next-test, so I can still change it if needed for a day
or two. But if you're happy with me squashing those changes in then
that's easy, no need to send a v2.

cheers

Re: [PATCH] powerpc: Make virt_to_pfn() a static inline

[Michael Ellerman]

On Wed, 09 Aug 2023 10:07:13 +0200, Linus Walleij wrote:
> Making virt_to_pfn() a static inline taking a strongly typed
> (const void *) makes the contract of a passing a pointer of that
> type to the function explicit and exposes any misuse of the
> macro virt_to_pfn() acting polymorphic and accepting many types
> such as (void *), (unitptr_t) or (unsigned long) as arguments
> without warnings.
> 
> [...]

Applied to powerpc/next.

[1/1] powerpc: Make virt_to_pfn() a static inline
      https://git.kernel.org/powerpc/c/58b6fed89ab0f602de0d143c617c29c3d4c67429

cheers