Re: [Qemu-devel] [PATCH 1/2] hw/mips/jazz: create ESP device directly via qdev

[Mark Cave-Ayland]

On 13/06/18 11:06, Paolo Bonzini wrote:

> On 13/06/2018 11:47, Mark Cave-Ayland wrote:
> > +    dev = qdev_create(NULL, TYPE_ESP);
> > +    sysbus_esp = ESP_STATE(dev);
> > +    esp = &sysbus_esp->esp;
> > +    esp->dma_memory_read = rc4030_dma_read;
> > +    esp->dma_memory_write = rc4030_dma_write;
> > +    esp->dma_opaque = dmas[0];
>
> Poking at the functions here is a bit ugly, and it's the last user of
> rc4030_dma_{read,write}.  It would be nicer if ESP could get a memory
> region like it's done a bit above for the NIC.  I guess it's not a big
> deal, but perhaps there could be a TODO comment.

Okay - I'll wait for Hervé to confirm it passes his tests and then see 
if wants a respin with a TODO added (or can add it himself).

> I'm mostly mentioning this because Hervé is copied and because SPARC DMA
> has the same issue of using function pointers instead of an IOMMU memory
> region...

Ahhh this is a fun one :)

As part of the SPARC32 cleanups for the last release there is now a 
proper sun4m IOMMU implementation, but from memory the big issue was 
that the DMA functions need access to the device state to affect the 
transfer.

Check out hw/dma/sparc32_dma.c for some ugly examples: 
espdma_memory_read()/espdma_memory_write() update the DMA address 
pointer register after each read/write, and 
ledma_memory_read()/ledma_memory_write() need to determine if the pcnet 
card has byte-swapping enabled: if so, then don't perform the required 
lance 16-bit swap and if not, do perform the 16-bit swap.

So yes, it's fairly ugly but I can't think of good solution involving 
just IOMMU memory regions.


ATB,

Mark.