Re: [PATCH v3 3/3] hw/usb/xhci: Always expect 'dma' link property to be set

[Mark Cave-Ayland]

On 27/08/2021 09:54, Peter Maydell wrote:

> > In a way I could see why you may wish to explicitly set the DMA memory region, 
> > but a
> > quick look around suggests that devices where the memory region is unspecified
> > (typically using a link property called "dma_mr") then the default is assumed 
> > to be
> > get_system_memory(). That seems a reasonably intuitive default to me, but 
> > presumably
> > there is another type of mistake you're trying to guard against here?
>
> Mostly we have allowed a default for "dma link not set" as a transitional
> thing. When we added the 'dma' links to a device which had multiple users
> and we didn't at the time want to go through and modify all those users to
> make sure they all set the link, we made the device default if the link
> wasn't set be "behave the same way the device behaved before we added support
> for the link property". I think it's useful cleanup to get rid of the
> back-compat
> default -- from a theoretical perspective devices should mostly not
> be directly grabbing and using the system_memory.

Ah so the plan moving forward is to always have an explicit MR passed in for DMA use. 
Sorry if I missed that in earlier versions of the patchset, I'm still getting back up 
to speed on QEMU hacking.

Was there a decision as to what the property name should be? I see "dma_mr" used 
quite a bit, and if there will be more patches to remove the system_memory default 
from other devices then it would be nice to try and use the same name everywhere.

> > > @@ -43,13 +48,7 @@ static void xhci_sysbus_realize(DeviceState *dev, Error 
> > > **errp)
> > >        s->irq = g_new0(qemu_irq, s->xhci.numintrs);
> > >        qdev_init_gpio_out_named(dev, s->irq, SYSBUS_DEVICE_GPIO_IRQ,
> > >                                 s->xhci.numintrs);
> > > -    if (s->xhci.dma_mr) {
> > > -        s->xhci.as =  g_malloc0(sizeof(AddressSpace));
> > > -        address_space_init(s->xhci.as, s->xhci.dma_mr, NULL);
> > > -    } else {
> > > -        s->xhci.as = &address_space_memory;
> > > -    }
> > > -
> > > +    address_space_init(&s->xhci.as, s->xhci.dma_mr, "usb-xhci-dma");
> > >        sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->xhci.mem);
> > >    }
> >
> > My understanding of the patch is that you're trying to avoid the heap allocation
> > above (which is a good idea!) so from that perspective all you need is 
> > somewhere to
> > store the AddressSpace used for the the xhci-sysbus device, for which 
> > XHCISysbusState
> > would be the natural choice.
> >
> > It seems to me that the easiest approach is just to set the s->xhci.as pointer 
> > in
> > xhci_sysbus_realize() in exactly the same as usb_xhci_pci_realize() does:
> >
> > typedef struct XHCISysbusState {
> >       ...
> >       ...
> >       AddressSpace as;
> > } XHCISysbusState
> >
> > static void xhci_sysbus_realize(DeviceState *dev, Error **errp)
> > {
> >       XHCISysbusState *s = XHCI_SYSBUS(dev);
> >       ...
> >       ...
> >       address_space_init(&s->as, s->xhci.dma_mr ? s->xhci.dma_mr : 
> > get_system_memory(),
> >                          "usb-xhci-dma");
> >       s->xhci.as = &s->as;
> > }
> >
> > I think this approach is clearer since the xhci-sysbus device always creates 
> > its own
> > address space which is either an alias onto normal system memory or the custom
> > MemoryRegion provided via the "dma_mr" link property.
>
> I don't think we should continue to provide the back-compat fallback
> for "no link property set", but I agree that we should have
> have s->xhci.as = &s->as. This means that for the PCI case we can
> continue to set s->xhci.as = pci_address_space(), so the other patch
> that exposes the root MR of the PCI AS then becomes unneeded.


ATB,

Mark.