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

[Hervé Poussineau]

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 <address@hidden>
> > ---
> >   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é