Add a type cast and use extract64() instead of extract32()
to avoid integer overflow on addition. Fix bit fields
extraction according to documentation.
Also fix host-endianness bug by swapping desc fields from guest
memory order to host memory order after dma_memory_read().

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
---
 hw/dma/xlnx_dpdma.c | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index dd66be5265..d22b942274 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -175,24 +175,24 @@ static uint64_t xlnx_dpdma_desc_get_source_address(DPDMADescriptor *desc,
 
     switch (frag) {
     case 0:
-        addr = desc->source_address
-            + (extract32(desc->address_extension, 16, 12) << 20);
+        addr = (uint64_t)desc->source_address
+            + (extract64(desc->address_extension, 16, 16) << 32);
         break;
     case 1:
-        addr = desc->source_address2
-            + (extract32(desc->address_extension_23, 0, 12) << 8);
+        addr = (uint64_t)desc->source_address2
+            + (extract64(desc->address_extension_23, 0, 16) << 32);
         break;
     case 2:
-        addr = desc->source_address3
-            + (extract32(desc->address_extension_23, 16, 12) << 20);
+        addr = (uint64_t)desc->source_address3
+            + (extract64(desc->address_extension_23, 16, 16) << 32);
         break;
     case 3:
-        addr = desc->source_address4
-            + (extract32(desc->address_extension_45, 0, 12) << 8);
+        addr = (uint64_t)desc->source_address4
+            + (extract64(desc->address_extension_45, 0, 16) << 32);
         break;
     case 4:
-        addr = desc->source_address5
-            + (extract32(desc->address_extension_45, 16, 12) << 20);
+        addr = (uint64_t)desc->source_address5
+            + (extract64(desc->address_extension_45, 16, 16) << 32);
         break;
     default:
         addr = 0;
@@ -660,6 +660,24 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
             break;
         }
 
+        /* Convert from LE into host endianness.  */
+        desc.control = le32_to_cpu(desc.control);
+        desc.descriptor_id = le32_to_cpu(desc.descriptor_id);
+        desc.xfer_size = le32_to_cpu(desc.xfer_size);
+        desc.line_size_stride = le32_to_cpu(desc.line_size_stride);
+        desc.timestamp_lsb = le32_to_cpu(desc.timestamp_lsb);
+        desc.timestamp_msb = le32_to_cpu(desc.timestamp_msb);
+        desc.address_extension = le32_to_cpu(desc.address_extension);
+        desc.next_descriptor = le32_to_cpu(desc.next_descriptor);
+        desc.source_address = le32_to_cpu(desc.source_address);
+        desc.address_extension_23 = le32_to_cpu(desc.address_extension_23);
+        desc.address_extension_45 = le32_to_cpu(desc.address_extension_45);
+        desc.source_address2 = le32_to_cpu(desc.source_address2);
+        desc.source_address3 = le32_to_cpu(desc.source_address3);
+        desc.source_address4 = le32_to_cpu(desc.source_address4);
+        desc.source_address5 = le32_to_cpu(desc.source_address5);
+        desc.crc = le32_to_cpu(desc.crc);
+
         xlnx_dpdma_update_desc_info(s, channel, &desc);
 
 #ifdef DEBUG_DPDMA
-- 
2.30.2

On Wed, 24 Apr 2024 at 13:53, Alexandra Diupina <adiupina@astralinux.ru> wrote:
>
> Add a type cast and use extract64() instead of extract32()
> to avoid integer overflow on addition. Fix bit fields
> extraction according to documentation.
> Also fix host-endianness bug by swapping desc fields from guest
> memory order to host memory order after dma_memory_read().

Thanks for this patch. Could you split it into two, please?
The error in handling of the descriptor extension fields is a
separate problem from the endianness bug, so they should be
fixed in separate patches.

> @@ -660,6 +660,24 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
>              break;
>          }
>
> +        /* Convert from LE into host endianness.  */
> +        desc.control = le32_to_cpu(desc.control);
> +        desc.descriptor_id = le32_to_cpu(desc.descriptor_id);
> +        desc.xfer_size = le32_to_cpu(desc.xfer_size);
> +        desc.line_size_stride = le32_to_cpu(desc.line_size_stride);
> +        desc.timestamp_lsb = le32_to_cpu(desc.timestamp_lsb);
> +        desc.timestamp_msb = le32_to_cpu(desc.timestamp_msb);
> +        desc.address_extension = le32_to_cpu(desc.address_extension);
> +        desc.next_descriptor = le32_to_cpu(desc.next_descriptor);
> +        desc.source_address = le32_to_cpu(desc.source_address);
> +        desc.address_extension_23 = le32_to_cpu(desc.address_extension_23);
> +        desc.address_extension_45 = le32_to_cpu(desc.address_extension_45);
> +        desc.source_address2 = le32_to_cpu(desc.source_address2);
> +        desc.source_address3 = le32_to_cpu(desc.source_address3);
> +        desc.source_address4 = le32_to_cpu(desc.source_address4);
> +        desc.source_address5 = le32_to_cpu(desc.source_address5);
> +        desc.crc = le32_to_cpu(desc.crc);
> +
>          xlnx_dpdma_update_desc_info(s, channel, &desc);
>
>  #ifdef DEBUG_DPDMA

