diff mbox

[v3,16/16,RFC] dma/rc4030: do multiple calls to address_space_rw when doing DMA transfers

Message ID 1433364350-19380-17-git-send-email-hpoussin@reactos.org
State New
Headers show

Commit Message

Hervé Poussineau June 3, 2015, 8:45 p.m. UTC 
This fixes Windows NT 4.0/MIPS, which was always bugchecking with
IRQL_NOT_LESS_OR_EQUAL.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/dma/rc4030.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Leon Alrae June 10, 2015, noon UTC | #1 
Hi Hervé,

On 03/06/2015 21:45, Hervé Poussineau wrote:
> This fixes Windows NT 4.0/MIPS, which was always bugchecking with
> IRQL_NOT_LESS_OR_EQUAL.
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
>  hw/dma/rc4030.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
> index 3efa6de..deac0a8 100644
> --- a/hw/dma/rc4030.c
> +++ b/hw/dma/rc4030.c
> @@ -681,6 +681,7 @@ static void rc4030_do_dma(void *opaque, int n, uint8_t *buf, int len, int is_wri
>      rc4030State *s = opaque;
>      hwaddr dma_addr;
>      int dev_to_mem;
> +    int i;
>  
>      s->dma_regs[n][DMA_REG_ENABLE] &= ~(DMA_FLAG_TC_INTR | DMA_FLAG_MEM_INTR | DMA_FLAG_ADDR_INTR);
>  
> @@ -699,8 +700,17 @@ static void rc4030_do_dma(void *opaque, int n, uint8_t *buf, int len, int is_wri
>      dma_addr = s->dma_regs[n][DMA_REG_ADDRESS];
>  
>      /* Read/write data at right place */
> -    address_space_rw(&s->dma_as, dma_addr, MEMTXATTRS_UNSPECIFIED,
> -                     buf, len, is_write);
> +    for (i = 0; i < len; ) {
> +        int ncpy = DMA_PAGESIZE - (dma_addr & (DMA_PAGESIZE - 1));
> +        if (ncpy > len - i) {
> +            ncpy = len - i;
> +        }
> +        address_space_rw(&s->dma_as, dma_addr, MEMTXATTRS_UNSPECIFIED,
> +                         buf + i, ncpy, is_write);
> +
> +        dma_addr += ncpy;
> +        i += ncpy;
> +    }
>  
>      s->dma_regs[n][DMA_REG_ENABLE] |= DMA_FLAG_TC_INTR;
>      s->dma_regs[n][DMA_REG_COUNT] -= len;
> 

I'm sending out current target-mips queue soon and I can include this series,
but I'm not quite sure what to do with this RFC patch. I presume you put RFC
here because it's a workaround for a bug, but it's not clear where the actual
bug is?

Thanks,
Leon
Hervé Poussineau June 10, 2015, 7:37 p.m. UTC | #2 
Hi Leon,

Le 10/06/2015 14:00, Leon Alrae a écrit :
> Hi Hervé,
>
> On 03/06/2015 21:45, Hervé Poussineau wrote:
>> This fixes Windows NT 4.0/MIPS, which was always bugchecking with
>> IRQL_NOT_LESS_OR_EQUAL.
>>
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> ---
>>   hw/dma/rc4030.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
>> index 3efa6de..deac0a8 100644
>> --- a/hw/dma/rc4030.c
>> +++ b/hw/dma/rc4030.c
>> @@ -681,6 +681,7 @@ static void rc4030_do_dma(void *opaque, int n, uint8_t *buf, int len, int is_wri
>>       rc4030State *s = opaque;
>>       hwaddr dma_addr;
>>       int dev_to_mem;
>> +    int i;
>>
>>       s->dma_regs[n][DMA_REG_ENABLE] &= ~(DMA_FLAG_TC_INTR | DMA_FLAG_MEM_INTR | DMA_FLAG_ADDR_INTR);
>>
>> @@ -699,8 +700,17 @@ static void rc4030_do_dma(void *opaque, int n, uint8_t *buf, int len, int is_wri
>>       dma_addr = s->dma_regs[n][DMA_REG_ADDRESS];
>>
>>       /* Read/write data at right place */
>> -    address_space_rw(&s->dma_as, dma_addr, MEMTXATTRS_UNSPECIFIED,
>> -                     buf, len, is_write);
>> +    for (i = 0; i < len; ) {
>> +        int ncpy = DMA_PAGESIZE - (dma_addr & (DMA_PAGESIZE - 1));
>> +        if (ncpy > len - i) {
>> +            ncpy = len - i;
>> +        }
>> +        address_space_rw(&s->dma_as, dma_addr, MEMTXATTRS_UNSPECIFIED,
>> +                         buf + i, ncpy, is_write);
>> +
>> +        dma_addr += ncpy;
>> +        i += ncpy;
>> +    }
>>
>>       s->dma_regs[n][DMA_REG_ENABLE] |= DMA_FLAG_TC_INTR;
>>       s->dma_regs[n][DMA_REG_COUNT] -= len;
>>
>
> I'm sending out current target-mips queue soon and I can include this series,
> but I'm not quite sure what to do with this RFC patch. I presume you put RFC
> here because it's a workaround for a bug, but it's not clear where the actual
> bug is?

Indeed, that's a workaround for a bug that I failed to identify. You can still take the RFC patch as a patch, as I didn't see any objection (yet).
I think we can defer the bug correction for later.

Regards,

Hervé
Peter Maydell June 10, 2015, 8:20 p.m. UTC | #3 
On 10 June 2015 at 20:37, Hervé Poussineau <hpoussin@reactos.org> wrote:
> Hi Leon,
>
>
> Le 10/06/2015 14:00, Leon Alrae a écrit :
>>
>> Hi Hervé,
>>
>> On 03/06/2015 21:45, Hervé Poussineau wrote:
>>>
>>> This fixes Windows NT 4.0/MIPS, which was always bugchecking with
>>> IRQL_NOT_LESS_OR_EQUAL.
>>>
>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>> ---
>>>   hw/dma/rc4030.c | 14 ++++++++++++--
>>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
>>> index 3efa6de..deac0a8 100644
>>> --- a/hw/dma/rc4030.c
>>> +++ b/hw/dma/rc4030.c
>>> @@ -681,6 +681,7 @@ static void rc4030_do_dma(void *opaque, int n,
>>> uint8_t *buf, int len, int is_wri
>>>       rc4030State *s = opaque;
>>>       hwaddr dma_addr;
>>>       int dev_to_mem;
>>> +    int i;
>>>
>>>       s->dma_regs[n][DMA_REG_ENABLE] &= ~(DMA_FLAG_TC_INTR |
>>> DMA_FLAG_MEM_INTR | DMA_FLAG_ADDR_INTR);
>>>
>>> @@ -699,8 +700,17 @@ static void rc4030_do_dma(void *opaque, int n,
>>> uint8_t *buf, int len, int is_wri
>>>       dma_addr = s->dma_regs[n][DMA_REG_ADDRESS];
>>>
>>>       /* Read/write data at right place */
>>> -    address_space_rw(&s->dma_as, dma_addr, MEMTXATTRS_UNSPECIFIED,
>>> -                     buf, len, is_write);
>>> +    for (i = 0; i < len; ) {
>>> +        int ncpy = DMA_PAGESIZE - (dma_addr & (DMA_PAGESIZE - 1));
>>> +        if (ncpy > len - i) {
>>> +            ncpy = len - i;
>>> +        }
>>> +        address_space_rw(&s->dma_as, dma_addr, MEMTXATTRS_UNSPECIFIED,
>>> +                         buf + i, ncpy, is_write);
>>> +
>>> +        dma_addr += ncpy;
>>> +        i += ncpy;
>>> +    }
>>>
>>>       s->dma_regs[n][DMA_REG_ENABLE] |= DMA_FLAG_TC_INTR;
>>>       s->dma_regs[n][DMA_REG_COUNT] -= len;
>>>
>>
>> I'm sending out current target-mips queue soon and I can include this
>> series,
>> but I'm not quite sure what to do with this RFC patch. I presume you put
>> RFC
>> here because it's a workaround for a bug, but it's not clear where the
>> actual
>> bug is?
>
>
> Indeed, that's a workaround for a bug that I failed to identify. You can
> still take the RFC patch as a patch, as I didn't see any objection (yet).
> I think we can defer the bug correction for later.

...but address_space_rw() is supposed to be the same as if
you do a bunch of shorter writes, so if we have a problem
in the memory system core we should really track it down...

Paolo, does this look familiar at all?

-- PMM
Peter Maydell June 10, 2015, 8:22 p.m. UTC | #4 
On 10 June 2015 at 20:37, Hervé Poussineau <hpoussin@reactos.org> wrote:
> Hi Leon,
>
>
> Le 10/06/2015 14:00, Leon Alrae a écrit :
>>
>> Hi Hervé,
>>
>> On 03/06/2015 21:45, Hervé Poussineau wrote:
>>>
>>> This fixes Windows NT 4.0/MIPS, which was always bugchecking with
>>> IRQL_NOT_LESS_OR_EQUAL.
>>>
>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>> ---
>>>   hw/dma/rc4030.c | 14 ++++++++++++--
>>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
>>> index 3efa6de..deac0a8 100644
>>> --- a/hw/dma/rc4030.c
>>> +++ b/hw/dma/rc4030.c
>>> @@ -681,6 +681,7 @@ static void rc4030_do_dma(void *opaque, int n,
>>> uint8_t *buf, int len, int is_wri
>>>       rc4030State *s = opaque;
>>>       hwaddr dma_addr;
>>>       int dev_to_mem;
>>> +    int i;
>>>
>>>       s->dma_regs[n][DMA_REG_ENABLE] &= ~(DMA_FLAG_TC_INTR |
>>> DMA_FLAG_MEM_INTR | DMA_FLAG_ADDR_INTR);
>>>
>>> @@ -699,8 +700,17 @@ static void rc4030_do_dma(void *opaque, int n,
>>> uint8_t *buf, int len, int is_wri
>>>       dma_addr = s->dma_regs[n][DMA_REG_ADDRESS];
>>>
>>>       /* Read/write data at right place */
>>> -    address_space_rw(&s->dma_as, dma_addr, MEMTXATTRS_UNSPECIFIED,
>>> -                     buf, len, is_write);
>>> +    for (i = 0; i < len; ) {
>>> +        int ncpy = DMA_PAGESIZE - (dma_addr & (DMA_PAGESIZE - 1));
>>> +        if (ncpy > len - i) {
>>> +            ncpy = len - i;
>>> +        }
>>> +        address_space_rw(&s->dma_as, dma_addr, MEMTXATTRS_UNSPECIFIED,
>>> +                         buf + i, ncpy, is_write);
>>> +
>>> +        dma_addr += ncpy;
>>> +        i += ncpy;
>>> +    }
>>>
>>>       s->dma_regs[n][DMA_REG_ENABLE] |= DMA_FLAG_TC_INTR;
>>>       s->dma_regs[n][DMA_REG_COUNT] -= len;
>>>
>>
>> I'm sending out current target-mips queue soon and I can include this
>> series,
>> but I'm not quite sure what to do with this RFC patch. I presume you put
>> RFC
>> here because it's a workaround for a bug, but it's not clear where the
>> actual
>> bug is?
>
>
> Indeed, that's a workaround for a bug that I failed to identify. You can
> still take the RFC patch as a patch, as I didn't see any objection (yet).
> I think we can defer the bug correction for later.

At a minimum, we need a comment saying that we're doing this weird
thing as a workaround (and ideally with enough info in the commit
message to reproduce so that at some later date we can confirm whether
we've really fixed the underlying issue and can revert this).

-- PMM
diff mbox

Patch

diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
index 3efa6de..deac0a8 100644
--- a/hw/dma/rc4030.c
+++ b/hw/dma/rc4030.c
@@ -681,6 +681,7 @@  static void rc4030_do_dma(void *opaque, int n, uint8_t *buf, int len, int is_wri
     rc4030State *s = opaque;
     hwaddr dma_addr;
     int dev_to_mem;
+    int i;
 
     s->dma_regs[n][DMA_REG_ENABLE] &= ~(DMA_FLAG_TC_INTR | DMA_FLAG_MEM_INTR | DMA_FLAG_ADDR_INTR);
 
@@ -699,8 +700,17 @@  static void rc4030_do_dma(void *opaque, int n, uint8_t *buf, int len, int is_wri
     dma_addr = s->dma_regs[n][DMA_REG_ADDRESS];
 
     /* Read/write data at right place */
-    address_space_rw(&s->dma_as, dma_addr, MEMTXATTRS_UNSPECIFIED,
-                     buf, len, is_write);
+    for (i = 0; i < len; ) {
+        int ncpy = DMA_PAGESIZE - (dma_addr & (DMA_PAGESIZE - 1));
+        if (ncpy > len - i) {
+            ncpy = len - i;
+        }
+        address_space_rw(&s->dma_as, dma_addr, MEMTXATTRS_UNSPECIFIED,
+                         buf + i, ncpy, is_write);
+
+        dma_addr += ncpy;
+        i += ncpy;
+    }
 
     s->dma_regs[n][DMA_REG_ENABLE] |= DMA_FLAG_TC_INTR;
     s->dma_regs[n][DMA_REG_COUNT] -= len;