linux-kernel - Re: [PATCH] asm-generic: introduce io_stop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b8245b7f-9c94-4715-6e9d-467b3a4cb5e1@huawei.com>
Date:   Mon, 20 Dec 2021 20:23:54 +0800
From:   Xiongfeng Wang <wangxiongfeng2@...wei.com>
To:     Catalin Marinas <catalin.marinas@....com>
CC:     <will@...nel.org>, <mark.rutland@....com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <moyufeng@...wei.com>,
        <linux-arch@...r.kernel.org>
Subject: Re: [PATCH] asm-generic: introduce io_stop_wc() and add
 implementation for ARM64

Hi, Catalin

On 2021/12/17 19:59, Catalin Marinas wrote:
> On Fri, Dec 17, 2021 at 04:56:11PM +0800, Xiongfeng Wang wrote:
>> For memory accesses with Normal-Non Cacheable attributes, the CPU may do
> 
> You may want to mention "arm64 Normal Non-Cacheable" as other
> architectures have a different meaning of NC.
> 
>> write combining. But in some situation, this is bad for the performance
>> because the prior access may wait too long just to be merged.
>>
>> We introduce io_stop_wc() to prevent the Normal-NC memory accesses before
>> this instruction to be merged with memory accesses after this
>> instruction.
>>
>> We add implementation for ARM64 using DGH instruction and provide NOP
>> implementation for other architectures.
>>
>> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@...wei.com>
>> ---
>>  Documentation/memory-barriers.txt |  9 +++++++++
>>  arch/arm64/include/asm/barrier.h  |  9 +++++++++
>>  include/asm-generic/barrier.h     | 11 +++++++++++
>>  3 files changed, 29 insertions(+)
>>
>> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
>> index 7367ada13208..b868b51b1801 100644
>> --- a/Documentation/memory-barriers.txt
>> +++ b/Documentation/memory-barriers.txt
>> @@ -1950,6 +1950,15 @@ There are some more advanced barrier functions:
>>       For load from persistent memory, existing read memory barriers are sufficient
>>       to ensure read ordering.
>>  
>> + (*) io_stop_wc();
>> +
>> +     For memory accesses with Normal-Non Cacheable attributes (e.g. those
>> +     returned by ioremap_wc()), the CPU may do write combining. But in some
>> +     situation, this is bad for the performance because the prior access may
>> +     wait too long just to be merged. io_stop_wc() can be used to prevent
>> +     merging memory accesses with Normal-Non Cacheable attributes before this
>> +     instruction with any memory accesses appearing after this instruction.
> 
> I'm fine with the patch in general but the comment here and in
> asm-generic/barrier.h should avoid Normal Non-Cacheable as that's an
> arm-specific term. Looking at Documentation/driver-api/device-io.rst, we
> could simply say "write-combining". Something like:
> 
>      For memory accesses with write-combining attributes (e.g. those
>      returned by ioremap_wc()), the CPU may wait for prior accesses to
>      be merged with subsequent ones. io_stop_wc() can be used to prevent
>      the merging of write-combining memory accesses before this macro
>      with those after it when such wait has performance implications.
> 
> (feel free to rephrase it but avoid Normal NC here)

Thanks for your advice! I will send another version soon.

Thanks,
Xiongfeng

> 
>> +
>>  ===============================
>>  IMPLICIT KERNEL MEMORY BARRIERS
>>  ===============================
>> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
>> index 1c5a00598458..62217be36217 100644
>> --- a/arch/arm64/include/asm/barrier.h
>> +++ b/arch/arm64/include/asm/barrier.h
>> @@ -26,6 +26,14 @@
>>  #define __tsb_csync()	asm volatile("hint #18" : : : "memory")
>>  #define csdb()		asm volatile("hint #20" : : : "memory")
>>  
>> +/*
>> + * Data Gathering Hint:
>> + * This instruction prevents merging memory accesses with Normal-NC or
>> + * Device-GRE attributes before the hint instruction with any memory accesses
>> + * appearing after the hint instruction.
>> + */
>> +#define dgh()		asm volatile("hint #6" : : : "memory")
> 
> This is fine, arm-specific code.
> 
>> +
>>  #ifdef CONFIG_ARM64_PSEUDO_NMI
>>  #define pmr_sync()						\
>>  	do {							\
>> @@ -46,6 +54,7 @@
>>  #define dma_rmb()	dmb(oshld)
>>  #define dma_wmb()	dmb(oshst)
>>  
>> +#define io_stop_wc()	dgh()
>>  
>>  #define tsb_csync()								\
>>  	do {									\
>> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
>> index 640f09479bdf..083be6d34cb9 100644
>> --- a/include/asm-generic/barrier.h
>> +++ b/include/asm-generic/barrier.h
>> @@ -251,5 +251,16 @@ do {									\
>>  #define pmem_wmb()	wmb()
>>  #endif
>>  
>> +/*
>> + * ioremap_wc() maps I/O memory as memory with Normal-Non Cacheable attributes.
>> + * The CPU may do write combining for this kind of memory access. io_stop_wc()
>> + * prevents merging memory accesses with Normal-Non Cacheable attributes
>> + * before this instruction with any memory accesses appearing after this
>> + * instruction.
> 
> Please change this as well along the lines of my comment above.
> 
>> + */
>> +#ifndef io_stop_wc
>> +#define io_stop_wc do { } while (0)
>> +#endif
>> +
>>  #endif /* !__ASSEMBLY__ */
>>  #endif /* __ASM_GENERIC_BARRIER_H */
> 
> Thanks.
>