I would suggest factoring out a function like

static MemTxResult xlnx_dpdma_read_descriptor(XlnxDPDMAState *s,
uint64_t desc_addr,
                                              DPDMADescriptor *desc)

which wraps up "read the descriptor from desc_addr and fill in desc"
as a single operation (by calling dma_memory_read() and then
doing the byteswap).

thanks
-- PMM

Add a type cast and use extract64() instead of extract32()
to avoid integer overflow on addition. Fix bit fields
extraction according to documentation.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
---
 hw/dma/xlnx_dpdma.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index 1f5cd64ed1..52e8c594fe 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -175,24 +175,24 @@ static uint64_t xlnx_dpdma_desc_get_source_address(DPDMADescriptor *desc,
 
     switch (frag) {
     case 0:
-        addr = desc->source_address
-            + (extract32(desc->address_extension, 16, 12) << 20);
+        addr = (uint64_t)desc->source_address
+            + (extract64(desc->address_extension, 16, 12) << 20);
         break;
     case 1:
-        addr = desc->source_address2
-            + (extract32(desc->address_extension_23, 0, 12) << 8);
+        addr = (uint64_t)desc->source_address2
+            + (extract64(desc->address_extension_23, 0, 12) << 8);
         break;
     case 2:
-        addr = desc->source_address3
-            + (extract32(desc->address_extension_23, 16, 12) << 20);
+        addr = (uint64_t)desc->source_address3
+            + (extract64(desc->address_extension_23, 16, 12) << 20);
         break;
     case 3:
-        addr = desc->source_address4
-            + (extract32(desc->address_extension_45, 0, 12) << 8);
+        addr = (uint64_t)desc->source_address4
+            + (extract64(desc->address_extension_45, 0, 12) << 8);
         break;
     case 4:
-        addr = desc->source_address5
-            + (extract32(desc->address_extension_45, 16, 12) << 20);
+        addr = (uint64_t)desc->source_address5
+            + (extract64(desc->address_extension_45, 16, 12) << 20);
         break;
     default:
         addr = 0;
-- 
2.30.2

On Wed, 24 Apr 2024 at 19:13, Alexandra Diupina <adiupina@astralinux.ru> wrote:
>
> Add a type cast and use extract64() instead of extract32()
> to avoid integer overflow on addition. Fix bit fields
> extraction according to documentation.

The commit message here says we make the handling of the
address_extension fields match the documentation, and the
version of the fixes to this function in the patch
"[PATCH v2 RFC] fix host-endianness bug and prevent overflow"
did that, but here we seem to have gone back to not changing
the shift amounts.

We also need to extract all 16 bits of the address_extension
fields, not just 12, according to the datasheet.

thanks
-- PMM

Add a function xlnx_dpdma_read_descriptor() that
combines reading the descriptor from desc_addr
by calling dma_memory_read() and swapping desc
fields from guest memory order to host memory order.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
---
 hw/dma/xlnx_dpdma.c | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index 1f5cd64ed1..5fd4e31699 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -614,6 +614,38 @@ static void xlnx_dpdma_register_types(void)
     type_register_static(&xlnx_dpdma_info);
 }
 
+static MemTxResult xlnx_dpdma_read_descriptor(XlnxDPDMAState *s, uint8_t channel, 
+                                        uint64_t desc_addr, DPDMADescriptor *desc)
+{
+    if (dma_memory_read(&address_space_memory, desc_addr, &desc,
+                            sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED)) {
+        s->registers[DPDMA_EISR] |= ((1 << 1) << channel);
+        xlnx_dpdma_update_irq(s);
+        s->operation_finished[channel] = true;
+        return MEMTX_ERROR;
+    }
+
+    /* Convert from LE into host endianness.  */
+    desc->control = le32_to_cpu(desc->control);
+    desc->descriptor_id = le32_to_cpu(desc->descriptor_id);
+    desc->xfer_size = le32_to_cpu(desc->xfer_size);
+    desc->line_size_stride = le32_to_cpu(desc->line_size_stride);
+    desc->timestamp_lsb = le32_to_cpu(desc->timestamp_lsb);
+    desc->timestamp_msb = le32_to_cpu(desc->timestamp_msb);
+    desc->address_extension = le32_to_cpu(desc->address_extension);
+    desc->next_descriptor = le32_to_cpu(desc->next_descriptor);
+    desc->source_address = le32_to_cpu(desc->source_address);
+    desc->address_extension_23 = le32_to_cpu(desc->address_extension_23);
+    desc->address_extension_45 = le32_to_cpu(desc->address_extension_45);
+    desc->source_address2 = le32_to_cpu(desc->source_address2);
+    desc->source_address3 = le32_to_cpu(desc->source_address3);
+    desc->source_address4 = le32_to_cpu(desc->source_address4);
+    desc->source_address5 = le32_to_cpu(desc->source_address5);
+    desc->crc = le32_to_cpu(desc->crc);
+
+    return MEMTX_OK;
+}
+
 size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
                                     bool one_desc)
 {
@@ -651,11 +683,7 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
             desc_addr = xlnx_dpdma_descriptor_next_address(s, channel);
         }
 
