Re: [PATCH 5/9] x86/clear_pages: add clear_pages()

[Ankur Arora]

Peter Zijlstra <peterz@xxxxxxxxxxxxx> writes:

> On Sun, Apr 02, 2023 at 10:22:29PM -0700, Ankur Arora wrote:
>> Add clear_pages() and define the ancillary clear_user_pages().
>>
>> Signed-off-by: Ankur Arora <ankur.a.arora@xxxxxxxxxx>
>> ---
>>  arch/x86/include/asm/page.h    | 6 ++++++
>>  arch/x86/include/asm/page_32.h | 6 ++++++
>>  arch/x86/include/asm/page_64.h | 9 +++++++--
>>  3 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/page.h b/arch/x86/include/asm/page.h
>> index d18e5c332cb9..03e3c69fc427 100644
>> --- a/arch/x86/include/asm/page.h
>> +++ b/arch/x86/include/asm/page.h
>> @@ -28,6 +28,12 @@ static inline void clear_user_page(void *page, unsigned long vaddr,
>>  	clear_page(page);
>>  }
>>
>> +static inline void clear_user_pages(void *page, unsigned long vaddr,
>> +				    struct page *pg, unsigned int nsubpages)
>> +{
>> +	clear_pages(page, nsubpages);
>> +}
>
> This seems dodgy, clear_user* has slightly different semantics. It needs
> the access_ok() and stac/clac thing on at the very least.

That can't be right. On x86, clear_user_page(), copy_user_page() (and
now the multi-page versions) only write to kernel maps of user pages.
That's why they can skip the access_ok(), stac/clac or uacess
exception handling.

>From core-api/cachetlb.rst:

  ``void copy_user_page(void *to, void *from, unsigned long addr, struct page *page)``
  ``void clear_user_page(void *to, unsigned long addr, struct page *page)``

        These two routines store data in user anonymous or COW
        pages.  It allows a port to efficiently avoid D-cache alias
        issues between userspace and the kernel.

        For example, a port may temporarily map 'from' and 'to' to
        kernel virtual addresses during the copy.  The virtual address
        for these two pages is chosen in such a way that the kernel
        load/store instructions happen to virtual addresses which are
        of the same "color" as the user mapping of the page.  Sparc64
        for example, uses this technique.

        The 'addr' parameter tells the virtual address where the
        user will ultimately have this page mapped, and the 'page'
        parameter gives a pointer to the struct page of the target.

The naming OTOH does seems dodgy. Especially because as you say it
suggests semantics similar to clear_user() etc.

On x86, I think it is definitely a mistake for clear_huge_page() to be
calling clear_user_page*() (especially given that it is getting the
kernel map.) Will fix that.

Even for non-x86, I see just two users in common code:
  highmem.h: copy_user_highpage(), clear_user_highpage()
  fs/dax.c: copy_cow_page_dax()

All of them do a kmap_atomic() so there's really no "may" as documented
above:
        For example, a port may temporarily map 'from' and 'to' to
        kernel virtual addresses during the copy.  The virtual address

Maybe a name change is warranted, if nothing else?

>> +
>>  static inline void copy_user_page(void *to, void *from, unsigned long vaddr,
>>  				  struct page *topage)
>>  {
>> diff --git a/arch/x86/include/asm/page_32.h b/arch/x86/include/asm/page_32.h
>> index 580d71aca65a..3523d1150cfc 100644
>> --- a/arch/x86/include/asm/page_32.h
>> +++ b/arch/x86/include/asm/page_32.h
>> @@ -22,6 +22,12 @@ static inline void clear_page(void *page)
>>  	memset(page, 0, PAGE_SIZE);
>>  }
>>
>> +static inline void clear_pages(void *page, unsigned int nsubpages)
>> +{
>> +	for (int i = 0; i < nsubpages; i++)
>> +		clear_page(page + i * PAGE_SIZE);
>
> cond_resched() ?

Missed that. Thanks. Will fix.

--
ankur