arm: Implement MPS2 watchdogs and DMA

[Qemu-devel] [PATCH 14/16] hw/dma/pl080: Correct bug in register address decode logic

Posted by Peter Maydell 7 years, 6 months ago

A bug in the handling of the register address decode logic
for the PL08x meant that we were incorrectly treating
accesses to the DMA channel registers (DMACCxSrcAddr,
DMACCxDestaddr, DMACCxLLI, DMACCxControl, DMACCxConfiguration)
as bad offsets. Fix this long-standing bug.

Fixes: https://bugs.launchpad.net/qemu/+bug/1637974
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This has been around for a long time, identified by code
inspection several years ago in the LP bug. Now I have
some guest code that actually tries to use the PL08x I
can test the fix...
---
 hw/dma/pl080.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/dma/pl080.c b/hw/dma/pl080.c
index a7aacad74f0..8f92550392b 100644
--- a/hw/dma/pl080.c
+++ b/hw/dma/pl080.c
@@ -229,7 +229,7 @@ static uint64_t pl080_read(void *opaque, hwaddr offset,
         i = (offset & 0xe0) >> 5;
         if (i >= s->nchannels)
             goto bad_offset;
-        switch (offset >> 2) {
+        switch ((offset >> 2) & 7) {
         case 0: /* SrcAddr */
             return s->chan[i].src;
         case 1: /* DestAddr */
@@ -290,7 +290,7 @@ static void pl080_write(void *opaque, hwaddr offset,
         i = (offset & 0xe0) >> 5;
         if (i >= s->nchannels)
             goto bad_offset;
-        switch (offset >> 2) {
+        switch ((offset >> 2) & 7) {
         case 0: /* SrcAddr */
             s->chan[i].src = value;
             break;
@@ -308,6 +308,7 @@ static void pl080_write(void *opaque, hwaddr offset,
             pl080_run(s);
             break;
         }
+        return;
     }
     switch (offset >> 2) {
     case 2: /* IntTCClear */
-- 
2.17.1

Re: [Qemu-devel] [Qemu-arm] [PATCH 14/16] hw/dma/pl080: Correct bug in register address decode logic

Posted by Philippe Mathieu-Daudé 7 years, 5 months ago

On 08/09/2018 10:01 AM, Peter Maydell wrote:
> A bug in the handling of the register address decode logic
> for the PL08x meant that we were incorrectly treating
> accesses to the DMA channel registers (DMACCxSrcAddr,
> DMACCxDestaddr, DMACCxLLI, DMACCxControl, DMACCxConfiguration)
> as bad offsets. Fix this long-standing bug.

Since this file's origin (cdbdb648b7c).

> 
> Fixes: https://bugs.launchpad.net/qemu/+bug/1637974
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This has been around for a long time, identified by code
> inspection several years ago in the LP bug. Now I have
> some guest code that actually tries to use the PL08x I
> can test the fix...
> ---
>  hw/dma/pl080.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/dma/pl080.c b/hw/dma/pl080.c
> index a7aacad74f0..8f92550392b 100644
> --- a/hw/dma/pl080.c
> +++ b/hw/dma/pl080.c
> @@ -229,7 +229,7 @@ static uint64_t pl080_read(void *opaque, hwaddr offset,
>          i = (offset & 0xe0) >> 5;
>          if (i >= s->nchannels)
>              goto bad_offset;
> -        switch (offset >> 2) {
> +        switch ((offset >> 2) & 7) {

So only the first channel ever worked...

>          case 0: /* SrcAddr */
>              return s->chan[i].src;
>          case 1: /* DestAddr */
> @@ -290,7 +290,7 @@ static void pl080_write(void *opaque, hwaddr offset,
>          i = (offset & 0xe0) >> 5;
>          if (i >= s->nchannels)
>              goto bad_offset;
> -        switch (offset >> 2) {
> +        switch ((offset >> 2) & 7) {
>          case 0: /* SrcAddr */
>              s->chan[i].src = value;
>              break;
> @@ -308,6 +308,7 @@ static void pl080_write(void *opaque, hwaddr offset,
>              pl080_run(s);
>              break;
>          }
> +        return;
>      }
>      switch (offset >> 2) {

Eventually copy/pasted from here.

>      case 2: /* IntTCClear */
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [Qemu-arm] [PATCH 14/16] hw/dma/pl080: Correct bug in register address decode logic

Posted by Peter Maydell 7 years, 5 months ago

On 15 August 2018 at 15:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 08/09/2018 10:01 AM, Peter Maydell wrote:
>> diff --git a/hw/dma/pl080.c b/hw/dma/pl080.c
>> index a7aacad74f0..8f92550392b 100644
>> --- a/hw/dma/pl080.c
>> +++ b/hw/dma/pl080.c
>> @@ -229,7 +229,7 @@ static uint64_t pl080_read(void *opaque, hwaddr offset,
>>          i = (offset & 0xe0) >> 5;
>>          if (i >= s->nchannels)
>>              goto bad_offset;
>> -        switch (offset >> 2) {
>> +        switch ((offset >> 2) & 7) {
>
> So only the first channel ever worked...

Not even that -- the per-channel registers are in the
0x100..0x200 region, so channel 0's registers start at 0x100.


> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


thanks
-- PMM