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
