Re: [Qemu-devel] [PATCH] dma/rc4030: do multiple calls to address_space_rw when doing DMA transfers

["Hervé Poussineau" <hpoussin@reactos.org>]

Hi Aurelien,

Le 12/06/2015 01:30, Aurelien Jarno a écrit :
> On 2015-06-11 22:30, Hervé Poussineau wrote:
>> This workarounds a bug in memory management.
>>
>> To reproduce the problem, try to start the Windows NT 4.0/MIPS installer.
>> After loading some files, you should see a screen saying
>> "To set up Windows NT now, press ENTER."
>> However, you're welcomed with an IRQL_NOT_LESS_OR_EQUAL bugcheck or an
>> Unknown Hard Error c0000221.
>>
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> ---
>>   hw/dma/rc4030.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
>> index 3efa6de..d265d6c 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,22 @@ 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 */
>> +#if 1 /* workaround for a bug in memory management */
>> +    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;
>> +    }
>> +#else
>>       address_space_rw(&s->dma_as, dma_addr, MEMTXATTRS_UNSPECIFIED,
>>                        buf, len, is_write);
>> +#endif
>
> Hmm, basically your code splits the transfers so that they don't cross
> DMA page boundaries. It seems that your DMA memory region is actually
> made of small subregions of size DMA_PAGESIZE aliased to the RAM.

Yes, that's the case. I have lots of DMA_PAGESIZE memory region aliases in the DMA memory region.

> Now looking at the address_space_rw function, it seems it optimizes the
> write to RAM case by calling address_space_translate() and then doing a
> memcpy() of the whole region. It doesn't work given the memory region is
> not linear.
>
> That said address_space_translate is supposed to adjust the length if
> needed, but does so only if iommu_ops is defined.

Then, the problem lies here.
If you can use address_space_rw only on an address range which is linear in underlying memory region, or if underlying memory region is a iommu, then you have a big problem. As you can't query if 
that's the case, your only bet is to use address_space_rw with only 1 byte quantities...
Adding Paolo, as he may have an idea.

 > I therefore wonder if
> you therefore shouldn't model this DMA translation tables by using IOMMU
> ops instead of subregions.
>
No, in my opinion, that's an implementation detail. Paolo said that it was OK:
"Both are okay.  The IOMMU makes address space changes faster; your
scheme is basically a form of caching, it trades update performance for
improved translation performance."
http://lists.gnu.org/archive/html/qemu-devel/2015-03/msg05486.html

Regards,

Hervé