-        if (dma_memory_read(&address_space_memory, desc_addr, &desc,
-                            sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED)) {
-            s->registers[DPDMA_EISR] |= ((1 << 1) << channel);
-            xlnx_dpdma_update_irq(s);
-            s->operation_finished[channel] = true;
+        if (xlnx_dpdma_read_descriptor(s, channel, desc_addr, &desc)) {
             DPRINTF("Can't get the descriptor.\n");
             break;
         }
-- 
2.30.2

On Wed, 24 Apr 2024 at 19:13, Alexandra Diupina <adiupina@astralinux.ru> wrote:
>
> Add a function xlnx_dpdma_read_descriptor() that
> combines reading the descriptor from desc_addr
> by calling dma_memory_read() and swapping desc
> fields from guest memory order to host memory order.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: d3c6369a96 ("introduce xlnx-dpdma")
> Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
> ---
>  hw/dma/xlnx_dpdma.c | 38 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
> index 1f5cd64ed1..5fd4e31699 100644
> --- a/hw/dma/xlnx_dpdma.c
> +++ b/hw/dma/xlnx_dpdma.c
> @@ -614,6 +614,38 @@ static void xlnx_dpdma_register_types(void)
>      type_register_static(&xlnx_dpdma_info);
>  }
>
> +static MemTxResult xlnx_dpdma_read_descriptor(XlnxDPDMAState *s, uint8_t channel,
> +                                        uint64_t desc_addr, DPDMADescriptor *desc)
> +{
> +    if (dma_memory_read(&address_space_memory, desc_addr, &desc,
> +                            sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED)) {
> +        s->registers[DPDMA_EISR] |= ((1 << 1) << channel);
> +        xlnx_dpdma_update_irq(s);
> +        s->operation_finished[channel] = true;

I think these three lines in the if() here should remain at the callsite --
although we only have one place that reads a descriptor at the
moment, different callers might in theory handle the descriptor-read
failure differently.

> +        return MEMTX_ERROR;

Otherwise this looks good; thanks.

-- PMM

Add a function xlnx_dpdma_read_descriptor() that
combines reading the descriptor from desc_addr
by calling dma_memory_read() and swapping desc
fields from guest memory order to host memory order.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
---
v2:minor changes in xlnx_dpdma_read_descriptor()
 hw/dma/xlnx_dpdma.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index dd66be5265..62a0952377 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -614,6 +614,34 @@ static void xlnx_dpdma_register_types(void)
     type_register_static(&xlnx_dpdma_info);
 }
 
+static MemTxResult xlnx_dpdma_read_descriptor(XlnxDPDMAState *s,
+                                    uint64_t desc_addr, DPDMADescriptor *desc)
+{
+    if (dma_memory_read(&address_space_memory, desc_addr, &desc,
+                            sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED))
+        return MEMTX_ERROR;
+
+    /* Convert from LE into host endianness.  */
+    desc->control = le32_to_cpu(desc->control);
+    desc->descriptor_id = le32_to_cpu(desc->descriptor_id);
+    desc->xfer_size = le32_to_cpu(desc->xfer_size);
+    desc->line_size_stride = le32_to_cpu(desc->line_size_stride);
+    desc->timestamp_lsb = le32_to_cpu(desc->timestamp_lsb);
+    desc->timestamp_msb = le32_to_cpu(desc->timestamp_msb);
+    desc->address_extension = le32_to_cpu(desc->address_extension);
+    desc->next_descriptor = le32_to_cpu(desc->next_descriptor);
+    desc->source_address = le32_to_cpu(desc->source_address);
+    desc->address_extension_23 = le32_to_cpu(desc->address_extension_23);
+    desc->address_extension_45 = le32_to_cpu(desc->address_extension_45);
+    desc->source_address2 = le32_to_cpu(desc->source_address2);
+    desc->source_address3 = le32_to_cpu(desc->source_address3);
+    desc->source_address4 = le32_to_cpu(desc->source_address4);
+    desc->source_address5 = le32_to_cpu(desc->source_address5);
+    desc->crc = le32_to_cpu(desc->crc);
+
+    return MEMTX_OK;
+}
+
 size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
                                     bool one_desc)
 {
@@ -651,8 +679,7 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
             desc_addr = xlnx_dpdma_descriptor_next_address(s, channel);
         }
 
