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

[Eric Auger <eric.auger@redhat.com>]

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);
>