[PATCH 0/2] arm: allwinner: fix endianness bugs in sdhost and sun8i-emac

[PATCH 0/2] arm: allwinner: fix endianness bugs in sdhost and sun8i-emac

[Peter Maydell]

This patchset fixes bugs in the sd controller and ethernet controller
devices used in the orangepi-pc board model. The bug is the same in
both cases: we read and write a descriptor struct from guest memory
without byte-swapping it, so the code only does the right thing on
a little-endian host.

These fixes (together with some of the others I've sent out earlier
today) are enough to get the BootLinuxConsole.test_arm_orangepi_sd
avocado test passing on an s390x host.

thanks
-- PMM

Peter Maydell (2):
  hw/sd/allwinner-sdhost: Correctly byteswap descriptor fields
  hw/net/allwinner-sun8i-emac: Correctly byteswap descriptor fields

 hw/net/allwinner-sun8i-emac.c | 22 +++++++++++++++-------
 hw/sd/allwinner-sdhost.c      | 31 ++++++++++++++++++++++++++-----
 2 files changed, 41 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH 1/2] hw/sd/allwinner-sdhost: Correctly byteswap descriptor fields

[Peter Maydell]

In allwinner_sdhost_process_desc() we just read directly from
guest memory into a host TransferDescriptor struct and back.
This only works on little-endian hosts. Abstract the reading
and writing of descriptors into functions that handle the
byte-swapping so that TransferDescriptor structs as seen by
the rest of the code are always in host-order.

This fixes a failure of one of the avocado tests on s390.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/allwinner-sdhost.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
index 51e5e908307..92a0f42708d 100644
--- a/hw/sd/allwinner-sdhost.c
+++ b/hw/sd/allwinner-sdhost.c
@@ -302,6 +302,30 @@ static void allwinner_sdhost_auto_stop(AwSdHostState *s)
     }
 }
 
