[PATCH for-8.1] hw/arm/smmu: Handle big-endian hosts correctly

[PATCH for-8.1] hw/arm/smmu: Handle big-endian hosts correctly

[Peter Maydell]

The implementation of the SMMUv3 has multiple places where it reads a
data structure from the guest and directly operates on it without
doing a guest-to-host endianness conversion.  Since all SMMU data
structures are little-endian, this means that the SMMU doesn't work
on a big-endian host.  In particular, this causes the Avocado test
  machine_aarch64_virt.py:Aarch64VirtMachine.test_alpine_virt_tcg_gic_max
to fail on an s390x host.

Add appropriate byte-swapping on reads and writes of guest in-memory
data structures so that the device works correctly on big-endian
hosts.

As part of this we constrain queue_read() to operate only on Cmd
structs and queue_write() on Evt structs, because in practice these
are the only data structures the two functions are used with, and we
need to know what the data structure is to be able to byte-swap its
parts correctly.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-stable@nongnu.org
---
 hw/arm/smmu-common.c |  3 +--
 hw/arm/smmuv3.c      | 39 +++++++++++++++++++++++++++++++--------
 2 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 5ab9d45d58a..f35ae9aa22c 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -216,8 +216,7 @@ static int get_pte(dma_addr_t baseaddr, uint32_t index, uint64_t *pte,
     dma_addr_t addr = baseaddr + index * sizeof(*pte);
 
     /* TODO: guarantee 64-bit single-copy atomicity */
-    ret = dma_memory_read(&address_space_memory, addr, pte, sizeof(*pte),
-                          MEMTXATTRS_UNSPECIFIED);
+    ret = ldq_le_dma(&address_space_memory, addr, pte, MEMTXATTRS_UNSPECIFIED);
 
     if (ret != MEMTX_OK) {
         info->type = SMMU_PTW_ERR_WALK_EABT;
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 932f0096974..1e9be8e89af 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -102,20 +102,34 @@ static void smmuv3_write_gerrorn(SMMUv3State *s, uint32_t new_gerrorn)
     trace_smmuv3_write_gerrorn(toggled & pending, s->gerrorn);
 }
 
-static inline MemTxResult queue_read(SMMUQueue *q, void *data)
+static inline MemTxResult queue_read(SMMUQueue *q, Cmd *cmd)
 {
     dma_addr_t addr = Q_CONS_ENTRY(q);
+    MemTxResult ret;
+    int i;
 
-    return dma_memory_read(&address_space_memory, addr, data, q->entry_size,
-                           MEMTXATTRS_UNSPECIFIED);
+    ret = dma_memory_read(&address_space_memory, addr, cmd, sizeof(Cmd),
+                          MEMTXATTRS_UNSPECIFIED);
+    if (ret != MEMTX_OK) {
+        return ret;
+    }
+    for (i = 0; i < ARRAY_SIZE(cmd->word); i++) {
+        le32_to_cpus(&cmd->word[i]);
+    }
+    return ret;
 }
 
-static MemTxResult queue_write(SMMUQueue *q, void *data)
+static MemTxResult queue_write(SMMUQueue *q, Evt *evt_in)
 {
     dma_addr_t addr = Q_PROD_ENTRY(q);
     MemTxResult ret;
+    Evt evt = *evt_in;
+    int i;
 
-    ret = dma_memory_write(&address_space_memory, addr, data, q->entry_size,
+    for (i = 0; i < ARRAY_SIZE(evt.word); i++) {
+        cpu_to_le32s(&evt.word[i]);
+    }
+    ret = dma_memory_write(&address_space_memory, addr, &evt, sizeof(Evt),
                            MEMTXATTRS_UNSPECIFIED);
     if (ret != MEMTX_OK) {
         return ret;
@@ -298,7 +312,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
 static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
                         SMMUEventInfo *event)
 {
-    int ret;
+    int ret, i;
 
     trace_smmuv3_get_ste(addr);
     /* TODO: guarantee 64-bit single-copy atomicity */
@@ -311,6 +325,9 @@ static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
         event->u.f_ste_fetch.addr = addr;
         return -EINVAL;
     }
+    for (i = 0; i < ARRAY_SIZE(buf->word); i++) {
+        le32_to_cpus(&buf->word[i]);
+    }
     return 0;
 
 }
@@ -320,7 +337,7 @@ static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid,
                        CD *buf, SMMUEventInfo *event)
 {
     dma_addr_t addr = STE_CTXPTR(ste);
-    int ret;
+    int ret, i;
 
     trace_smmuv3_get_cd(addr);
     /* TODO: guarantee 64-bit single-copy atomicity */
@@ -333,6 +350,9 @@ static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid,
         event->u.f_ste_fetch.addr = addr;
         return -EINVAL;
     }
+    for (i = 0; i < ARRAY_SIZE(buf->word); i++) {
+        le32_to_cpus(&buf->word[i]);
+    }
     return 0;
 }
 