-        if (dma_memory_read(&address_space_memory, desc_addr, &desc,
-                            sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED)) {
+        if (xlnx_dpdma_read_descriptor(s, desc_addr, &desc)) {
             s->registers[DPDMA_EISR] |= ((1 << 1) << channel);
             xlnx_dpdma_update_irq(s);
             s->operation_finished[channel] = true;
-- 
2.30.2

Hi Alexandra,

On 25/4/24 12:07, Alexandra Diupina wrote:
> Add a function xlnx_dpdma_read_descriptor() that
> combines reading the descriptor from desc_addr
> by calling dma_memory_read() and swapping desc
> fields from guest memory order to host memory order.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: d3c6369a96 ("introduce xlnx-dpdma")
> Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
> ---
> v2:minor changes in xlnx_dpdma_read_descriptor()
>   hw/dma/xlnx_dpdma.c | 31 +++++++++++++++++++++++++++++--
>   1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
> index dd66be5265..62a0952377 100644
> --- a/hw/dma/xlnx_dpdma.c
> +++ b/hw/dma/xlnx_dpdma.c
> @@ -614,6 +614,34 @@ static void xlnx_dpdma_register_types(void)
>       type_register_static(&xlnx_dpdma_info);
>   }
>   
> +static MemTxResult xlnx_dpdma_read_descriptor(XlnxDPDMAState *s,
> +                                    uint64_t desc_addr, DPDMADescriptor *desc)
> +{
> +    if (dma_memory_read(&address_space_memory, desc_addr, &desc,
> +                            sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED))
> +        return MEMTX_ERROR;
> +
> +    /* Convert from LE into host endianness.  */
> +    desc->control = le32_to_cpu(desc->control);
> +    desc->descriptor_id = le32_to_cpu(desc->descriptor_id);
> +    desc->xfer_size = le32_to_cpu(desc->xfer_size);
> +    desc->line_size_stride = le32_to_cpu(desc->line_size_stride);
> +    desc->timestamp_lsb = le32_to_cpu(desc->timestamp_lsb);
> +    desc->timestamp_msb = le32_to_cpu(desc->timestamp_msb);
> +    desc->address_extension = le32_to_cpu(desc->address_extension);
> +    desc->next_descriptor = le32_to_cpu(desc->next_descriptor);
> +    desc->source_address = le32_to_cpu(desc->source_address);
> +    desc->address_extension_23 = le32_to_cpu(desc->address_extension_23);
> +    desc->address_extension_45 = le32_to_cpu(desc->address_extension_45);
> +    desc->source_address2 = le32_to_cpu(desc->source_address2);
> +    desc->source_address3 = le32_to_cpu(desc->source_address3);
> +    desc->source_address4 = le32_to_cpu(desc->source_address4);
> +    desc->source_address5 = le32_to_cpu(desc->source_address5);
> +    desc->crc = le32_to_cpu(desc->crc);
> +
> +    return MEMTX_OK;
> +}
> +
>   size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
>                                       bool one_desc)
>   {
> @@ -651,8 +679,7 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
>               desc_addr = xlnx_dpdma_descriptor_next_address(s, channel);
>           }
>   
> -        if (dma_memory_read(&address_space_memory, desc_addr, &desc,
> -                            sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED)) {
> +        if (xlnx_dpdma_read_descriptor(s, desc_addr, &desc)) {

Correct, but this is incomplete, because we have the same problem
on the write descriptor back path, few lines later in the
xlnx_dpdma_desc_update_enabled() block.

>               s->registers[DPDMA_EISR] |= ((1 << 1) << channel);
>               xlnx_dpdma_update_irq(s);
>               s->operation_finished[channel] = true;

Add xlnx_dpdma_read_descriptor() and
xlnx_dpdma_write_descriptor() functions.
xlnx_dpdma_read_descriptor() combines reading a
descriptor from desc_addr by calling dma_memory_read()
and swapping the desc fields from guest memory order
to host memory order. xlnx_dpdma_write_descriptor()
performs similar actions when writing a descriptor.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
---
v3: add xlnx_dpdma_write_descriptor()
v2: minor changes in xlnx_dpdma_read_descriptor()
 hw/dma/xlnx_dpdma.c | 59 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 4 deletions(-)

diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index dd66be5265..7845f43221 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -614,6 +614,59 @@ static void xlnx_dpdma_register_types(void)
     type_register_static(&xlnx_dpdma_info);
 }
 
+static MemTxResult xlnx_dpdma_read_descriptor(XlnxDPDMAState *s,
+                                    uint64_t desc_addr, DPDMADescriptor *desc)
+{
+    if (dma_memory_read(&address_space_memory, desc_addr, &desc,
+                            sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED))
+        return MEMTX_ERROR;
+
+    /* Convert from LE into host endianness.  */
+    desc->control = le32_to_cpu(desc->control);
+    desc->descriptor_id = le32_to_cpu(desc->descriptor_id);
+    desc->xfer_size = le32_to_cpu(desc->xfer_size);
+    desc->line_size_stride = le32_to_cpu(desc->line_size_stride);
+    desc->timestamp_lsb = le32_to_cpu(desc->timestamp_lsb);
+    desc->timestamp_msb = le32_to_cpu(desc->timestamp_msb);
+    desc->address_extension = le32_to_cpu(desc->address_extension);
+    desc->next_descriptor = le32_to_cpu(desc->next_descriptor);
+    desc->source_address = le32_to_cpu(desc->source_address);
+    desc->address_extension_23 = le32_to_cpu(desc->address_extension_23);
+    desc->address_extension_45 = le32_to_cpu(desc->address_extension_45);
+    desc->source_address2 = le32_to_cpu(desc->source_address2);
+    desc->source_address3 = le32_to_cpu(desc->source_address3);
+    desc->source_address4 = le32_to_cpu(desc->source_address4);
+    desc->source_address5 = le32_to_cpu(desc->source_address5);
+    desc->crc = le32_to_cpu(desc->crc);
+
+    return MEMTX_OK;
+}
+
+static void xlnx_dpdma_write_descriptor(uint64_t desc_addr,
+                                                DPDMADescriptor *desc)
+{
+    /* Convert from host endianness into LE.  */
+    desc->control = cpu_to_le32(desc->control);
+    desc->descriptor_id = cpu_to_le32(desc->descriptor_id);
+    desc->xfer_size = cpu_to_le32(desc->xfer_size);
+    desc->line_size_stride = cpu_to_le32(desc->line_size_stride);
+    desc->timestamp_lsb = cpu_to_le32(desc->timestamp_lsb);
+    desc->timestamp_msb = cpu_to_le32(desc->timestamp_msb);
+    desc->address_extension = cpu_to_le32(desc->address_extension);
+    desc->next_descriptor = cpu_to_le32(desc->next_descriptor);
+    desc->source_address = cpu_to_le32(desc->source_address);
+    desc->address_extension_23 = cpu_to_le32(desc->address_extension_23);
+    desc->address_extension_45 = cpu_to_le32(desc->address_extension_45);
+    desc->source_address2 = cpu_to_le32(desc->source_address2);
+    desc->source_address3 = cpu_to_le32(desc->source_address3);
+    desc->source_address4 = cpu_to_le32(desc->source_address4);
+    desc->source_address5 = cpu_to_le32(desc->source_address5);
+    desc->crc = cpu_to_le32(desc->crc);
+
+    dma_memory_write(&address_space_memory, desc_addr, &desc,
+                            sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED);
+}
+
 size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
                                     bool one_desc)
 {
@@ -651,8 +704,7 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
             desc_addr = xlnx_dpdma_descriptor_next_address(s, channel);
         }
 
