linux-kernel - Re: [PATCH v2 2/2] firmware: arm_scmi: Support 'reg-io-width' property for shared memory

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <345aca0f-12f0-4a66-a760-3b8524fda7fe@broadcom.com>
Date: Fri, 16 Aug 2024 10:39:42 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Cristian Marussi <cristian.marussi@....com>
Cc: linux-kernel@...r.kernel.org, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Sudeep Holla <sudeep.holla@....com>,
 "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
 <devicetree@...r.kernel.org>,
 "open list:SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE"
 <arm-scmi@...r.kernel.org>,
 "moderated list:SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE"
 <linux-arm-kernel@...ts.infradead.org>, justin.chen@...adcom.com,
 opendmb@...il.com, kapil.hali@...adcom.com,
 bcm-kernel-feedback-list@...adcom.com
Subject: Re: [PATCH v2 2/2] firmware: arm_scmi: Support 'reg-io-width'
 property for shared memory

On 8/16/24 10:02, Cristian Marussi wrote:
> On Tue, Aug 13, 2024 at 11:07:47AM -0700, Florian Fainelli wrote:
>> Some shared memory areas might only support a certain access width,
>> such as 32-bit, which memcpy_{from,to}_io() does not adhere to at least
>> on ARM64 by making both 8-bit and 64-bit accesses to such memory.
>>
>> Update the shmem layer to support reading from and writing to such
>> shared memory area using the specified I/O width in the Device Tree. The
>> various transport layers making use of the shmem.c code are updated
>> accordingly to pass the I/O accessors that they store.
>>
> 
> Hi Florian,
> 
> I gave it ago at this on a JUNO regarding the mailbox/shmem transport
> without any issue. I'll have a go later on an OPTEE/shmem scenario too.
> 
> This looks fundamentally good to me, since you moved all ops setup at
> setup time and you keep the pointers per-channel instead of global...

Thanks!

> 
> A few remarks down below.
> 
>> Signed-off-by: Florian Fainelli <florian.fainelli@...adcom.com>

[snip]

> 
> ... I may be missing a lot here...bear with me...so...
> 
> ... AFAIU, as suggested by Peng, you moved away from iowrite##s and ioread##s
> in favour of __raw_write/read##w so as to avoid the implicit barriers on each
> loop iteration...(I suppose..)
> 
> ...but should we place some sort of final io barrier (similarly to iowrite)
> at the end of the loop ?

There is no leading or trailing barrier with the ARM64 
memcpy_{to,from}io routines which is why this was carried forward as-is.

There is an implicit barrier with the iowrite32() in the tx_prepare(), 
so I suppose we are somewhat safe on that part. Likewise with the 
fetch_response()/fetch_notification() we have an implicit barrier within 
the ioread32() and then there is a data dependency since we ought to be 
consuming the response/notification.

For ARM 32-bit the implementation uses readb()/writeb() which does 
include barriers.

> 
>> +static inline void shmem_memcpy_toio##s(volatile void __iomem *to,	\
>> +					const void *from,		\
>> +					size_t count)			\
>> +{									\
>> +	while (count) {							\
>> +		__raw_write##w(*(u##s *)from, to);			\
>> +		from += amt;						\
>> +		to += amt;						\
>> +		count -= amt;						\
>> +	}								\
>> +}		
> 
> ...same concern here
> 
>> +static struct scmi_shmem_io_ops shmem_io_ops##s = {			\
>> +	.fromio	= shmem_memcpy_fromio##s,				\
>> +	.toio	= shmem_memcpy_toio##s,					\
>> +};

The macro might be a tad too much given that we only support one width, 
in case we needed to add a specific size in the future we could use a 
macro again I suppose, for now, just inlined the implementation for the 
4-byte / 32-bit size.

>> +
> 
> There are a bunch of warn/errs from checkpatch --strict, beside the volatile
> here and on the previous typedefs, also about args reuse and trailing semicolon
> in these macros...

I don't think we can silence the volatile ones, checkpatch --strict did 
not complain about the typedefs in my case, what did it look like in yours?
-- 
Florian