linux-kernel - Re: [PATCH] asm-generic: provide generic page_to_phys and phys_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241010070342.GB6674@lst.de>
Date: Thu, 10 Oct 2024 09:03:42 +0200
From: Christoph Hellwig <hch@....de>
To: Arnd Bergmann <arnd@...db.de>
Cc: Christoph Hellwig <hch@....de>, linux-alpha@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-snps-arc@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org,
	"linux-csky@...r.kernel.org" <linux-csky@...r.kernel.org>,
	linux-hexagon@...r.kernel.org, loongarch@...ts.linux.dev,
	linux-m68k@...ts.linux-m68k.org, linux-mips@...r.kernel.org,
	"linux-openrisc@...r.kernel.org" <linux-openrisc@...r.kernel.org>,
	linux-parisc@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
	linux-riscv@...ts.infradead.org, linux-s390@...r.kernel.org,
	linux-sh@...r.kernel.org, sparclinux@...r.kernel.org,
	linux-um@...ts.infradead.org,
	Linux-Arch <linux-arch@...r.kernel.org>,
	Christophe Leroy <christophe.leroy@...roup.eu>
Subject: Re: [PATCH] asm-generic: provide generic page_to_phys and
 phys_to_page implementations

On Wed, Oct 09, 2024 at 02:06:27PM +0000, Arnd Bergmann wrote:
> 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?

Well, I have new code that would benefit from these helpers, but just
open coding it for now and then doing a swipe to clean that up later
together with the existing open coded versions is easy enough.

> > -/*
> > - * 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.

Well, the only other comment to the patch so far mentioned it.
It also feels like a useful check, but I'm a bit worried about
it triggering in various new places.  Although that's just with
CONFIG_DEBUG_VIRTUAL and probably points to real bugs, so maybe
adding it everywhere is a good idea.

> > +#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.

Doing them as inlines seems useful to me, let me throw that at
the buildbot and see if anything explodes.

> 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.

Agreed, but that's a separate project.