Re: [PATCH] asm-generic: provide generic page_to_phys and phys_to_page implementations

[Arnd Bergmann]

On Wed, Oct 9, 2024, at 11:43, Christoph Hellwig wrote:
> page_to_phys is duplicated by all architectures, and from some strange
> reason placed in <asm/io.h> where it doesn't fit at all.
>
> phys_to_page is only provided by a few architectures despite having a lot
> of open coded users.
>
> Provide generic versions in <asm-generic/memory_model.h> to make these
> helpers more easily usable.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

This is clearly a good idea, and I'm happy to take that through
the asm-generic tree if there are no complaints.

Do you have any other patches that depend on it?

> -/*
> - * Change "struct page" to physical address.
> - */
> -static inline phys_addr_t page_to_phys(struct page *page)
> -{
> -	unsigned long pfn = page_to_pfn(page);
> -
> -	WARN_ON(IS_ENABLED(CONFIG_DEBUG_VIRTUAL) && !pfn_valid(pfn));
> -
> -	return PFN_PHYS(pfn);
> -}

This part is technically a change in behavior, not sure how
much anyone cares.

> diff --git a/include/asm-generic/memory_model.h 
> b/include/asm-generic/memory_model.h
> index 6796abe1900e30..3d51066f88f819 100644
> --- a/include/asm-generic/memory_model.h
> +++ b/include/asm-generic/memory_model.h
> @@ -64,6 +64,9 @@ static inline int pfn_valid(unsigned long pfn)
>  #define page_to_pfn __page_to_pfn
>  #define pfn_to_page __pfn_to_page
> 
> +#define page_to_phys(page)	__pfn_to_phys(page_to_pfn(page))
> +#define phys_to_page(phys)	pfn_to_page(__phys_to_pfn(phys))

I think we should try to have a little fewer nested macros
to evaluate here, right now this ends up expanding
__pfn_to_phys, PFN_PHYS, PAGE_SHIFT, CONFIG_PAGE_SHIFT,
page_to_pfn and __page_to_pfn. While the behavior is fine,
modern gcc versions list all of those in an warning message
if someone passes the wrong arguments.

Changing the two macros above into inline functions
would help as well, but may cause other problems.

On a related note, it would be even better if we could come
up with a generic definition for either __pa/__va or
virt_to_phys/phys_to_virt. Most architectures define one
of the two pairs in terms of the other, which leads to
confusion with header include order.

      Arnd