Re: [PATCH 3/9] x86: vdso: Introduce asm/vdso/page.h

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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