+ To: Fred On Tue, 16 Apr 2024 at 19:56, Alexandra Diupina <adiup...@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 <adiup...@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 <adiup...@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 > > >
