[PATCH v1 0/3] Fixed bugs in pl080

[PATCH v1 0/3] Fixed bugs in pl080

[1774596582]

From: TaoDing <1774596582@qq.com>

Hi all,
When I tested pl080 using the existing dmatest tool in Linux, 
I found that pl080 could not output the results correctly.
The main issues involved are inconsistent source and destination bit widths, 
misalignment of LLI, which can lead to incorrect data and even cause qemu crashes.

TaoDing (3):
  An interrupt is generated after the DMA transfer complete.
  The swidth and dwidth of pl080 are not equal.
  The LLI of pl080 should be aligned with 4 bytes.

 hw/dma/pl080.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

-- 
2.43.0

Re: [PATCH v1 0/3] Fixed bugs in pl080

[Tao Tang]

Hi,

It looks like three patches of this series were sent out-of-thread and 
are not attached to this cover letter.

You may want to resend the full series, or resend those patches with 
In-Reply-To pointing to the Message-ID of this cover letter so they 
appear in the same thread:

git 
send-email --in-reply-to="tencent_1BF621A658E043C088A2681178D57ED14A09@qq.com" 
xxxxx

Thanks for working on this!

Tao

Re: [PATCH v1 0/3] Fixed bugs in pl080

[Peter Maydell]

On Wed, 11 Mar 2026 at 16:14, Tao Tang <tangtao1634@phytium.com.cn> wrote:
>
> Hi,
>
> It looks like three patches of this series were sent out-of-thread and
> are not attached to this cover letter.
>
> You may want to resend the full series, or resend those patches with
> In-Reply-To pointing to the Message-ID of this cover letter so they
> appear in the same thread:
>
> git
> send-email --in-reply-to="tencent_1BF621A658E043C088A2681178D57ED14A09@qq.com"
> xxxxx

It looks to me looking at the headers that the emails were sent with correct
message-id and in-reply-to headers, but something in the qq.com
mail infrastructure has re-written the message-IDs and broken the
threading.

-- PMM

Re: [PATCH v1 0/3] Fixed bugs in pl080

[有点]