+static void read_descriptor(AwSdHostState *s, hwaddr desc_addr,
+                            TransferDescriptor *desc)
+{
+    uint32_t desc_words[4];
+    dma_memory_read(&s->dma_as, desc_addr, &desc_words, sizeof(desc_words),
+                    MEMTXATTRS_UNSPECIFIED);
+    desc->status = le32_to_cpu(desc_words[0]);
+    desc->size = le32_to_cpu(desc_words[1]);
+    desc->addr = le32_to_cpu(desc_words[2]);
+    desc->next = le32_to_cpu(desc_words[3]);
+}
+
+static void write_descriptor(AwSdHostState *s, hwaddr desc_addr,
+                             const TransferDescriptor *desc)
+{
+    uint32_t desc_words[4];
+    desc_words[0] = cpu_to_le32(desc->status);
+    desc_words[1] = cpu_to_le32(desc->size);
+    desc_words[2] = cpu_to_le32(desc->addr);
+    desc_words[3] = cpu_to_le32(desc->next);
+    dma_memory_write(&s->dma_as, desc_addr, &desc_words, sizeof(desc_words),
+                     MEMTXATTRS_UNSPECIFIED);
+}
+
 static uint32_t allwinner_sdhost_process_desc(AwSdHostState *s,
                                               hwaddr desc_addr,
                                               TransferDescriptor *desc,
@@ -312,9 +336,7 @@ static uint32_t allwinner_sdhost_process_desc(AwSdHostState *s,
     uint32_t num_bytes = max_bytes;
     uint8_t buf[1024];
 
-    /* Read descriptor */
-    dma_memory_read(&s->dma_as, desc_addr, desc, sizeof(*desc),
-                    MEMTXATTRS_UNSPECIFIED);
+    read_descriptor(s, desc_addr, desc);
     if (desc->size == 0) {
         desc->size = klass->max_desc_size;
     } else if (desc->size > klass->max_desc_size) {
@@ -356,8 +378,7 @@ static uint32_t allwinner_sdhost_process_desc(AwSdHostState *s,
 
     /* Clear hold flag and flush descriptor */
     desc->status &= ~DESC_STATUS_HOLD;
-    dma_memory_write(&s->dma_as, desc_addr, desc, sizeof(*desc),
-                     MEMTXATTRS_UNSPECIFIED);
+    write_descriptor(s, desc_addr, desc);
 
     return num_done;
 }
-- 
2.34.1

Re: [PATCH 1/2] hw/sd/allwinner-sdhost: Correctly byteswap descriptor fields

[Thomas Huth]

On 24/04/2023 18.50, Peter Maydell wrote:
> In allwinner_sdhost_process_desc() we just read directly from
> guest memory into a host TransferDescriptor struct and back.
> This only works on little-endian hosts. Abstract the reading
> and writing of descriptors into functions that handle the
> byte-swapping so that TransferDescriptor structs as seen by
> the rest of the code are always in host-order.
> 
> This fixes a failure of one of the avocado tests on s390.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/sd/allwinner-sdhost.c | 31 ++++++++++++++++++++++++++-----
>   1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
> index 51e5e908307..92a0f42708d 100644
> --- a/hw/sd/allwinner-sdhost.c
> +++ b/hw/sd/allwinner-sdhost.c
> @@ -302,6 +302,30 @@ static void allwinner_sdhost_auto_stop(AwSdHostState *s)
>       }
>   }
>   
> +static void read_descriptor(AwSdHostState *s, hwaddr desc_addr,
> +                            TransferDescriptor *desc)
> +{
> +    uint32_t desc_words[4];
> +    dma_memory_read(&s->dma_as, desc_addr, &desc_words, sizeof(desc_words),
> +                    MEMTXATTRS_UNSPECIFIED);
> +    desc->status = le32_to_cpu(desc_words[0]);
> +    desc->size = le32_to_cpu(desc_words[1]);
> +    desc->addr = le32_to_cpu(desc_words[2]);
> +    desc->next = le32_to_cpu(desc_words[3]);
> +}
> +
> +static void write_descriptor(AwSdHostState *s, hwaddr desc_addr,
> +                             const TransferDescriptor *desc)
> +{
> +    uint32_t desc_words[4];
> +    desc_words[0] = cpu_to_le32(desc->status);
> +    desc_words[1] = cpu_to_le32(desc->size);
> +    desc_words[2] = cpu_to_le32(desc->addr);
> +    desc_words[3] = cpu_to_le32(desc->next);
> +    dma_memory_write(&s->dma_as, desc_addr, &desc_words, sizeof(desc_words),
> +                     MEMTXATTRS_UNSPECIFIED);
> +}
> +
>   static uint32_t allwinner_sdhost_process_desc(AwSdHostState *s,
>                                                 hwaddr desc_addr,
>                                                 TransferDescriptor *desc,
> @@ -312,9 +336,7 @@ static uint32_t allwinner_sdhost_process_desc(AwSdHostState *s,
>       uint32_t num_bytes = max_bytes;
>       uint8_t buf[1024];
>   
> -    /* Read descriptor */
> -    dma_memory_read(&s->dma_as, desc_addr, desc, sizeof(*desc),
> -                    MEMTXATTRS_UNSPECIFIED);
> +    read_descriptor(s, desc_addr, desc);
>       if (desc->size == 0) {
>           desc->size = klass->max_desc_size;
>       } else if (desc->size > klass->max_desc_size) {
> @@ -356,8 +378,7 @@ static uint32_t allwinner_sdhost_process_desc(AwSdHostState *s,
>   
>       /* Clear hold flag and flush descriptor */
>       desc->status &= ~DESC_STATUS_HOLD;
> -    dma_memory_write(&s->dma_as, desc_addr, desc, sizeof(*desc),
> -                     MEMTXATTRS_UNSPECIFIED);
> +    write_descriptor(s, desc_addr, desc);
>   
>       return num_done;
>   }

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

Re: [PATCH 1/2] hw/sd/allwinner-sdhost: Correctly byteswap descriptor fields

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> In allwinner_sdhost_process_desc() we just read directly from
> guest memory into a host TransferDescriptor struct and back.
> This only works on little-endian hosts. Abstract the reading
> and writing of descriptors into functions that handle the
> byte-swapping so that TransferDescriptor structs as seen by
> the rest of the code are always in host-order.

I couldn't find any datasheets on the sdhost hardware but the kernel
certainly explicitly sets the endianess of the descriptors so:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 1/2] hw/sd/allwinner-sdhost: Correctly byteswap descriptor fields

[Philippe Mathieu-Daudé]

On 24/4/23 18:50, Peter Maydell wrote:
> In allwinner_sdhost_process_desc() we just read directly from
> guest memory into a host TransferDescriptor struct and back.
> This only works on little-endian hosts. Abstract the reading
> and writing of descriptors into functions that handle the
> byte-swapping so that TransferDescriptor structs as seen by
> the rest of the code are always in host-order.
> 
> This fixes a failure of one of the avocado tests on s390.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/sd/allwinner-sdhost.c | 31 ++++++++++++++++++++++++++-----
>   1 file changed, 26 insertions(+), 5 deletions(-)

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

[PATCH 2/2] hw/net/allwinner-sun8i-emac: Correctly byteswap descriptor fields

[Peter Maydell]

In allwinner-sun8i-emac we just read directly from guest memory into
a host FrameDescriptor struct and back.  This only works on
little-endian hosts.  Reading and writing of descriptors is already
abstracted into functions; make those functions also handle the
byte-swapping so that TransferDescriptor structs as seen by the rest
of the code are always in host-order, and fix two places that were
doing ad-hoc descriptor reading without using the functions.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/allwinner-sun8i-emac.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/net/allwinner-sun8i-emac.c b/hw/net/allwinner-sun8i-emac.c
index b861d8ff352..fac4405f452 100644
--- a/hw/net/allwinner-sun8i-emac.c
+++ b/hw/net/allwinner-sun8i-emac.c
@@ -350,8 +350,13 @@ static void allwinner_sun8i_emac_get_desc(AwSun8iEmacState *s,
                                           FrameDescriptor *desc,
                                           uint32_t phys_addr)
 {
-    dma_memory_read(&s->dma_as, phys_addr, desc, sizeof(*desc),
+    uint32_t desc_words[4];
+    dma_memory_read(&s->dma_as, phys_addr, &desc_words, sizeof(desc_words),
                     MEMTXATTRS_UNSPECIFIED);
+    desc->status = le32_to_cpu(desc_words[0]);
+    desc->status2 = le32_to_cpu(desc_words[1]);
+    desc->addr = le32_to_cpu(desc_words[2]);
+    desc->next = le32_to_cpu(desc_words[3]);
 }
 
 static uint32_t allwinner_sun8i_emac_next_desc(AwSun8iEmacState *s,
@@ -400,10 +405,15 @@ static uint32_t allwinner_sun8i_emac_tx_desc(AwSun8iEmacState *s,
 }
 
 static void allwinner_sun8i_emac_flush_desc(AwSun8iEmacState *s,
-                                            FrameDescriptor *desc,
+                                            const FrameDescriptor *desc,
                                             uint32_t phys_addr)
 {
-    dma_memory_write(&s->dma_as, phys_addr, desc, sizeof(*desc),
+    uint32_t desc_words[4];
+    desc_words[0] = cpu_to_le32(desc->status);
+    desc_words[1] = cpu_to_le32(desc->status2);
+    desc_words[2] = cpu_to_le32(desc->addr);
+    desc_words[3] = cpu_to_le32(desc->next);
+    dma_memory_write(&s->dma_as, phys_addr, &desc_words, sizeof(desc_words),
                      MEMTXATTRS_UNSPECIFIED);
 }
 
@@ -638,8 +648,7 @@ static uint64_t allwinner_sun8i_emac_read(void *opaque, hwaddr offset,
         break;
     case REG_TX_CUR_BUF:        /* Transmit Current Buffer */
         if (s->tx_desc_curr != 0) {
-            dma_memory_read(&s->dma_as, s->tx_desc_curr, &desc, sizeof(desc),
-                            MEMTXATTRS_UNSPECIFIED);
+            allwinner_sun8i_emac_get_desc(s, &desc, s->tx_desc_curr);
             value = desc.addr;
         } else {
             value = 0;
@@ -652,8 +661,7 @@ static uint64_t allwinner_sun8i_emac_read(void *opaque, hwaddr offset,
         break;
     case REG_RX_CUR_BUF:        /* Receive Current Buffer */
         if (s->rx_desc_curr != 0) {
-            dma_memory_read(&s->dma_as, s->rx_desc_curr, &desc, sizeof(desc),
-                            MEMTXATTRS_UNSPECIFIED);
+            allwinner_sun8i_emac_get_desc(s, &desc, s->rx_desc_curr);
             value = desc.addr;
         } else {
             value = 0;
-- 
2.34.1

Re: [PATCH 2/2] hw/net/allwinner-sun8i-emac: Correctly byteswap descriptor fields

[Thomas Huth]

On 24/04/2023 18.50, Peter Maydell wrote:
> In allwinner-sun8i-emac we just read directly from guest memory into
> a host FrameDescriptor struct and back.  This only works on
> little-endian hosts.  Reading and writing of descriptors is already
> abstracted into functions; make those functions also handle the
> byte-swapping so that TransferDescriptor structs as seen by the rest
> of the code are always in host-order, and fix two places that were
> doing ad-hoc descriptor reading without using the functions.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/net/allwinner-sun8i-emac.c | 22 +++++++++++++++-------
>   1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/net/allwinner-sun8i-emac.c b/hw/net/allwinner-sun8i-emac.c
> index b861d8ff352..fac4405f452 100644
> --- a/hw/net/allwinner-sun8i-emac.c
> +++ b/hw/net/allwinner-sun8i-emac.c
> @@ -350,8 +350,13 @@ static void allwinner_sun8i_emac_get_desc(AwSun8iEmacState *s,
>                                             FrameDescriptor *desc,
>                                             uint32_t phys_addr)
>   {
> -    dma_memory_read(&s->dma_as, phys_addr, desc, sizeof(*desc),
> +    uint32_t desc_words[4];
> +    dma_memory_read(&s->dma_as, phys_addr, &desc_words, sizeof(desc_words),
>                       MEMTXATTRS_UNSPECIFIED);
> +    desc->status = le32_to_cpu(desc_words[0]);
> +    desc->status2 = le32_to_cpu(desc_words[1]);
> +    desc->addr = le32_to_cpu(desc_words[2]);
> +    desc->next = le32_to_cpu(desc_words[3]);
>   }
>   
>   static uint32_t allwinner_sun8i_emac_next_desc(AwSun8iEmacState *s,
> @@ -400,10 +405,15 @@ static uint32_t allwinner_sun8i_emac_tx_desc(AwSun8iEmacState *s,
>   }
>   
>   static void allwinner_sun8i_emac_flush_desc(AwSun8iEmacState *s,
> -                                            FrameDescriptor *desc,
> +                                            const FrameDescriptor *desc,
>                                               uint32_t phys_addr)
>   {
> -    dma_memory_write(&s->dma_as, phys_addr, desc, sizeof(*desc),
> +    uint32_t desc_words[4];
> +    desc_words[0] = cpu_to_le32(desc->status);
> +    desc_words[1] = cpu_to_le32(desc->status2);
> +    desc_words[2] = cpu_to_le32(desc->addr);
> +    desc_words[3] = cpu_to_le32(desc->next);
> +    dma_memory_write(&s->dma_as, phys_addr, &desc_words, sizeof(desc_words),
>                        MEMTXATTRS_UNSPECIFIED);
>   }
>   
> @@ -638,8 +648,7 @@ static uint64_t allwinner_sun8i_emac_read(void *opaque, hwaddr offset,
>           break;
>       case REG_TX_CUR_BUF:        /* Transmit Current Buffer */
>           if (s->tx_desc_curr != 0) {
> -            dma_memory_read(&s->dma_as, s->tx_desc_curr, &desc, sizeof(desc),
> -                            MEMTXATTRS_UNSPECIFIED);
> +            allwinner_sun8i_emac_get_desc(s, &desc, s->tx_desc_curr);
>               value = desc.addr;
>           } else {
>               value = 0;
> @@ -652,8 +661,7 @@ static uint64_t allwinner_sun8i_emac_read(void *opaque, hwaddr offset,
>           break;
>       case REG_RX_CUR_BUF:        /* Receive Current Buffer */
>           if (s->rx_desc_curr != 0) {
> -            dma_memory_read(&s->dma_as, s->rx_desc_curr, &desc, sizeof(desc),
> -                            MEMTXATTRS_UNSPECIFIED);
> +            allwinner_sun8i_emac_get_desc(s, &desc, s->rx_desc_curr);
>               value = desc.addr;
>           } else {
>               value = 0;

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

Re: [PATCH 2/2] hw/net/allwinner-sun8i-emac: Correctly byteswap descriptor fields

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> In allwinner-sun8i-emac we just read directly from guest memory into
> a host FrameDescriptor struct and back.  This only works on
> little-endian hosts.  Reading and writing of descriptors is already
> abstracted into functions; make those functions also handle the
> byte-swapping so that TransferDescriptor structs as seen by the rest
> of the code are always in host-order, and fix two places that were
> doing ad-hoc descriptor reading without using the functions.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 2/2] hw/net/allwinner-sun8i-emac: Correctly byteswap descriptor fields

[Philippe Mathieu-Daudé]

On 24/4/23 18:50, Peter Maydell wrote:
> In allwinner-sun8i-emac we just read directly from guest memory into
> a host FrameDescriptor struct and back.  This only works on
> little-endian hosts.  Reading and writing of descriptors is already
> abstracted into functions; make those functions also handle the
> byte-swapping so that TransferDescriptor structs as seen by the rest
> of the code are always in host-order, and fix two places that were
> doing ad-hoc descriptor reading without using the functions.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/net/allwinner-sun8i-emac.c | 22 +++++++++++++++-------
>   1 file changed, 15 insertions(+), 7 deletions(-)

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

Re: [PATCH 0/2] arm: allwinner: fix endianness bugs in sdhost and sun8i-emac

[Peter Maydell]

On Mon, 24 Apr 2023 at 17:50, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset fixes bugs in the sd controller and ethernet controller
> devices used in the orangepi-pc board model. The bug is the same in
> both cases: we read and write a descriptor struct from guest memory
> without byte-swapping it, so the code only does the right thing on
> a little-endian host.
>
> These fixes (together with some of the others I've sent out earlier
> today) are enough to get the BootLinuxConsole.test_arm_orangepi_sd
> avocado test passing on an s390x host.
>



Applied to target-arm.next, thanks.

-- PMM