Overflow can occur in a situation where desc->source_address
has a maximum value (pow(2, 32) - 1), so add a cast to a
larger type before the assignment.

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..224259225c 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
+            + (extract32(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
+            + (extract32(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
+            + (extract32(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
+            + (extract32(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
+            + (extract32(desc->address_extension_45, 16, 12) << 20));
         break;
     default:
         addr = 0;
-- 
2.30.2

On Fri, 12 Apr 2024 at 09:13, Alexandra Diupina <adiupina@astralinux.ru> wrote:
>
> Overflow can occur in a situation where desc->source_address
> has a maximum value (pow(2, 32) - 1), so add a cast to a
> larger type before the assignment.
>
> 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..224259225c 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
> +            + (extract32(desc->address_extension, 16, 12) << 20));

Unless I'm confused, this cast doesn't help, because we
will have already done a 32-bit addition of desc->source_address
and the value from the address_extension part, so it doesn't
change the result.

If we want to do the addition at 64 bits then using extract64()
would be the simplest way to arrange for that.

However, I can't figure out what this code is trying to do and
make that line up with the data sheet; maybe this isn't the
right datasheet for this device?

https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/ADDR_EXT-Field

The datasheet suggests that we should take 32 bits of the address
from one field (here desc->source_address) and 16 bits of the
address from another (here desc->address_extension's high bits)
and combine them to make a 48 bit address. But this code is only
looking at 12 bits of the high 16 in address_extension, and it
doesn't shift them right enough to put them into bits [47:32]
of the final address.

Xilinx folks: what hardware is this modelling, and is it
really the right behaviour?

Also, this device looks like it has a host-endianness bug: the
DPDMADescriptor struct is read directly from guest memory in
dma_memory_read(), but the device never does anything to swap
the fields from guest memory order to host memory order. So
this is likely broken on big-endian hosts.

thanks
-- PMM

Hi,

> -----Original Message-----
> From: qemu-devel-bounces+fkonrad=amd.com@nongnu.org <qemu-devel-bounces+fkonrad=amd.com@nongnu.org> On Behalf Of
> Peter Maydell
> Sent: Friday, April 12, 2024 12:07 PM
> To: Alexandra Diupina <adiupina@astralinux.ru>
> Cc: Alistair Francis <alistair@alistair23.me>; Edgar E. Iglesias <edgar.iglesias@gmail.com>; qemu-arm@nongnu.org; qemu-
> devel@nongnu.org; sdl.qemu@linuxtesting.org
> Subject: Re: [PATCH RFC] prevent overflow in xlnx_dpdma_desc_get_source_address()
> 
> On Fri, 12 Apr 2024 at 09:13, Alexandra Diupina <adiupina@astralinux.ru> wrote:
> >
> > Overflow can occur in a situation where desc->source_address
> > has a maximum value (pow(2, 32) - 1), so add a cast to a
> > larger type before the assignment.
> >
> > 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..224259225c 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
> > +            + (extract32(desc->address_extension, 16, 12) << 20));
> 
> Unless I'm confused, this cast doesn't help, because we
> will have already done a 32-bit addition of desc->source_address
> and the value from the address_extension part, so it doesn't
> change the result.
> 
> If we want to do the addition at 64 bits then using extract64()
> would be the simplest way to arrange for that.
> 
> However, I can't figure out what this code is trying to do and
> make that line up with the data sheet; maybe this isn't the
> right datasheet for this device?
> 
> https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/ADDR_EXT-Field
> 
> The datasheet suggests that we should take 32 bits of the address
> from one field (here desc->source_address) and 16 bits of the
> address from another (here desc->address_extension's high bits)
> and combine them to make a 48 bit address. But this code is only
> looking at 12 bits of the high 16 in address_extension, and it
> doesn't shift them right enough to put them into bits [47:32]
> of the final address.
> 
> Xilinx folks: what hardware is this modelling, and is it
> really the right behaviour?

Looks like this is the right documentation.  Most probably the descriptor field changed
since I did that model, or I got really confused.

> 
> Also, this device looks like it has a host-endianness bug: the
> DPDMADescriptor struct is read directly from guest memory in
> dma_memory_read(), but the device never does anything to swap
> the fields from guest memory order to host memory order. So
> this is likely broken on big-endian hosts.
> 

Yes indeed.

Best Regards,
Fred

> thanks
> -- PMM

17/04/24 13:05, Konrad, Frederic пишет:
> Hi,
>
>> -----Original Message-----
>> From: qemu-devel-bounces+fkonrad=amd.com@nongnu.org <qemu-devel-bounces+fkonrad=amd.com@nongnu.org> On Behalf Of
>> Peter Maydell
>> Sent: Friday, April 12, 2024 12:07 PM
>> To: Alexandra Diupina <adiupina@astralinux.ru>
>> Cc: Alistair Francis <alistair@alistair23.me>; Edgar E. Iglesias <edgar.iglesias@gmail.com>; qemu-arm@nongnu.org; qemu-
>> devel@nongnu.org; sdl.qemu@linuxtesting.org
>> Subject: Re: [PATCH RFC] prevent overflow in xlnx_dpdma_desc_get_source_address()
>>
>> On Fri, 12 Apr 2024 at 09:13, Alexandra Diupina <adiupina@astralinux.ru> wrote:
>>> Overflow can occur in a situation where desc->source_address
>>> has a maximum value (pow(2, 32) - 1), so add a cast to a
>>> larger type before the assignment.
>>>
>>> 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..224259225c 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
>>> +            + (extract32(desc->address_extension, 16, 12) << 20));
>> Unless I'm confused, this cast doesn't help, because we
>> will have already done a 32-bit addition of desc->source_address
>> and the value from the address_extension part, so it doesn't
>> change the result.
>>
>> If we want to do the addition at 64 bits then using extract64()
>> would be the simplest way to arrange for that.
>>
>> However, I can't figure out what this code is trying to do and
>> make that line up with the data sheet; maybe this isn't the
>> right datasheet for this device?
>>
>> https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/ADDR_EXT-Field
>>
>> The datasheet suggests that we should take 32 bits of the address
>> from one field (here desc->source_address) and 16 bits of the
>> address from another (here desc->address_extension's high bits)
>> and combine them to make a 48 bit address. But this code is only
>> looking at 12 bits of the high 16 in address_extension, and it
>> doesn't shift them right enough to put them into bits [47:32]
>> of the final address.
>>
>> Xilinx folks: what hardware is this modelling, and is it
>> really the right behaviour?
> Looks like this is the right documentation.  Most probably the descriptor field changed
> since I did that model, or I got really confused.
>
>> Also, this device looks like it has a host-endianness bug: the
>> DPDMADescriptor struct is read directly from guest memory in
>> dma_memory_read(), but the device never does anything to swap
>> the fields from guest memory order to host memory order. So
>> this is likely broken on big-endian hosts.
>>
> Yes indeed.
>
> Best Regards,
> Fred
>
>> thanks
>> -- PMM

hw/dma/xlnx_dpdma.c defines a dma_ops structure
with the endianness field set to DEVICE_NATIVE_ENDIAN.
Doesn't this guarantee that there will be no endian errors?

Alexandra

On Tue, 23 Apr 2024 at 11:23, Alexandra Diupina <adiupina@astralinux.ru> wrote:
> 17/04/24 13:05, Konrad, Frederic пишет:
> > Peter Maydell wrote:
> >> Also, this device looks like it has a host-endianness bug: the
> >> DPDMADescriptor struct is read directly from guest memory in
> >> dma_memory_read(), but the device never does anything to swap
> >> the fields from guest memory order to host memory order. So
> >> this is likely broken on big-endian hosts.

> hw/dma/xlnx_dpdma.c defines a dma_ops structure
> with the endianness field set to DEVICE_NATIVE_ENDIAN.
> Doesn't this guarantee that there will be no endian errors?

The endianness on a MemoryRegionOps struct tells the
memory core how to handle guest accesses to the
*registers* that struct is for. So if the .endianness
is DEVICE_LITTLE_ENDIAN and the guest is big-endian,
the memory core will byteswap the data values. That
means that the read and write functions always take
and return "value" arguments which are what you expect
(i.e. the guest wrote 0x12345678 and the write fn
value argument is 0x12345678).

However, if a device does direct accesses to guest *memory* (like
a DMA-capable device will do), that is something it has to handle
the endianness for itself. (The device's manual should say what
way round it does memory loads.)
There are two basic ways a device can do guest memory access:

 (1) we provide functions which say "load from guest memory
 a value of this type with this endianness"; for instance
 ldq_le_dma() will load a 64-bit little-endian value into a
 host C uint64_t, and ldq_be_dma() will do the same for a 64-bit
 big-endian value. There are similar functions for stores.
 This can be the simplest approach but it's a bit less efficient
 because it goes up and down the memory subsystem for each data
 item being read.

 (2) we provide functions which are "load/store a sequence of bytes
 from guest memory". This is what dma_memory_read() does.
 Because it's just a sequence of bytes, the device code is
 then responsible for interpreting what those bytes mean:
 maybe they're just bytes, or maybe they're a sequence of
 little-endian 32-bit values, or maybe they're something else.
 The device code also has to deal with the fact that the
 alignment of these values in memory is not necessarily the
 same as what the host CPU requires. These things together
 mean you can't simply cast the pointer to a sequence-of-bytes
 to a host type, or tell dma_memory_read() to write the
 sequence-of-bytes to a host struct type. We provide functions
 like ldl_le_p() to say "read a little-endian 32-bit value
 from this host pointer location" to assist in pulling
 values out of a sequence-of-bytes array. Or if the data is
 all of the same type (eg it's an array of 32-bit values)
 you can dma_memory_read() it into a host uint32_t[] array
 and then use le32_to_cpu() to convert those uint32_t values
 to the host's endianness.

(I use the dma_* family of functions here as an example since that's
what xlnx_dpdma.c happens to use; the same general principle applies
for our various other families of guest load/store functions, like
the address_space_* ones.)

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.
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~

Peter, thank you! I agree with you that
as mentioned in the documentation
https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/ADDR_EXT-Field,
we should take 32 bits of the address from one field
(for example, case 1, SRC_ADDR2_EXT - in code it is desc->source_address2)
and 16 bits (high or low) of the address from another field
(ADDR_EXT_23 - in code it is desc->address_extension_23, we need [15:0] 
bits)
and combine them to make a 48 bit address.

Therefore, I suggest making the following changes to the code
so that it matches the documentation:

static uint64_t xlnx_dpdma_desc_get_source_address(DPDMADescriptor *desc,
                                                      uint8_t frag)
{
     uint64_t addr = 0;
     assert(frag < 5);

     switch (frag) {
     case 0:
         addr = (uint64_t)desc->source_address
             + (extract64(desc->address_extension, 16, 16) << 32);
         break;
     case 1:
         addr = (uint64_t)desc->source_address2
             + (extract64(desc->address_extension_23, 0, 16) << 32);
         break;
     case 2:
         addr = (uint64_t)desc->source_address3
             + (extract64(desc->address_extension_23, 16, 16) << 32);
         break;
     case 3:
         addr = (uint64_t)desc->source_address4
             + (extract64(desc->address_extension_45, 0, 16) << 32);
         break;
     case 4:
         addr = (uint64_t)desc->source_address5
             + (extract64(desc->address_extension_45, 16, 16) << 32);
         break;
     default:
         addr = 0;
         break;
     }

     return addr;
}


This change adds a type cast and also uses extract64() instead of 
extract32()
to avoid integer overflow on addition (there was a typo in the previous 
letter).
Also in extract64() extracts a bit field with a length of 16 bits 
instead of 12,
the shift is changed to 32 so that the extracted field fits into bits 
[47:32] of the final address.

if this calculation is correct, I'm ready to create a second version of 
the patch.




12/04/24 13:06, Peter Maydell пишет:
> On Fri, 12 Apr 2024 at 09:13, Alexandra Diupina <adiupina@astralinux.ru> wrote:
>> Overflow can occur in a situation where desc->source_address
>> has a maximum value (pow(2, 32) - 1), so add a cast to a
>> larger type before the assignment.
>>
>> 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..224259225c 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
>> +            + (extract32(desc->address_extension, 16, 12) << 20));
> Unless I'm confused, this cast doesn't help, because we
> will have already done a 32-bit addition of desc->source_address
> and the value from the address_extension part, so it doesn't
> change the result.
>
> If we want to do the addition at 64 bits then using extract64()
> would be the simplest way to arrange for that.
>
> However, I can't figure out what this code is trying to do and
> make that line up with the data sheet; maybe this isn't the
> right datasheet for this device?
>
> https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/ADDR_EXT-Field
>
> The datasheet suggests that we should take 32 bits of the address
> from one field (here desc->source_address) and 16 bits of the
> address from another (here desc->address_extension's high bits)
> and combine them to make a 48 bit address. But this code is only
> looking at 12 bits of the high 16 in address_extension, and it
> doesn't shift them right enough to put them into bits [47:32]
> of the final address.
>
> Xilinx folks: what hardware is this modelling, and is it
> really the right behaviour?
>
> Also, this device looks like it has a host-endianness bug: the
> DPDMADescriptor struct is read directly from guest memory in
> dma_memory_read(), but the device never does anything to swap
> the fields from guest memory order to host memory order. So
> this is likely broken on big-endian hosts.
>
> thanks
> -- PMM

+ To: Fred

On Tue, 16 Apr 2024 at 19:56, Alexandra Diupina <adiupina@astralinux.ru>
wrote:

> Peter, thank you! I agree with you that
> as mentioned in the documentation
> https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/ADDR_EXT-Field,
> we should take 32 bits of the address from one field
> (for example, case 1, SRC_ADDR2_EXT - in code it is desc->source_address2)
> and 16 bits (high or low) of the address from another field
> (ADDR_EXT_23 - in code it is desc->address_extension_23, we need [15:0]
> bits)
> and combine them to make a 48 bit address.
>
> Therefore, I suggest making the following changes to the code
> so that it matches the documentation:
>
> static uint64_t xlnx_dpdma_desc_get_source_address(DPDMADescriptor *desc,
>                                                       uint8_t frag)
> {
>      uint64_t addr = 0;
>      assert(frag < 5);
>
>      switch (frag) {
>      case 0:
>          addr = (uint64_t)desc->source_address
>              + (extract64(desc->address_extension, 16, 16) << 32);
>          break;
>      case 1:
>          addr = (uint64_t)desc->source_address2
>              + (extract64(desc->address_extension_23, 0, 16) << 32);
>          break;
>      case 2:
>          addr = (uint64_t)desc->source_address3
>              + (extract64(desc->address_extension_23, 16, 16) << 32);
>          break;
>      case 3:
>          addr = (uint64_t)desc->source_address4
>              + (extract64(desc->address_extension_45, 0, 16) << 32);
>          break;
>      case 4:
>          addr = (uint64_t)desc->source_address5
>              + (extract64(desc->address_extension_45, 16, 16) << 32);
>          break;
>      default:
>          addr = 0;
>          break;
>      }
>
>      return addr;
> }
>
>
> This change adds a type cast and also uses extract64() instead of
> extract32()
> to avoid integer overflow on addition (there was a typo in the previous
> letter).
> Also in extract64() extracts a bit field with a length of 16 bits
> instead of 12,
> the shift is changed to 32 so that the extracted field fits into bits
> [47:32] of the final address.
>
> if this calculation is correct, I'm ready to create a second version of
> the patch.
>
>
>
>
> 12/04/24 13:06, Peter Maydell пишет:
> > On Fri, 12 Apr 2024 at 09:13, Alexandra Diupina <adiupina@astralinux.ru>
> wrote:
> >> Overflow can occur in a situation where desc->source_address
> >> has a maximum value (pow(2, 32) - 1), so add a cast to a
> >> larger type before the assignment.
> >>
> >> 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..224259225c 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
> >> +            + (extract32(desc->address_extension, 16, 12) << 20));
> > Unless I'm confused, this cast doesn't help, because we
> > will have already done a 32-bit addition of desc->source_address
> > and the value from the address_extension part, so it doesn't
> > change the result.
> >
> > If we want to do the addition at 64 bits then using extract64()
> > would be the simplest way to arrange for that.
> >
> > However, I can't figure out what this code is trying to do and
> > make that line up with the data sheet; maybe this isn't the
> > right datasheet for this device?
> >
> > https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/ADDR_EXT-Field
> >
> > The datasheet suggests that we should take 32 bits of the address
> > from one field (here desc->source_address) and 16 bits of the
> > address from another (here desc->address_extension's high bits)
> > and combine them to make a 48 bit address. But this code is only
> > looking at 12 bits of the high 16 in address_extension, and it
> > doesn't shift them right enough to put them into bits [47:32]
> > of the final address.
> >
> > Xilinx folks: what hardware is this modelling, and is it
> > really the right behaviour?
> >
> > Also, this device looks like it has a host-endianness bug: the
> > DPDMADescriptor struct is read directly from guest memory in
> > dma_memory_read(), but the device never does anything to swap
> > the fields from guest memory order to host memory order. So
> > this is likely broken on big-endian hosts.
> >
> > thanks
> > -- PMM
>
>
>