[-- Attachment #1: Type: text/plain, Size: 1817 bytes --]

Hi,


Thank you very much for your guidance
This is my first time using this system to submit code.
The message ID for patch v1 0/3 has unexpectedly changed, I am trying to submit it using a other email.




         原始邮件
         
       
发件人：Peter Maydell <peter.maydell@linaro.org&gt;
发件时间：2026年3月12日 01:05
收件人：Tao Tang <tangtao1634@phytium.com.cn&gt;
抄送：1774596582 <1774596582@qq.com&gt;, qemu-arm <qemu-arm@nongnu.org&gt;, qemu-devel <qemu-devel@nongnu.org&gt;
主题：Re: [PATCH v1 0/3] Fixed bugs in pl080



       On&nbsp;Wed,&nbsp;11&nbsp;Mar&nbsp;2026&nbsp;at&nbsp;16:14,&nbsp;Tao&nbsp;Tang&nbsp;<tangtao1634@phytium.com.cn&gt;&nbsp;wrote:
&gt;
&gt;&nbsp;Hi,
&gt;
&gt;&nbsp;It&nbsp;looks&nbsp;like&nbsp;three&nbsp;patches&nbsp;of&nbsp;this&nbsp;series&nbsp;were&nbsp;sent&nbsp;out-of-thread&nbsp;and
&gt;&nbsp;are&nbsp;not&nbsp;attached&nbsp;to&nbsp;this&nbsp;cover&nbsp;letter.
&gt;
&gt;&nbsp;You&nbsp;may&nbsp;want&nbsp;to&nbsp;resend&nbsp;the&nbsp;full&nbsp;series,&nbsp;or&nbsp;resend&nbsp;those&nbsp;patches&nbsp;with
&gt;&nbsp;In-Reply-To&nbsp;pointing&nbsp;to&nbsp;the&nbsp;Message-ID&nbsp;of&nbsp;this&nbsp;cover&nbsp;letter&nbsp;so&nbsp;they
&gt;&nbsp;appear&nbsp;in&nbsp;the&nbsp;same&nbsp;thread:
&gt;
&gt;&nbsp;git
&gt;&nbsp;send-email&nbsp;--in-reply-to="tencent_1BF621A658E043C088A2681178D57ED14A09@qq.com"
&gt;&nbsp;xxxxx

It&nbsp;looks&nbsp;to&nbsp;me&nbsp;looking&nbsp;at&nbsp;the&nbsp;headers&nbsp;that&nbsp;the&nbsp;emails&nbsp;were&nbsp;sent&nbsp;with&nbsp;correct
message-id&nbsp;and&nbsp;in-reply-to&nbsp;headers,&nbsp;but&nbsp;something&nbsp;in&nbsp;the&nbsp;qq.com
mail&nbsp;infrastructure&nbsp;has&nbsp;re-written&nbsp;the&nbsp;message-IDs&nbsp;and&nbsp;broken&nbsp;the
threading.

--&nbsp;PMM

[-- Attachment #2: Type: text/html, Size: 4197 bytes --]

[PATCH v1 0/3] Fixed bugs in pl080

[Tao Ding]

Hi all,
When I tested pl080 using the existing dmatest tool in Linux, 
I found that pl080 could not output the results correctly.
The main issues involved are inconsistent source and destination bit widths, 
misalignment of LLI, which can lead to incorrect data and even cause qemu crashes.

Tao Ding (3):
  An interrupt is generated after the DMA transfer complete.
  The swidth and dwidth of pl080 are not equal.
  The LLI of pl080 should be aligned with 4 bytes.

 hw/dma/pl080.c | 45 ++++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)


base-commit: 1fd5ff9d76d23ab23a68419cbc76d5ee33e8b455
-- 
2.43.0

[PATCH v1 1/3] An interrupt is generated after the DMA transfer complete.

[Tao Ding]

PL080 cannot trigger interrupt correctly. Reproduce this problem in a simple way.

In the versatilpb board, the interrupt output of pl080 is connected to the 17th interrupt input of pl190.
By reading the status register of pl190, determined whether there is an interrupt generated for the corresponding input.

The basic address of pl080 is 0x10130000, and the basic address of pl190 is 0x10140000. 
Read the value of the address through the qemu monitor.

Configuration
    ../configure --target-list=arm-softmmu --enable-debug
Reproducer
    ./qemu-system-arm -M versatilepb -m 128M -nographic -S \
    -device loader,addr=0x00000000,data=0x11223344,data-len=4 \
    -device loader,addr=0x00001000,data=0x00000000,data-len=4 \
    -device loader,addr=0x10130030,data=0x00000001,data-len=4 \
    -device loader,addr=0x10130100,data=0x00000000,data-len=4 \
    -device loader,addr=0x10130104,data=0x00001000,data-len=4 \
    -device loader,addr=0x10130108,data=0x00000000,data-len=4 \
    -device loader,addr=0x1013010C,data=0x9e4bf001,data-len=4 \
    -device loader,addr=0x10130110,data=0x0000c001,data-len=4

Qemu monitor
    (qemu) xp /1wx 0x10140008
    10140008: 0x00000000

The result correctly after fixed
    (qemu) xp /1wx 0x10140008
    10140008: 0x00020000

Signed-off-by: Tao Ding <dingtao0430@163.com>
---
 hw/dma/pl080.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/dma/pl080.c b/hw/dma/pl080.c
index 3f8acb03de..0979a687aa 100644
--- a/hw/dma/pl080.c
+++ b/hw/dma/pl080.c
@@ -212,6 +212,7 @@ again:
         if (--s->running)
             s->running = 1;
     }
