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

[Peter Maydell]

On Thu, 26 Aug 2021 at 22:15, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 26/08/2021 21:07, Philippe Mathieu-Daudé wrote:
>
> > Simplify by always passing a MemoryRegion property to the device.
> > Doing so we can move the AddressSpace field to the device struct,
> > removing need for heap allocation.
> >
> > Update the MicroVM machine to pass the default system memory instead
> > of a NULL value.
> >
> > We don't need to change the Versal machine, as the link property is
> > initialize as "versal.dwc3_alias" MemoryRegion alias.
> >
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> > Versal untested
> > ---
> >   hw/usb/hcd-xhci.h        |  2 +-
> >   hw/i386/microvm.c        |  2 ++
> >   hw/usb/hcd-xhci-pci.c    |  3 ++-
> >   hw/usb/hcd-xhci-sysbus.c | 13 ++++++-------
> >   hw/usb/hcd-xhci.c        | 20 ++++++++++----------
> >   5 files changed, 21 insertions(+), 19 deletions(-)
> >
> > diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
> > index 98f598382ad..ea76ec4f277 100644
> > --- a/hw/usb/hcd-xhci.h
> > +++ b/hw/usb/hcd-xhci.h
> > @@ -180,7 +180,7 @@ typedef struct XHCIState {
> >       USBBus bus;
> >       MemoryRegion mem;
> >       MemoryRegion *dma_mr;
> > -    AddressSpace *as;
> > +    AddressSpace as;
> >       MemoryRegion mem_cap;
> >       MemoryRegion mem_oper;
> >       MemoryRegion mem_runtime;
> > diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> > index aba0c832190..2d55114a676 100644
> > --- a/hw/i386/microvm.c
> > +++ b/hw/i386/microvm.c
> > @@ -219,6 +219,8 @@ static void microvm_devices_init(MicrovmMachineState 
> > *mms)
> >           qdev_prop_set_uint32(dev, "slots", XHCI_MAXSLOTS);
> >           qdev_prop_set_uint32(dev, "p2", 8);
> >           qdev_prop_set_uint32(dev, "p3", 8);
> > +        object_property_set_link(OBJECT(dev), "dma",
> > +                                 OBJECT(get_system_memory()), 
> > &error_abort);
>
> 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.

> > @@ -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.

-- PMM