Re: [PATCH] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK

[Vincenzo Frascino <vincenzo.frascino@arm.com>]

Hi Christophe,

On 27/08/2024 18:38, LEROY Christophe wrote:
> Hi Vicenzo,
> 
> Le 27/08/2024 à 18:05, Vincenzo Frascino a écrit :
>> Hi Christophe,
>>
>> On 27/08/2024 11:49, Christophe Leroy wrote:
>>
>> ...
>>
>>
>>
>> I agree with Arnd here. uapi/linux/mman.h can cause us problems in the long run.
>>
>> I am attaching a patch to provide my view on how to minimize the headers
>> included and use only the vdso/ namespace. Please, before using the code,
>> consider that I conducted very limited testing.
>>
>> Note: It should apply clean on Jason's tree.
>>
>> Let me know your thoughts.
>>
> 
> Your patch looks nice, maybe a bit too much. For instance getrandom.c 
> can include directly asm/vdso/page.h instead of creating vdso/page.h
> 
> Or create a vdso/page.h that only use CONFIG_PAGE_SHIFT and doesn't 
> include anything from architectures.
> 

IMHO there should be only one place per architecture where PAGE_SIZE and
PAGE_MASK are defined. This makes sure that if there is a problem, we do not
have multiple places to look into.

The indirection helps to keep consistent the namespace and allows for future
extension. Similar logic has been used during my original vDSO headers
definition and implementation.

> We should also keep PROT_READ and PROT_WRITE in getrandom.c , that's 
> better for readability. Same for MAP_DROPPABLE | MAP_ANONYMOUS. I can't 
> see the benefit of hiding them in a header.
> 

The idea is not to make the code unreadable but to defer to the architecture the
decision of prot and flags avoiding the inclusion of headers coming from the
uapi namespace.

> I can't see which header provides you with min_t() or ARRAY_SIZE().
> 

Good point, this needs to be addressed by my patch, I will extend it, do some
more testing and post it again next week.

> I think you should also work on removing headers included by 
> arch/x86/include/asm/vdso/gettimeofday.h which is included by 
> include/vdso/datapage.h :
> 
>    #include <uapi/linux/time.h>
>    #include <asm/vgtod.h>
>    #include <asm/vvar.h>
>    #include <asm/unistd.h>
>    #include <asm/msr.h>
>    #include <asm/pvclock.h>
>    #include <clocksource/hyperv_timer.h>
> 
> As a comparison, the one from powerpc only includes the following one so 
> it pulls a lot less non-vdso headers:
> 
>    #include <asm/vdso/timebase.h>
>    #include <asm/barrier.h>
>    #include <asm/unistd.h>
>    #include <uapi/linux/time.h>
> 
> Christophe

This does not seem a concern, in fact I believe that the generic vDSO library
should not mandate to the architecture how to organize headers. As far as the
requirements are satisfied each architecture should be able to define its own
naming and conventions independently.

-- 
Regards,
Vincenzo