+    pl080_update(s);
 }
 
 static uint64_t pl080_read(void *opaque, hwaddr offset,
-- 
2.43.0

[PATCH v1 2/3] The swidth and dwidth of pl080 are not equal.

[Tao Ding]

The amount of data transmitted each time is the maximum value between s and w, 
but it should be noted that the remaining transmission amount is less than the width.

Configure swidth to 1 byte, width to 4 bytes, and transfer size to 4.

Configuration
    ../configure --target-list=arm-softmmu --enable-debug
Reproducer
    ./qemu-system-arm -M versatilepb -m 128M -nographic -S \
    -device loader,addr=0x00000000,data=0x44332211,data-len=4 \
    -device loader,addr=0x00001000,data=0x00000000,data-len=4 \
    -device loader,addr=0x10130030,data=0x00000001,data-len=4 \
    -device loader,addr=0x10130100,data=0x00000000,data-len=4 \
    -device loader,addr=0x10130104,data=0x00001000,data-len=4 \
    -device loader,addr=0x10130108,data=0x00000000,data-len=4 \
    -device loader,addr=0x1013010C,data=0x9e47f004,data-len=4 \
    -device loader,addr=0x10130110,data=0x0000c001,data-len=4 

Qemu monitor
    (qemu) xp /1wx 0x00001000
    00001000: 0x00002211

The result correctly after fixed
    (qemu) xp /1wx 0x00001000
    00001000: 0x44332211

Signed-off-by: Tao Ding <dingtao0430@163.com>
---
 hw/dma/pl080.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/hw/dma/pl080.c b/hw/dma/pl080.c
index 0979a687aa..4f97943b28 100644
--- a/hw/dma/pl080.c
+++ b/hw/dma/pl080.c
@@ -160,26 +160,34 @@ again:
 
             /* Transfer one element.  */
             /* ??? Should transfer multiple elements for a burst request.  */
-            /* ??? Unclear what the proper behavior is when source and
-               destination widths are different.  */
             swidth = 1 << ((ch->ctrl >> 18) & 7);
             dwidth = 1 << ((ch->ctrl >> 21) & 7);
-            for (n = 0; n < dwidth; n+= swidth) {
+            xsize = (dwidth < swidth) ? swidth : dwidth;
+            xsize = (xsize < size * swidth) ? xsize : (size * swidth);
+            for (n = 0; n < xsize; n += swidth) {
                 address_space_read(&s->downstream_as, ch->src,
                                    MEMTXATTRS_UNSPECIFIED, buff + n, swidth);
-                if (ch->ctrl & PL080_CCTRL_SI)
+                if (ch->ctrl & PL080_CCTRL_SI) {
                     ch->src += swidth;
+                }
             }
-            xsize = (dwidth < swidth) ? swidth : dwidth;
-            /* ??? This may pad the value incorrectly for dwidth < 32.  */
-            for (n = 0; n < xsize; n += dwidth) {
-                address_space_write(&s->downstream_as, ch->dest + n,
-                                    MEMTXATTRS_UNSPECIFIED, buff + n, dwidth);
-                if (ch->ctrl & PL080_CCTRL_DI)
-                    ch->dest += swidth;
+            if (xsize < dwidth) {
+                address_space_write(&s->downstream_as, ch->dest,
+                                    MEMTXATTRS_UNSPECIFIED, buff, xsize);
+                if (ch->ctrl & PL080_CCTRL_DI) {
+                    ch->dest += dwidth;
+                }
+            } else {
+                for (n = 0; n < xsize; n += dwidth) {
+                    address_space_write(&s->downstream_as, ch->dest,
+                                        MEMTXATTRS_UNSPECIFIED, buff + n, dwidth);
+                    if (ch->ctrl & PL080_CCTRL_DI) {
+                        ch->dest += dwidth;
+                    }
+                }
             }
 
-            size--;
+            size -= xsize / swidth;
             ch->ctrl = (ch->ctrl & 0xfffff000) | size;
             if (size == 0) {
                 /* Transfer complete.  */
-- 
2.43.0

[PATCH v1 3/3] The LLI of pl080 should be aligned with 4 bytes.

[Tao Ding]

When the bit0 of the LLI register is configured as master 1, 
it can cause incorrect data reading, and even lead to qemu crash.

Constructed 1 LLI item. The initial LLI register points to the address of the LLI item.
But the bit 0 of the LLI register is 1, which mean read from Master1 port.
According to the description in the PL080 manual, the address for the next LLI should be word aligned.

Configuration
    ../configure --target-list=arm-softmmu --enable-debug
Reproducer
    ./qemu-system-arm -M versatilepb -m 128M -nographic -S \
     -device loader,addr=0x00002000,data=0x00000004,data-len=4 \
     -device loader,addr=0x00002004,data=0x00001004,data-len=4 \
     -device loader,addr=0x00002008,data=0x00000000,data-len=4 \
     -device loader,addr=0x0000200c,data=0x9e4bf001,data-len=4 \
     -device loader,addr=0x00000000,data=0x44332211,data-len=4 \
     -device loader,addr=0x00000004,data=0x88776655,data-len=4 \
     -device loader,addr=0x00001000,data=0x00000000,data-len=4 \
     -device loader,addr=0x00001004,data=0x00000000,data-len=4 \
     -device loader,addr=0x10130030,data=0x00000001,data-len=4 \
     -device loader,addr=0x10130100,data=0x00000000,data-len=4 \
     -device loader,addr=0x10130104,data=0x00001000,data-len=4 \
     -device loader,addr=0x10130108,data=0x00002001,data-len=4 \
     -device loader,addr=0x1013010C,data=0x1e4bf001,data-len=4 \
     -device loader,addr=0x10130110,data=0x0000c001,data-len=4  

Qemu Crash.

The result correctly after fixed
    (qemu) xp /1wx 0x00001000
     00001000: 0x44332211
     (qemu) xp /1wx 0x00001004
     00001004: 0x88776655

Signed-off-by: Tao Ding <dingtao0430@163.com>
---
 hw/dma/pl080.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/dma/pl080.c b/hw/dma/pl080.c
index 4f97943b28..4e24f6595a 100644
--- a/hw/dma/pl080.c
+++ b/hw/dma/pl080.c
@@ -102,6 +102,7 @@ static void pl080_run(PL080State *s)
     int size;
     uint8_t buff[4];
     uint32_t req;
+    uint32_t next_lli;
 
     s->tc_mask = 0;
     for (c = 0; c < s->nchannels; c++) {
@@ -191,21 +192,22 @@ again:
             ch->ctrl = (ch->ctrl & 0xfffff000) | size;
             if (size == 0) {
                 /* Transfer complete.  */
-                if (ch->lli) {
+                next_lli = (ch->lli & ~3);
+                if (next_lli) {
                     ch->src = address_space_ldl_le(&s->downstream_as,
-                                                   ch->lli,
+                                                   next_lli,
                                                    MEMTXATTRS_UNSPECIFIED,
                                                    NULL);
                     ch->dest = address_space_ldl_le(&s->downstream_as,
-                                                    ch->lli + 4,
+                                                    next_lli + 4,
                                                     MEMTXATTRS_UNSPECIFIED,
                                                     NULL);
                     ch->ctrl = address_space_ldl_le(&s->downstream_as,
-                                                    ch->lli + 12,
+                                                    next_lli + 12,
                                                     MEMTXATTRS_UNSPECIFIED,
                                                     NULL);
                     ch->lli = address_space_ldl_le(&s->downstream_as,
-                                                   ch->lli + 8,
+                                                   next_lli + 8,
                                                    MEMTXATTRS_UNSPECIFIED,
                                                    NULL);
                 } else {
-- 
2.43.0

Re: [PATCH v1 3/3] The LLI of pl080 should be aligned with 4 bytes.

[Peter Maydell]

On Thu, 12 Mar 2026 at 08:02, Tao Ding <dingtao0430@163.com> wrote:
>
> When the bit0 of the LLI register is configured as master 1,
> it can cause incorrect data reading, and even lead to qemu crash.
>
> Constructed 1 LLI item. The initial LLI register points to the address of the LLI item.
> But the bit 0 of the LLI register is 1, which mean read from Master1 port.
> According to the description in the PL080 manual, the address for the next LLI should be word aligned.
>
> Configuration
>     ../configure --target-list=arm-softmmu --enable-debug
> Reproducer
>     ./qemu-system-arm -M versatilepb -m 128M -nographic -S \
>      -device loader,addr=0x00002000,data=0x00000004,data-len=4 \
>      -device loader,addr=0x00002004,data=0x00001004,data-len=4 \
>      -device loader,addr=0x00002008,data=0x00000000,data-len=4 \
>      -device loader,addr=0x0000200c,data=0x9e4bf001,data-len=4 \
>      -device loader,addr=0x00000000,data=0x44332211,data-len=4 \
>      -device loader,addr=0x00000004,data=0x88776655,data-len=4 \
>      -device loader,addr=0x00001000,data=0x00000000,data-len=4 \
>      -device loader,addr=0x00001004,data=0x00000000,data-len=4 \
>      -device loader,addr=0x10130030,data=0x00000001,data-len=4 \
>      -device loader,addr=0x10130100,data=0x00000000,data-len=4 \
>      -device loader,addr=0x10130104,data=0x00001000,data-len=4 \
>      -device loader,addr=0x10130108,data=0x00002001,data-len=4 \
>      -device loader,addr=0x1013010C,data=0x1e4bf001,data-len=4 \
>      -device loader,addr=0x10130110,data=0x0000c001,data-len=4
>
> Qemu Crash.
>
> The result correctly after fixed
>     (qemu) xp /1wx 0x00001000
>      00001000: 0x44332211
>      (qemu) xp /1wx 0x00001004
>      00001004: 0x88776655
>
> Signed-off-by: Tao Ding <dingtao0430@163.com>
> ---

The crash only happens because of a different PL080 bug where
we weren't correctly handling invalid swidth and dwidth values,
which is fixed by this patch I sent last week:
https://patchew.org/QEMU/20260306152140.2191653-1-peter.maydell@linaro.org/

but we are certainly mishandling the LLI register here.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v1 2/3] The swidth and dwidth of pl080 are not equal.

[Peter Maydell]

On Thu, 12 Mar 2026 at 08:02, Tao Ding <dingtao0430@163.com> wrote:
>
> The amount of data transmitted each time is the maximum value between s and w,
> but it should be noted that the remaining transmission amount is less than the width.
>
> Configure swidth to 1 byte, width to 4 bytes, and transfer size to 4.
>
> Configuration
>     ../configure --target-list=arm-softmmu --enable-debug
> Reproducer
>     ./qemu-system-arm -M versatilepb -m 128M -nographic -S \
>     -device loader,addr=0x00000000,data=0x44332211,data-len=4 \
>     -device loader,addr=0x00001000,data=0x00000000,data-len=4 \
>     -device loader,addr=0x10130030,data=0x00000001,data-len=4 \
>     -device loader,addr=0x10130100,data=0x00000000,data-len=4 \
>     -device loader,addr=0x10130104,data=0x00001000,data-len=4 \
>     -device loader,addr=0x10130108,data=0x00000000,data-len=4 \
>     -device loader,addr=0x1013010C,data=0x9e47f004,data-len=4 \
>     -device loader,addr=0x10130110,data=0x0000c001,data-len=4
>
> Qemu monitor
>     (qemu) xp /1wx 0x00001000
>     00001000: 0x00002211
>
> The result correctly after fixed
>     (qemu) xp /1wx 0x00001000
>     00001000: 0x44332211
>
> Signed-off-by: Tao Ding <dingtao0430@163.com>
> ---
>  hw/dma/pl080.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/hw/dma/pl080.c b/hw/dma/pl080.c
> index 0979a687aa..4f97943b28 100644
> --- a/hw/dma/pl080.c
> +++ b/hw/dma/pl080.c
> @@ -160,26 +160,34 @@ again:
>
>              /* Transfer one element.  */
>              /* ??? Should transfer multiple elements for a burst request.  */
> -            /* ??? Unclear what the proper behavior is when source and
> -               destination widths are different.  */
>              swidth = 1 << ((ch->ctrl >> 18) & 7);
>              dwidth = 1 << ((ch->ctrl >> 21) & 7);
> -            for (n = 0; n < dwidth; n+= swidth) {
> +            xsize = (dwidth < swidth) ? swidth : dwidth;

I think this can be more clearly written as
     xsize = MAX(swidth, dwidth);
since we're editing the line anyway.

> +            xsize = (xsize < size * swidth) ? xsize : (size * swidth);

   xsize = MIN(xsize, size * swidth);

So this is handling the case where our destination width is larger
than the remaining amount the transfer size says we should be reading
from the source, which is cases like
"source width = 1, dest width = 4, transfer count not a multiple of 4".
The TRM is unfortunately very vague about what the hardware does here.
I think the hardware designers considered this to be a programming error:
in the "Software considerations" section of the TRM one of the items is:

 * If the source width is less than the destination width, the
   TransferSize value multiplied by the source width must be an
   integral multiple of the destination width.

> +            for (n = 0; n < xsize; n += swidth) {
>                  address_space_read(&s->downstream_as, ch->src,
>                                     MEMTXATTRS_UNSPECIFIED, buff + n, swidth);
> -                if (ch->ctrl & PL080_CCTRL_SI)
> +                if (ch->ctrl & PL080_CCTRL_SI) {
>                      ch->src += swidth;
> +                }
>              }
> -            xsize = (dwidth < swidth) ? swidth : dwidth;
> -            /* ??? This may pad the value incorrectly for dwidth < 32.  */
> -            for (n = 0; n < xsize; n += dwidth) {
> -                address_space_write(&s->downstream_as, ch->dest + n,
> -                                    MEMTXATTRS_UNSPECIFIED, buff + n, dwidth);
> -                if (ch->ctrl & PL080_CCTRL_DI)
> -                    ch->dest += swidth;
> +            if (xsize < dwidth) {
> +                address_space_write(&s->downstream_as, ch->dest,
> +                                    MEMTXATTRS_UNSPECIFIED, buff, xsize);
> +                if (ch->ctrl & PL080_CCTRL_DI) {
> +                    ch->dest += dwidth;
> +                }

Here we choose to handle the "swidth and tsize say we should
read e.g. 2 bytes from the source but dwidth is e.g. 4" case
by performing a 2 byte write to the destination. I don't think this
is what the hardware would do.

There is this interesting bit in the "software considerations" section:

 * While writing, the TransferSize bit-field is like a control register
   because it determines how many transfers the DMAC performs. However,
   during read-back, TransferSize behaves like a status register because
   it returns the number of remaining transfers in terms of source width.
   So when TransferSize is read back, it returns the number of
   destination-transfer-completed stored in a separate counter called
   TrfSizeDst multiplied by a factor.

My best guess from stuff like this is that the way the PL080 works
internally is that for each dest-write it needs to do for the transfer
it does the appropriate number of source-reads. If the guest
misprograms it such that that means it reads more than
(transferwidth * swidth) that will result in reading more data
from the source than intended, but that would be a software bug.

I would suggest doing something like that, but at any rate
commenting and doing a LOG_GUEST_ERROR if there's a mismatch in
swidth/dwidth/tsize such that we wouldn't have enough bytes to
do a full destination-write.

> +            } else {
> +                for (n = 0; n < xsize; n += dwidth) {
> +                    address_space_write(&s->downstream_as, ch->dest,
> +                                        MEMTXATTRS_UNSPECIFIED, buff + n, dwidth);
> +                    if (ch->ctrl & PL080_CCTRL_DI) {
> +                        ch->dest += dwidth;
> +                    }
> +                }
>              }
>
> -            size--;
> +            size -= xsize / swidth;

This change is correct (the transfer size field in the control register
is supposed to count down by 1 for each swidth-sized data unit we
transfer, and our loop can do more than one swidth read), and probably
the most important bugfix in this patch, because it's the one that
matters for normal correct use of the PL080.

>              ch->ctrl = (ch->ctrl & 0xfffff000) | size;
>              if (size == 0) {
>                  /* Transfer complete.  */
> --
> 2.43.0

thanks
-- PMM

Re: [PATCH v1 0/3] Fixed bugs in pl080

[Peter Maydell]

On Thu, 12 Mar 2026 at 08:02, Tao Ding <dingtao0430@163.com> wrote:
>
> Hi all,
> When I tested pl080 using the existing dmatest tool in Linux,
> I found that pl080 could not output the results correctly.
> The main issues involved are inconsistent source and destination bit widths,
> misalignment of LLI, which can lead to incorrect data and even cause qemu crashes.
>
> Tao Ding (3):
>   An interrupt is generated after the DMA transfer complete.
>   The swidth and dwidth of pl080 are not equal.
>   The LLI of pl080 should be aligned with 4 bytes.

Hi; I've taken patches 1 and 3 into target-arm.next (with
some adjustments to the commit messages). I had some
review comments on patch 2.

thanks
-- PMM

Re: [PATCH v1 2/3] The swidth and dwidth of pl080 are not equal.

[Tao Ding]

[-- Attachment #1: Type: text/plain, Size: 5645 bytes --]

Thank you for your valuable review.

I agree with your suggestion, it is necessary to satisfy that transfersize * swidth is
an integer multiple of dwidth. So, some of the code is actually redundant in this patch.
Before that, swidth,dwidth and transfersize will be checked, and if the conditions are not met,
there will be qemu_log_mask(LOG_GUEST_ERROR).

Can I modify this patch and submit it as patch v2.

>> ---
>>   hw/dma/pl080.c | 32 ++++++++++++++++++++------------
>>   1 file changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/dma/pl080.c b/hw/dma/pl080.c
>> index 0979a687aa..4f97943b28 100644
>> --- a/hw/dma/pl080.c
>> +++ b/hw/dma/pl080.c
>> @@ -160,26 +160,34 @@ again:
>>
>>               /* Transfer one element.  */
>>               /* ??? Should transfer multiple elements for a burst request.  */
>> -            /* ??? Unclear what the proper behavior is when source and
>> -               destination widths are different.  */
>>               swidth = 1 << ((ch->ctrl >> 18) & 7);
>>               dwidth = 1 << ((ch->ctrl >> 21) & 7);
>> -            for (n = 0; n < dwidth; n+= swidth) {
>> +            xsize = (dwidth < swidth) ? swidth : dwidth;
> I think this can be more clearly written as
>       xsize = MAX(swidth, dwidth);
> since we're editing the line anyway.
>
>> +            xsize = (xsize < size * swidth) ? xsize : (size * swidth);
>     xsize = MIN(xsize, size * swidth);
>
> So this is handling the case where our destination width is larger
> than the remaining amount the transfer size says we should be reading
> from the source, which is cases like
> "source width = 1, dest width = 4, transfer count not a multiple of 4".
> The TRM is unfortunately very vague about what the hardware does here.
> I think the hardware designers considered this to be a programming error:
> in the "Software considerations" section of the TRM one of the items is:
>
>   * If the source width is less than the destination width, the
>     TransferSize value multiplied by the source width must be an
>     integral multiple of the destination width.
>
>> +            for (n = 0; n < xsize; n += swidth) {
>>                   address_space_read(&s->downstream_as, ch->src,
>>                                      MEMTXATTRS_UNSPECIFIED, buff + n, swidth);
>> -                if (ch->ctrl & PL080_CCTRL_SI)
>> +                if (ch->ctrl & PL080_CCTRL_SI) {
>>                       ch->src += swidth;
>> +                }
>>               }
>> -            xsize = (dwidth < swidth) ? swidth : dwidth;
>> -            /* ??? This may pad the value incorrectly for dwidth < 32.  */
>> -            for (n = 0; n < xsize; n += dwidth) {
>> -                address_space_write(&s->downstream_as, ch->dest + n,
>> -                                    MEMTXATTRS_UNSPECIFIED, buff + n, dwidth);
>> -                if (ch->ctrl & PL080_CCTRL_DI)
>> -                    ch->dest += swidth;
>> +            if (xsize < dwidth) {
>> +                address_space_write(&s->downstream_as, ch->dest,
>> +                                    MEMTXATTRS_UNSPECIFIED, buff, xsize);
>> +                if (ch->ctrl & PL080_CCTRL_DI) {
>> +                    ch->dest += dwidth;
>> +                }
> Here we choose to handle the "swidth and tsize say we should
> read e.g. 2 bytes from the source but dwidth is e.g. 4" case
> by performing a 2 byte write to the destination. I don't think this
> is what the hardware would do.
>
> There is this interesting bit in the "software considerations" section:
>
>   * While writing, the TransferSize bit-field is like a control register
>     because it determines how many transfers the DMAC performs. However,
>     during read-back, TransferSize behaves like a status register because
>     it returns the number of remaining transfers in terms of source width.
>     So when TransferSize is read back, it returns the number of
>     destination-transfer-completed stored in a separate counter called
>     TrfSizeDst multiplied by a factor.
>
> My best guess from stuff like this is that the way the PL080 works
> internally is that for each dest-write it needs to do for the transfer
> it does the appropriate number of source-reads. If the guest
> misprograms it such that that means it reads more than
> (transferwidth * swidth) that will result in reading more data
> from the source than intended, but that would be a software bug.
>
> I would suggest doing something like that, but at any rate
> commenting and doing a LOG_GUEST_ERROR if there's a mismatch in
> swidth/dwidth/tsize such that we wouldn't have enough bytes to
> do a full destination-write.
>
>> +            } else {
>> +                for (n = 0; n < xsize; n += dwidth) {
>> +                    address_space_write(&s->downstream_as, ch->dest,
>> +                                        MEMTXATTRS_UNSPECIFIED, buff + n, dwidth);
>> +                    if (ch->ctrl & PL080_CCTRL_DI) {
>> +                        ch->dest += dwidth;
>> +                    }
>> +                }
>>               }
>>
>> -            size--;
>> +            size -= xsize / swidth;
> This change is correct (the transfer size field in the control register
> is supposed to count down by 1 for each swidth-sized data unit we
> transfer, and our loop can do more than one swidth read), and probably
> the most important bugfix in this patch, because it's the one that
> matters for normal correct use of the PL080.
>
>>               ch->ctrl = (ch->ctrl & 0xfffff000) | size;
>>               if (size == 0) {
>>                   /* Transfer complete.  */
>> --
>> 2.43.0
> thanks
> -- PMM

[-- Attachment #2: Type: text/html, Size: 6635 bytes --]

[PATCH v2 0/1] Fix transfer size register decrement

[Tao Ding]

When source and destination widths are different, the transfer size
register was not being decremented correctly, causing data corruption.

Compared to the previous version(PATCH v1 2/3), simplified the code and
the judgment of constraint relationships between size, swidth, and dwidth has been added.

Tao Ding (1):
  Fix transfer size register decrement in pl080

 hw/dma/pl080.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)


base-commit: 8e711856d7639cbffa51405f2cc2366e3d9e3a23
-- 
2.43.0

[PATCH v2 1/1] Fix transfer size register decrement in pl080

[Tao Ding]

Configure swidth to 1 byte, width to 4 bytes, and transfer size to 4.

Configuration
    ../configure --target-list=arm-softmmu --enable-debug
Reproducer
    ./qemu-system-arm -M versatilepb -m 128M -nographic -S \
    -device loader,addr=0x00000000,data=0x44332211,data-len=4 \
    -device loader,addr=0x00001000,data=0x00000000,data-len=4 \
    -device loader,addr=0x10130030,data=0x00000001,data-len=4 \
    -device loader,addr=0x10130100,data=0x00000000,data-len=4 \
    -device loader,addr=0x10130104,data=0x00001000,data-len=4 \
    -device loader,addr=0x10130108,data=0x00000000,data-len=4 \
    -device loader,addr=0x1013010C,data=0x9e47f004,data-len=4 \
    -device loader,addr=0x10130110,data=0x0000c001,data-len=4 

Qemu monitor
    (qemu) xp /1wx 0x00001000
    00001000: 0x00002211

The result correctly after fixed
    (qemu) xp /1wx 0x00001000
    00001000: 0x44332211

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Tao Ding <dingtao0430@163.com>
---
 hw/dma/pl080.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/dma/pl080.c b/hw/dma/pl080.c
index 627ccbbd81..4a90c7bb27 100644
--- a/hw/dma/pl080.c
+++ b/hw/dma/pl080.c
@@ -179,23 +179,28 @@ again:
                               c, extract32(ch->ctrl, 21, 3));
                 continue;
             }
-
-            for (n = 0; n < dwidth; n+= swidth) {
+            if ((size * swidth) % dwidth) {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                    "pl080: channel %d: transfer size mismatch: size=%d swidth=%d dwidth=%d\n",
+                    c, size, swidth, dwidth);
+                continue;
+            }
+            xsize = MAX(swidth, dwidth);
+            for (n = 0; n < xsize; n += swidth) {
                 address_space_read(&s->downstream_as, ch->src,
                                    MEMTXATTRS_UNSPECIFIED, buff + n, swidth);
                 if (ch->ctrl & PL080_CCTRL_SI)
                     ch->src += swidth;
             }
-            xsize = (dwidth < swidth) ? swidth : dwidth;
             /* ??? This may pad the value incorrectly for dwidth < 32.  */
             for (n = 0; n < xsize; n += dwidth) {
-                address_space_write(&s->downstream_as, ch->dest + n,
+                address_space_write(&s->downstream_as, ch->dest,
                                     MEMTXATTRS_UNSPECIFIED, buff + n, dwidth);
                 if (ch->ctrl & PL080_CCTRL_DI)
-                    ch->dest += swidth;
+                    ch->dest += dwidth;
             }
 
-            size--;
+            size -= xsize / swidth;
             ch->ctrl = (ch->ctrl & 0xfffff000) | size;
             if (size == 0) {
                 /* Transfer complete.  */
-- 
2.43.0

Re: [PATCH v2 1/1] Fix transfer size register decrement in pl080

[Peter Maydell]

On Mon, 23 Mar 2026 at 16:50, Tao Ding <dingtao0430@163.com> wrote:
>
> Configure swidth to 1 byte, width to 4 bytes, and transfer size to 4.
>
> Configuration
>     ../configure --target-list=arm-softmmu --enable-debug
> Reproducer
>     ./qemu-system-arm -M versatilepb -m 128M -nographic -S \
>     -device loader,addr=0x00000000,data=0x44332211,data-len=4 \
>     -device loader,addr=0x00001000,data=0x00000000,data-len=4 \
>     -device loader,addr=0x10130030,data=0x00000001,data-len=4 \
>     -device loader,addr=0x10130100,data=0x00000000,data-len=4 \
>     -device loader,addr=0x10130104,data=0x00001000,data-len=4 \
>     -device loader,addr=0x10130108,data=0x00000000,data-len=4 \
>     -device loader,addr=0x1013010C,data=0x9e47f004,data-len=4 \
>     -device loader,addr=0x10130110,data=0x0000c001,data-len=4
>
> Qemu monitor
>     (qemu) xp /1wx 0x00001000
>     00001000: 0x00002211
>
> The result correctly after fixed
>     (qemu) xp /1wx 0x00001000
>     00001000: 0x44332211
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Tao Ding <dingtao0430@163.com>
> ---
>  hw/dma/pl080.c | 17 +++++++++++------

Thanks for this patch; I've applied it to target-arm.next
(with an expansion to the commit message to describe the
bugs it is fixing).

-- PMM