linux-kernel - Re: [PATCH v3 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] [day] [month] [year] [list]

Message-ID: <078bc5ee-e591-427b-a9fe-9386738a847f@broadcom.com>
Date: Thu, 22 Aug 2024 15:50:03 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Sudeep Holla <sudeep.holla@....com>
Cc: linux-arm-kernel@...ts.infradead.org, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Cristian Marussi <cristian.marussi@....com>,
 "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
 <devicetree@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>,
 "open list:SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE"
 <arm-scmi@...r.kernel.org>, justin.chen@...adcom.com, opendmb@...il.com,
 kapil.hali@...adcom.com, bcm-kernel-feedback-list@...adcom.com
Subject: Re: [PATCH v3 2/2] firmware: arm_scmi: Support 'reg-io-width'
 property for shared memory

On 8/22/24 03:46, Sudeep Holla wrote:
> Hi Florian,
> 
> Sorry for getting late to this party, I wasn't able to review this before.
> Overall changes look correct. But my main concern is that is SCMI the right
> place to have such IO accessors. It is better to run it through Arnd if
> he is happy with it before I send him the pull request containing these.

Sure, would definitively want more eyes to review.

> 
> On Fri, Aug 16, 2024 at 03:42:21PM -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.
>>
> 
> Is this limitation on the hardware for both read and writes ?

This applies to both reads and writes. We have to make accesses on a 4 
byte boundary and of exactly 4 bytes in size.

> The reason I ask is I see arm64 does have memcpy_toio_aligned() or
> __iowrite32_copy_full() for 32 bit aligned writes.
> 

That appears to work nicely on ARM64 and ARM 32-bit, thanks for the 
suggestion! One needs to be careful that __io{read,write}32_copy takes 
32-bit units, not bytes!

FWIW, here is my diff between v3 and v4:

diff --git a/drivers/firmware/arm_scmi/common.h 
b/drivers/firmware/arm_scmi/common.h
index 73bb496fac01..a13f79b37c99 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -319,9 +319,9 @@ enum scmi_bad_msg {
  /* Used for compactness and signature validation of the function 
pointers being
   * passed.
   */
-typedef void (*shmem_copy_toio_t)(volatile void __iomem *to, const void 
*from,
+typedef void (*shmem_copy_toio_t)(void __iomem *to, const void *from,
                                   size_t count);
-typedef void (*shmem_copy_fromio_t)(void *to, const volatile void 
__iomem *from,
+typedef void (*shmem_copy_fromio_t)(void *to, const void __iomem *from,
                                     size_t count);

  /**
diff --git a/drivers/firmware/arm_scmi/shmem.c 
b/drivers/firmware/arm_scmi/shmem.c
index aded5f1cd49f..e9f30ab671a8 100644
--- a/drivers/firmware/arm_scmi/shmem.c
+++ b/drivers/firmware/arm_scmi/shmem.c
@@ -35,33 +35,25 @@ struct scmi_shared_mem {
  };

  static inline void shmem_memcpy_fromio32(void *to,
-                                        const volatile void __iomem *from,
+                                        const void __iomem *from,
                                          size_t count)
  {
-       while (count) {
-               *(u32 *)to = __raw_readl(from);
-               from += 4;
-               to += 4;
-               count -= 4;
-       }
+       WARN_ON(!IS_ALIGNED((unsigned long)from, 4) ||
+               !IS_ALIGNED((unsigned long)to, 4) ||
+               count % 4);

-       /* Ensure all reads from I/O have completed */
-       rmb();
+       __ioread32_copy(to, from, count / 4);
  }

-static inline void shmem_memcpy_toio32(volatile void __iomem *to,
+static inline void shmem_memcpy_toio32(void __iomem *to,
                                        const void *from,
                                        size_t count)
  {
-       while (count) {
-               __raw_writel(*(u32 *)from, to);
-               from += 4;
-               to += 4;
-               count -= 4;
-       }
+       WARN_ON(!IS_ALIGNED((unsigned long)to, 4) ||
+               !IS_ALIGNED((unsigned long)from, 4) ||
+               count % 4);

-       /* Ensure all writes to I/O have completed */
-       wmb();
+       __iowrite32_copy(to, from, count / 4);
  }

  static struct scmi_shmem_io_ops shmem_io_ops32 = {
@@ -73,13 +65,13 @@ static struct scmi_shmem_io_ops shmem_io_ops32 = {
   * pre-processor.
   */
  static inline void shmem_memcpy_fromio(void *to,
-                                      const volatile void __iomem *from,
+                                      const void __iomem *from,
                                        size_t count)
  {
         memcpy_fromio(to, from, count);
  }

-static inline void shmem_memcpy_toio(volatile void __iomem *to,
+static inline void shmem_memcpy_toio(void __iomem *to,
                                      const void *from,
                                      size_t count)
  {

-- 
Florian