@@ -569,7 +589,7 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
         return -EINVAL;
     }
     if (s->features & SMMU_FEATURE_2LVL_STE) {
-        int l1_ste_offset, l2_ste_offset, max_l2_ste, span;
+        int l1_ste_offset, l2_ste_offset, max_l2_ste, span, i;
         dma_addr_t l1ptr, l2ptr;
         STEDesc l1std;
 
@@ -593,6 +613,9 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
             event->u.f_ste_fetch.addr = l1ptr;
             return -EINVAL;
         }
+        for (i = 0; i < ARRAY_SIZE(l1std.word); i++) {
+            le32_to_cpus(&l1std.word[i]);
+        }
 
         span = L1STD_SPAN(&l1std);
 
-- 
2.34.1

Re: [PATCH for-8.1] hw/arm/smmu: Handle big-endian hosts correctly

[Thomas Huth]

On 17/07/2023 15.26, Peter Maydell wrote:
> The implementation of the SMMUv3 has multiple places where it reads a
> data structure from the guest and directly operates on it without
> doing a guest-to-host endianness conversion.  Since all SMMU data
> structures are little-endian, this means that the SMMU doesn't work
> on a big-endian host.  In particular, this causes the Avocado test
>    machine_aarch64_virt.py:Aarch64VirtMachine.test_alpine_virt_tcg_gic_max
> to fail on an s390x host.
> 
> Add appropriate byte-swapping on reads and writes of guest in-memory
> data structures so that the device works correctly on big-endian
> hosts.
> 
> As part of this we constrain queue_read() to operate only on Cmd
> structs and queue_write() on Evt structs, because in practice these
> are the only data structures the two functions are used with, and we
> need to know what the data structure is to be able to byte-swap its
> parts correctly.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-stable@nongnu.org
> ---
>   hw/arm/smmu-common.c |  3 +--
>   hw/arm/smmuv3.c      | 39 +++++++++++++++++++++++++++++++--------
>   2 files changed, 32 insertions(+), 10 deletions(-)

Thanks, this fixes the alpine test for me:

Tested-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH for-8.1] hw/arm/smmu: Handle big-endian hosts correctly

[Philippe Mathieu-Daudé]

On 17/7/23 15:26, Peter Maydell wrote:
> The implementation of the SMMUv3 has multiple places where it reads a
> data structure from the guest and directly operates on it without
> doing a guest-to-host endianness conversion.  Since all SMMU data
> structures are little-endian, this means that the SMMU doesn't work
> on a big-endian host.  In particular, this causes the Avocado test
>    machine_aarch64_virt.py:Aarch64VirtMachine.test_alpine_virt_tcg_gic_max
> to fail on an s390x host.
> 
> Add appropriate byte-swapping on reads and writes of guest in-memory
> data structures so that the device works correctly on big-endian
> hosts.
> 
> As part of this we constrain queue_read() to operate only on Cmd
> structs and queue_write() on Evt structs, because in practice these
> are the only data structures the two functions are used with, and we
> need to know what the data structure is to be able to byte-swap its
> parts correctly.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-stable@nongnu.org
> ---
>   hw/arm/smmu-common.c |  3 +--
>   hw/arm/smmuv3.c      | 39 +++++++++++++++++++++++++++++++--------
>   2 files changed, 32 insertions(+), 10 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH for-8.1] hw/arm/smmu: Handle big-endian hosts correctly

[Eric Auger]

Hi Peter,

