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

[Thomas Huth <thuth@redhat.com>]

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>