| Message ID | 20240903151437.1002990-4-vincenzo.frascino@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vdso: Use only headers from the vdso/ namespace | expand |
On Tue, Sep 3, 2024, at 15:14, Vincenzo Frascino wrote: > diff --git a/arch/x86/include/asm/vdso/page.h b/arch/x86/include/asm/vdso/page.h > new file mode 100644 > index 000000000000..b0af8fbef27c > --- /dev/null > +++ b/arch/x86/include/asm/vdso/page.h > @@ -0,0 +1,15 @@ > + > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __ASM_VDSO_PAGE_H > +#define __ASM_VDSO_PAGE_H > + > +#ifndef __ASSEMBLY__ > + > +#include <asm/page_types.h> > + > +#define VDSO_PAGE_MASK PAGE_MASK > +#define VDSO_PAGE_SIZE PAGE_SIZE > + > +#endif /* !__ASSEMBLY__ */ > + > +#endif /* __ASM_VDSO_PAGE_H */ I don't get this one: the x86 asm/page_types.h still includes other headers outside of the vdso namespace, but you seem to only need these two definitions that are the same across everything. Why not put PAGE_MASK and PAGE_SIZE into a global vdso/page.h header? I did spend a lot of time a few months ago ensuring that we can have a single definition for all architectures based on CONFIG_PAGE_SHIFT, so all the extra copies should just go away. Arnd
Le 04/09/2024 à 16:52, Arnd Bergmann a écrit : > On Tue, Sep 3, 2024, at 15:14, Vincenzo Frascino wrote: > >> diff --git a/arch/x86/include/asm/vdso/page.h b/arch/x86/include/asm/vdso/page.h >> new file mode 100644 >> index 000000000000..b0af8fbef27c >> --- /dev/null >> +++ b/arch/x86/include/asm/vdso/page.h >> @@ -0,0 +1,15 @@ >> + >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +#ifndef __ASM_VDSO_PAGE_H >> +#define __ASM_VDSO_PAGE_H >> + >> +#ifndef __ASSEMBLY__ >> + >> +#include <asm/page_types.h> >> + >> +#define VDSO_PAGE_MASK PAGE_MASK >> +#define VDSO_PAGE_SIZE PAGE_SIZE >> + >> +#endif /* !__ASSEMBLY__ */ >> + >> +#endif /* __ASM_VDSO_PAGE_H */ > > I don't get this one: the x86 asm/page_types.h still includes other > headers outside of the vdso namespace, but you seem to only need these > two definitions that are the same across everything. > > Why not put PAGE_MASK and PAGE_SIZE into a global vdso/page.h > header? I did spend a lot of time a few months ago ensuring that > we can have a single definition for all architectures based on > CONFIG_PAGE_SHIFT, so all the extra copies should just go away. > Just wondering, after looking at x86, powerpc and arm64, is there any difference between: X86,ARM64: #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT) #define PAGE_MASK (~(PAGE_SIZE-1)) POWERPC: #define PAGE_SIZE (ASM_CONST(1) << PAGE_SHIFT) /* * Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if we * assign PAGE_MASK to a larger type it gets extended the way we want * (i.e. with 1s in the high bits) */ #define PAGE_MASK (~((1 << PAGE_SHIFT) - 1)) Which one should be taken in vdso/page.h ? Christophe
Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit : > The VDSO implementation includes headers from outside of the > vdso/ namespace. > > Introduce asm/vdso/page.h to make sure that the generic library > uses only the allowed namespace. > > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Jason A. Donenfeld <Jason@zx2c4.com> > Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> > --- > arch/x86/include/asm/vdso/page.h | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > create mode 100644 arch/x86/include/asm/vdso/page.h > > diff --git a/arch/x86/include/asm/vdso/page.h b/arch/x86/include/asm/vdso/page.h > new file mode 100644 > index 000000000000..b0af8fbef27c > --- /dev/null > +++ b/arch/x86/include/asm/vdso/page.h > @@ -0,0 +1,15 @@ > + > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __ASM_VDSO_PAGE_H > +#define __ASM_VDSO_PAGE_H > + > +#ifndef __ASSEMBLY__ > + > +#include <asm/page_types.h> > + > +#define VDSO_PAGE_MASK PAGE_MASK > +#define VDSO_PAGE_SIZE PAGE_SIZE Same, I don't think we need those macros to duplicate PAGE_SIZE and PAGE_MASK in each and every architecture. The best would be to use CONFIG_PAGE_SHIFT which is defined the same way on every architecture and refactor PAGE_SIZE and PAGE_MASK and get rid of all arch specific definitions of PAGE_SIZE and PAGE_MASK. > + > +#endif /* !__ASSEMBLY__ */ > + > +#endif /* __ASM_VDSO_PAGE_H */
Hi Arnd, On 04/09/2024 15:52, Arnd Bergmann wrote: > On Tue, Sep 3, 2024, at 15:14, Vincenzo Frascino wrote: > ... >> + >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +#ifndef __ASM_VDSO_PAGE_H >> +#define __ASM_VDSO_PAGE_H >> + >> +#ifndef __ASSEMBLY__ >> + >> +#include <asm/page_types.h> >> + >> +#define VDSO_PAGE_MASK PAGE_MASK >> +#define VDSO_PAGE_SIZE PAGE_SIZE >> + >> +#endif /* !__ASSEMBLY__ */ >> + >> +#endif /* __ASM_VDSO_PAGE_H */ > > I don't get this one: the x86 asm/page_types.h still includes other > headers outside of the vdso namespace, but you seem to only need these > two definitions that are the same across everything. > > Why not put PAGE_MASK and PAGE_SIZE into a global vdso/page.h > header? I did spend a lot of time a few months ago ensuring that > we can have a single definition for all architectures based on > CONFIG_PAGE_SHIFT, so all the extra copies should just go away. > > Arnd Looking at the definition of PAGE_SIZE and PAGE_MASK for each architecture they all depend on CONFIG_PAGE_SHIFT but they are slightly different, e.g.: x86: #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT) powerpc: #define PAGE_SIZE (ASM_CONST(1) << PAGE_SHIFT) hence I left to the architecture the responsibility of redefining the constants for the VSDO. As a long term plan I would like to simplify the code and have a single definition as you are saying in vdso/page.h for both the macros. But my preference would be to post it as a separate series so that I can focus more on validating it properly.
On 04/09/2024 16:05, Christophe Leroy wrote: > > > Le 04/09/2024 à 16:52, Arnd Bergmann a écrit : >> On Tue, Sep 3, 2024, at 15:14, Vincenzo Frascino wrote: >> ... >>> + >>> +#ifndef __ASSEMBLY__ >>> + >>> +#include <asm/page_types.h> >>> + >>> +#define VDSO_PAGE_MASK PAGE_MASK >>> +#define VDSO_PAGE_SIZE PAGE_SIZE >>> + >>> +#endif /* !__ASSEMBLY__ */ >>> + >>> +#endif /* __ASM_VDSO_PAGE_H */ >> >> I don't get this one: the x86 asm/page_types.h still includes other >> headers outside of the vdso namespace, but you seem to only need these >> two definitions that are the same across everything. >> >> Why not put PAGE_MASK and PAGE_SIZE into a global vdso/page.h >> header? I did spend a lot of time a few months ago ensuring that >> we can have a single definition for all architectures based on >> CONFIG_PAGE_SHIFT, so all the extra copies should just go away. >> > > Just wondering, after looking at x86, powerpc and arm64, is there any difference > between: > > X86,ARM64: > #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT) > #define PAGE_MASK (~(PAGE_SIZE-1)) > > POWERPC: > #define PAGE_SIZE (ASM_CONST(1) << PAGE_SHIFT) > /* > * Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if we > * assign PAGE_MASK to a larger type it gets extended the way we want > * (i.e. with 1s in the high bits) > */ > #define PAGE_MASK (~((1 << PAGE_SHIFT) - 1)) > > > Which one should be taken in vdso/page.h ? > I am not sure either on this point. That's the main reason why I proposed an indirection for the definitions. > Christophe
Hi Arnd, Le 06/09/2024 à 21:19, Arnd Bergmann a écrit : > On Fri, Sep 6, 2024, at 11:20, Vincenzo Frascino wrote: >> On 04/09/2024 15:52, Arnd Bergmann wrote: >>> On Tue, Sep 3, 2024, at 15:14, Vincenzo Frascino wrote: >> Looking at the definition of PAGE_SIZE and PAGE_MASK for each architecture they >> all depend on CONFIG_PAGE_SHIFT but they are slightly different, e.g.: >> >> x86: >> #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT) >> >> powerpc: >> #define PAGE_SIZE (ASM_CONST(1) << PAGE_SHIFT) >> >> hence I left to the architecture the responsibility of redefining the constants >> for the VSDO. > > ASM_CONST() is a powerpc-specific macro that is defined the > same way as _AC(). We could probably just replace all ASM_CONST() > as a cleanup, but for this purpose, just remove the custom PAGE_SIZE > and PAGE_SHIFT macros. This can be a single patch fro all > architectures. > I'm not worried about _AC versus ASM_CONST, but I am by the 1UL versus 1. The two functions below don't provide the same result: #define PAGE_SIZE (1 << 12) #define PAGE_MASK (~(PAGE_SIZE - 1)) unsigned long long f(unsigned long long x) { return x & PAGE_MASK; } #define PAGE_SIZE_2 (1UL << 12) #define PAGE_MASK_2 (~(PAGE_SIZE_2 - 1)) unsigned long long g(unsigned long long x) { return x & PAGE_MASK_2; } 00000000 <f>: 0: 54 84 00 26 clrrwi r4,r4,12 4: 4e 80 00 20 blr 00000008 <g>: 8: 54 84 00 26 clrrwi r4,r4,12 c: 38 60 00 00 li r3,0 10: 4e 80 00 20 blr This can be a problem on 32 bits platforms with 64 bits phys_addr_t Christophe
On Fri, Sep 6, 2024, at 11:20, Vincenzo Frascino wrote: > On 04/09/2024 15:52, Arnd Bergmann wrote: >> On Tue, Sep 3, 2024, at 15:14, Vincenzo Frascino wrote: > Looking at the definition of PAGE_SIZE and PAGE_MASK for each architecture they > all depend on CONFIG_PAGE_SHIFT but they are slightly different, e.g.: > > x86: > #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT) > > powerpc: > #define PAGE_SIZE (ASM_CONST(1) << PAGE_SHIFT) > > hence I left to the architecture the responsibility of redefining the constants > for the VSDO. ASM_CONST() is a powerpc-specific macro that is defined the same way as _AC(). We could probably just replace all ASM_CONST() as a cleanup, but for this purpose, just remove the custom PAGE_SIZE and PAGE_SHIFT macros. This can be a single patch fro all architectures. Arnd
On Fri, Sep 6, 2024, at 18:40, Christophe Leroy wrote: > Le 06/09/2024 à 21:19, Arnd Bergmann a écrit : >> On Fri, Sep 6, 2024, at 11:20, Vincenzo Frascino wrote: >>> Looking at the definition of PAGE_SIZE and PAGE_MASK for each architecture they >>> all depend on CONFIG_PAGE_SHIFT but they are slightly different, e.g.: >>> >>> x86: >>> #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT) >>> >>> powerpc: >>> #define PAGE_SIZE (ASM_CONST(1) << PAGE_SHIFT) >>> >>> hence I left to the architecture the responsibility of redefining the constants >>> for the VSDO. >> >> ASM_CONST() is a powerpc-specific macro that is defined the >> same way as _AC(). We could probably just replace all ASM_CONST() >> as a cleanup, but for this purpose, just remove the custom PAGE_SIZE >> and PAGE_SHIFT macros. This can be a single patch fro all >> architectures. >> > > I'm not worried about _AC versus ASM_CONST, but I am by the 1UL versus 1. > > > This can be a problem on 32 bits platforms with 64 bits phys_addr_t But that would already be a bug if anything used this, however none of them do. The only instance of an open-coded #define PAGE_SIZE (1 << PAGE_SHIFT) is from openrisc, but that is only used inside of __ASSEMBLY__, for the same effect as _AC(). Arnd
Le 08/09/2024 à 22:48, Arnd Bergmann a écrit : > On Fri, Sep 6, 2024, at 18:40, Christophe Leroy wrote: >> Le 06/09/2024 à 21:19, Arnd Bergmann a écrit : >>> On Fri, Sep 6, 2024, at 11:20, Vincenzo Frascino wrote: > >>>> Looking at the definition of PAGE_SIZE and PAGE_MASK for each architecture they >>>> all depend on CONFIG_PAGE_SHIFT but they are slightly different, e.g.: >>>> >>>> x86: >>>> #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT) >>>> >>>> powerpc: >>>> #define PAGE_SIZE (ASM_CONST(1) << PAGE_SHIFT) >>>> >>>> hence I left to the architecture the responsibility of redefining the constants >>>> for the VSDO. >>> >>> ASM_CONST() is a powerpc-specific macro that is defined the >>> same way as _AC(). We could probably just replace all ASM_CONST() >>> as a cleanup, but for this purpose, just remove the custom PAGE_SIZE >>> and PAGE_SHIFT macros. This can be a single patch fro all >>> architectures. >>> >> >> I'm not worried about _AC versus ASM_CONST, but I am by the 1UL versus 1. >> >> >> This can be a problem on 32 bits platforms with 64 bits phys_addr_t > > But that would already be a bug if anything used this, however > none of them do. The only instance of an open-coded > > #define PAGE_SIZE (1 << PAGE_SHIFT) > > is from openrisc, but that is only used inside of __ASSEMBLY__, for > the same effect as _AC(). Maybe I was not clear enough. The problem is not with PAGE_SHIFT but with PAGE_MASK, and that's what I show with my exemple. If take the definition from ARM64 (which is the same as several other artchitectures): #define PAGE_SIZE (_AC(1, UL) << PAGE_SHIFT) #define PAGE_MASK (~(PAGE_SIZE-1)) PAGE_SHIFT is 12 PAGE_SIZE is then 4096 UL PAGE_MASK is then 0xfffff000 UL So if I take the probe() in drivers/uio/uio_pci_generic.c, it has: uiomem->addr = r->start & PAGE_MASK; uiomem->addr is a phys_addr_t r->start is a ressource_size_t hence a phys_addr_t And phys_addr_t is defined as: #ifdef CONFIG_PHYS_ADDR_T_64BIT typedef u64 phys_addr_t; #else typedef u32 phys_addr_t; #endif On a 32 bits platform, UL is unsigned 32 bits, so the r->start & PAGE_MASK will and r->start with 0x00000000fffff000 That is wrong. That's the reason why powerpc does not define PAGE_MASK like ARM64 but defines PAGE_MASK as: (~((1 << PAGE_SHIFT) - 1)) When using 1 instead of 1UL, PAGE_MASK is still 0xfffff000 but it is a signed constant, so when it is anded with an u64, it gets signed-extended to 0xfffffffffffff000 which gives the expected result. That's what I wanted to illustrate with the exemple in my previous message. The function f() was using the signed PAGE_MASK while function g() was using the unsigned PAGE_MASK: 00000000 <f>: 0: 54 84 00 26 clrrwi r4,r4,12 4: 4e 80 00 20 blr 00000008 <g>: 8: 54 84 00 26 clrrwi r4,r4,12 c: 38 60 00 00 li r3,0 10: 4e 80 00 20 blr Function f() returns the given u64 value with the 12 lowest bits cleared Function g() returns the given u64 value with the 12 lowest bits cleared and the 32 highest bits cleared as well, which is unexpected. Christophe
On Tue, Sep 10, 2024, at 12:28, Christophe Leroy wrote: > Le 08/09/2024 à 22:48, Arnd Bergmann a écrit : >> On Fri, Sep 6, 2024, at 18:40, Christophe Leroy wrote: > > uiomem->addr is a phys_addr_t > r->start is a ressource_size_t hence a phys_addr_t > > And phys_addr_t is defined as: > > #ifdef CONFIG_PHYS_ADDR_T_64BIT > typedef u64 phys_addr_t; > #else > typedef u32 phys_addr_t; > #endif > > On a 32 bits platform, UL is unsigned 32 bits, so the r->start & > PAGE_MASK will and r->start with 0x00000000fffff000 > > That is wrong. Right, I see. So out of the five 32-bit architectures with a 64-bit phys_addr_t, arc seems to ignore this problem, x86 has a separate PHYSICAL_PAGE_MASK for its internal uses and the other three have the definition you mention as > (~((1 << PAGE_SHIFT) - 1)) And this is only documented properly on powerpc. How about making the common definition this? #ifdef CONFIG_PHYS_ADDR_T_64BIT #define PAGE_MASK (~((1 << PAGE_SHIFT) - 1)) #else #define PAGE_MASK (~(PAGE_SIZE-1)) #endif That keeps it unchanged for everything other than arc and x86-32, and hopefully fixes the currently behavior on those two. Arnd
diff --git a/arch/x86/include/asm/vdso/page.h b/arch/x86/include/asm/vdso/page.h new file mode 100644 index 000000000000..b0af8fbef27c --- /dev/null +++ b/arch/x86/include/asm/vdso/page.h @@ -0,0 +1,15 @@ + +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __ASM_VDSO_PAGE_H +#define __ASM_VDSO_PAGE_H + +#ifndef __ASSEMBLY__ + +#include <asm/page_types.h> + +#define VDSO_PAGE_MASK PAGE_MASK +#define VDSO_PAGE_SIZE PAGE_SIZE + +#endif /* !__ASSEMBLY__ */ + +#endif /* __ASM_VDSO_PAGE_H */
The VDSO implementation includes headers from outside of the vdso/ namespace. Introduce asm/vdso/page.h to make sure that the generic library uses only the allowed namespace. Cc: Andy Lutomirski <luto@kernel.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Jason A. Donenfeld <Jason@zx2c4.com> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> --- arch/x86/include/asm/vdso/page.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 arch/x86/include/asm/vdso/page.h