On 7/17/23 15:26, Peter Maydell wrote:
> The implementation of the SMMUv3 has multiple places where it reads a
> data structure from the guest and directly operates on it without
> doing a guest-to-host endianness conversion.  Since all SMMU data
> structures are little-endian, this means that the SMMU doesn't work
> on a big-endian host.  In particular, this causes the Avocado test
>   machine_aarch64_virt.py:Aarch64VirtMachine.test_alpine_virt_tcg_gic_max
> to fail on an s390x host.
>
> Add appropriate byte-swapping on reads and writes of guest in-memory
> data structures so that the device works correctly on big-endian
> hosts.
>
> As part of this we constrain queue_read() to operate only on Cmd
> structs and queue_write() on Evt structs, because in practice these
> are the only data structures the two functions are used with, and we
> need to know what the data structure is to be able to byte-swap its
> parts correctly.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-stable@nongnu.org
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  hw/arm/smmu-common.c |  3 +--
>  hw/arm/smmuv3.c      | 39 +++++++++++++++++++++++++++++++--------
>  2 files changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 5ab9d45d58a..f35ae9aa22c 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -216,8 +216,7 @@ static int get_pte(dma_addr_t baseaddr, uint32_t index, uint64_t *pte,
>      dma_addr_t addr = baseaddr + index * sizeof(*pte);
>  
>      /* TODO: guarantee 64-bit single-copy atomicity */
> -    ret = dma_memory_read(&address_space_memory, addr, pte, sizeof(*pte),
> -                          MEMTXATTRS_UNSPECIFIED);
> +    ret = ldq_le_dma(&address_space_memory, addr, pte, MEMTXATTRS_UNSPECIFIED);
>  
>      if (ret != MEMTX_OK) {
>          info->type = SMMU_PTW_ERR_WALK_EABT;
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 932f0096974..1e9be8e89af 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -102,20 +102,34 @@ static void smmuv3_write_gerrorn(SMMUv3State *s, uint32_t new_gerrorn)
>      trace_smmuv3_write_gerrorn(toggled & pending, s->gerrorn);
>  }
>  
> -static inline MemTxResult queue_read(SMMUQueue *q, void *data)
> +static inline MemTxResult queue_read(SMMUQueue *q, Cmd *cmd)
>  {
>      dma_addr_t addr = Q_CONS_ENTRY(q);
> +    MemTxResult ret;
> +    int i;
>  
> -    return dma_memory_read(&address_space_memory, addr, data, q->entry_size,
> -                           MEMTXATTRS_UNSPECIFIED);
> +    ret = dma_memory_read(&address_space_memory, addr, cmd, sizeof(Cmd),
> +                          MEMTXATTRS_UNSPECIFIED);
> +    if (ret != MEMTX_OK) {
> +        return ret;
> +    }
> +    for (i = 0; i < ARRAY_SIZE(cmd->word); i++) {
> +        le32_to_cpus(&cmd->word[i]);
> +    }
> +    return ret;
>  }
>  
> -static MemTxResult queue_write(SMMUQueue *q, void *data)
> +static MemTxResult queue_write(SMMUQueue *q, Evt *evt_in)
>  {
>      dma_addr_t addr = Q_PROD_ENTRY(q);
>      MemTxResult ret;
> +    Evt evt = *evt_in;
> +    int i;
>  
> -    ret = dma_memory_write(&address_space_memory, addr, data, q->entry_size,
> +    for (i = 0; i < ARRAY_SIZE(evt.word); i++) {
> +        cpu_to_le32s(&evt.word[i]);
> +    }
> +    ret = dma_memory_write(&address_space_memory, addr, &evt, sizeof(Evt),
>                             MEMTXATTRS_UNSPECIFIED);
>      if (ret != MEMTX_OK) {
>          return ret;
> @@ -298,7 +312,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
>  static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
>                          SMMUEventInfo *event)
>  {
> -    int ret;
> +    int ret, i;
>  
>      trace_smmuv3_get_ste(addr);
>      /* TODO: guarantee 64-bit single-copy atomicity */
> @@ -311,6 +325,9 @@ static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
>          event->u.f_ste_fetch.addr = addr;
>          return -EINVAL;
>      }
> +    for (i = 0; i < ARRAY_SIZE(buf->word); i++) {
> +        le32_to_cpus(&buf->word[i]);
> +    }
>      return 0;
>  
>  }
> @@ -320,7 +337,7 @@ static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid,
>                         CD *buf, SMMUEventInfo *event)
>  {
>      dma_addr_t addr = STE_CTXPTR(ste);
> -    int ret;
> +    int ret, i;
>  
>      trace_smmuv3_get_cd(addr);
>      /* TODO: guarantee 64-bit single-copy atomicity */
> @@ -333,6 +350,9 @@ static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid,
>          event->u.f_ste_fetch.addr = addr;
>          return -EINVAL;
>      }
> +    for (i = 0; i < ARRAY_SIZE(buf->word); i++) {
> +        le32_to_cpus(&buf->word[i]);
> +    }
>      return 0;
>  }
>  
> @@ -569,7 +589,7 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
>          return -EINVAL;
>      }
>      if (s->features & SMMU_FEATURE_2LVL_STE) {
> -        int l1_ste_offset, l2_ste_offset, max_l2_ste, span;
> +        int l1_ste_offset, l2_ste_offset, max_l2_ste, span, i;
>          dma_addr_t l1ptr, l2ptr;
>          STEDesc l1std;
>  
> @@ -593,6 +613,9 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
>              event->u.f_ste_fetch.addr = l1ptr;
>              return -EINVAL;
>          }
> +        for (i = 0; i < ARRAY_SIZE(l1std.word); i++) {
> +            le32_to_cpus(&l1std.word[i]);
> +        }
>  
>          span = L1STD_SPAN(&l1std);
>