-        if (dma_memory_read(&address_space_memory, desc_addr, &desc,
-                            sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED)) {
+        if (xlnx_dpdma_read_descriptor(s, desc_addr, &desc)) {
             s->registers[DPDMA_EISR] |= ((1 << 1) << channel);
             xlnx_dpdma_update_irq(s);
             s->operation_finished[channel] = true;
@@ -755,8 +807,7 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
             /* The descriptor need to be updated when it's completed. */
             DPRINTF("update the descriptor with the done flag set.\n");
             xlnx_dpdma_desc_set_done(&desc);
-            dma_memory_write(&address_space_memory, desc_addr, &desc,
-                             sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED);
+            xlnx_dpdma_write_descriptor(desc_addr, &desc);
         }
 
         if (xlnx_dpdma_desc_completion_interrupt(&desc)) {
-- 
2.30.2

On 4/25/24 06:41, Alexandra Diupina wrote:
> +static MemTxResult xlnx_dpdma_read_descriptor(XlnxDPDMAState *s,
> +                                    uint64_t desc_addr, DPDMADescriptor *desc)
> +{
> +    if (dma_memory_read(&address_space_memory, desc_addr, &desc,
> +                            sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED))
> +        return MEMTX_ERROR;
> +

Missing { } for docs/devel/style.rst.

> +static void xlnx_dpdma_write_descriptor(uint64_t desc_addr,
> +                                                DPDMADescriptor *desc)
> +{
> +    /* Convert from host endianness into LE.  */
> +    desc->control = cpu_to_le32(desc->control);
> +    desc->descriptor_id = cpu_to_le32(desc->descriptor_id);
> +    desc->xfer_size = cpu_to_le32(desc->xfer_size);
> +    desc->line_size_stride = cpu_to_le32(desc->line_size_stride);
> +    desc->timestamp_lsb = cpu_to_le32(desc->timestamp_lsb);
> +    desc->timestamp_msb = cpu_to_le32(desc->timestamp_msb);
> +    desc->address_extension = cpu_to_le32(desc->address_extension);
> +    desc->next_descriptor = cpu_to_le32(desc->next_descriptor);
> +    desc->source_address = cpu_to_le32(desc->source_address);
> +    desc->address_extension_23 = cpu_to_le32(desc->address_extension_23);
> +    desc->address_extension_45 = cpu_to_le32(desc->address_extension_45);
> +    desc->source_address2 = cpu_to_le32(desc->source_address2);
> +    desc->source_address3 = cpu_to_le32(desc->source_address3);
> +    desc->source_address4 = cpu_to_le32(desc->source_address4);
> +    desc->source_address5 = cpu_to_le32(desc->source_address5);
> +    desc->crc = cpu_to_le32(desc->crc);

This is incorrect, rewriting in place, because after the call,

>          if (xlnx_dpdma_desc_completion_interrupt(&desc)) {

the memory block is still live, and the swap here has corrupted it.

> +
> +    dma_memory_write(&address_space_memory, desc_addr, &desc,

This is incorrect because desc is now a pointer so &desc is DPDMADescriptor **.

Do not reply to an existing thread to post a new patch.


r~