powerpc: Make virt_to_pfn() a static inline
Commit Message
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(-)
---
base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
change-id: 20230808-virt-to-phys-powerpc-634cf7acd39a
Best regards,
Comments
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
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
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
@@ -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
@@ -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)))
@@ -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
@@ -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
@@ -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);