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

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

[Philippe Mathieu-Daudé]

Simplify the XHCI based devices 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.

Since v2:
- Use &error_abort (Eduardo)

Philippe Mathieu-Daudé (3):
  hw/pci: Introduce pci_dma_memory_region() helper
  hw/usb/hcd-xhci-pci: Abort if setting link property failed
  hw/usb/xhci: Always expect 'dma' link property to be set

 hw/usb/hcd-xhci.h        |  2 +-
 include/hw/pci/pci.h     |  5 +++++
 hw/i386/microvm.c        |  2 ++
 hw/usb/hcd-xhci-pci.c    |  5 +++--
 hw/usb/hcd-xhci-sysbus.c | 13 ++++++-------
 hw/usb/hcd-xhci.c        | 20 ++++++++++----------
 6 files changed, 27 insertions(+), 20 deletions(-)

-- 
2.31.1

[PATCH v3 1/3] hw/pci: Introduce pci_dma_memory_region() helper

[Philippe Mathieu-Daudé]

pci_get_address_space() returns an AddressSpace. In some
cases we want a MemoryRegion. Add the pci_dma_memory_region()
equivalent helper.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/pci/pci.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index d0f4266e372..5860f42e400 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -786,6 +786,11 @@ static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
     return &dev->bus_master_as;
 }
 
+static inline MemoryRegion *pci_dma_memory_region(PCIDevice *dev)
+{
+    return &dev->bus_master_container_region;
+}
+
 /**
  * pci_dma_rw: Read from or write to an address space from PCI device.
  *
-- 
2.31.1

Re: [PATCH v3 1/3] hw/pci: Introduce pci_dma_memory_region() helper

[Mark Cave-Ayland]

On 26/08/2021 21:07, Philippe Mathieu-Daudé wrote:

> pci_get_address_space() returns an AddressSpace. In some
> cases we want a MemoryRegion. Add the pci_dma_memory_region()
> equivalent helper.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   include/hw/pci/pci.h | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index d0f4266e372..5860f42e400 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -786,6 +786,11 @@ static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
>       return &dev->bus_master_as;
>   }
>   
> +static inline MemoryRegion *pci_dma_memory_region(PCIDevice *dev)
> +{
> +    return &dev->bus_master_container_region;
> +}
> +
>   /**
>    * pci_dma_rw: Read from or write to an address space from PCI device.
>    *

This patch doesn't quite feel right: there are already a number of devices that have 
been using pci_get_address_space() for some time without requiring access to the 
underlying MemoryRegion. So then the first question I ask myself when I see this 
patch is why do we suddenly need it now?


ATB,

Mark.

[PATCH v3 2/3] hw/usb/hcd-xhci-pci: Abort if setting link property failed

[Philippe Mathieu-Daudé]

Do not ignore eventual error if we failed at setting the 'host'
property of the TYPE_XHCI model.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/usb/hcd-xhci-pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
index e934b1a5b1f..24c528d210f 100644
--- a/hw/usb/hcd-xhci-pci.c
+++ b/hw/usb/hcd-xhci-pci.c
@@ -115,7 +115,7 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
     dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
     dev->config[0x60] = 0x30; /* release number */
 
-    object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL);
+    object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), &error_abort);
     s->xhci.intr_update = xhci_pci_intr_update;
     s->xhci.intr_raise = xhci_pci_intr_raise;
     if (!qdev_realize(DEVICE(&s->xhci), NULL, errp)) {
-- 
2.31.1

Re: [PATCH v3 2/3] hw/usb/hcd-xhci-pci: Abort if setting link property failed

[Mark Cave-Ayland]

On 26/08/2021 21:07, Philippe Mathieu-Daudé wrote:

> Do not ignore eventual error if we failed at setting the 'host'
> property of the TYPE_XHCI model.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   hw/usb/hcd-xhci-pci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
> index e934b1a5b1f..24c528d210f 100644
> --- a/hw/usb/hcd-xhci-pci.c
> +++ b/hw/usb/hcd-xhci-pci.c
> @@ -115,7 +115,7 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
>       dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
>       dev->config[0x60] = 0x30; /* release number */
>   
> -    object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL);
> +    object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), &error_abort);
>       s->xhci.intr_update = xhci_pci_intr_update;
>       s->xhci.intr_raise = xhci_pci_intr_raise;
>       if (!qdev_realize(DEVICE(&s->xhci), NULL, errp)) {

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

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

[Philippe Mathieu-Daudé]

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);
         sysbus_realize(SYS_BUS_DEVICE(dev), &error_fatal);
         sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, MICROVM_XHCI_BASE);
         sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
index 24c528d210f..10f5cc374fe 100644
--- a/hw/usb/hcd-xhci-pci.c
+++ b/hw/usb/hcd-xhci-pci.c
@@ -116,6 +116,8 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
     dev->config[0x60] = 0x30; /* release number */
 
     object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), &error_abort);
+    object_property_set_link(OBJECT(dev), "dma",
+                             OBJECT(pci_dma_memory_region(dev)), &error_abort);
     s->xhci.intr_update = xhci_pci_intr_update;
     s->xhci.intr_raise = xhci_pci_intr_raise;
     if (!qdev_realize(DEVICE(&s->xhci), NULL, errp)) {
@@ -161,7 +163,6 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
                   &s->xhci.mem, 0, OFF_MSIX_PBA,
                   0x90, NULL);
     }
-    s->xhci.as = pci_get_address_space(dev);
 }
 
 static void usb_xhci_pci_exit(PCIDevice *dev)
diff --git a/hw/usb/hcd-xhci-sysbus.c b/hw/usb/hcd-xhci-sysbus.c
index a14e4381960..f212ce785bd 100644
--- a/hw/usb/hcd-xhci-sysbus.c
+++ b/hw/usb/hcd-xhci-sysbus.c
@@ -36,6 +36,11 @@ static void xhci_sysbus_realize(DeviceState *dev, Error **errp)
 {
     XHCISysbusState *s = XHCI_SYSBUS(dev);
 
+    if (!s->xhci.dma_mr) {
+        error_setg(errp, TYPE_XHCI_SYSBUS " 'dma' link not set");
+        return;
+    }
+
     object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL);
     if (!qdev_realize(DEVICE(&s->xhci), NULL, errp)) {
         return;
@@ -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);
 }
 
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index e01700039b1..011f1233ef3 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -487,7 +487,7 @@ static inline void xhci_dma_read_u32s(XHCIState *xhci, dma_addr_t addr,
 
     assert((len % sizeof(uint32_t)) == 0);
 
-    dma_memory_read(xhci->as, addr, buf, len);
+    dma_memory_read(&xhci->as, addr, buf, len);
 
     for (i = 0; i < (len / sizeof(uint32_t)); i++) {
         buf[i] = le32_to_cpu(buf[i]);
@@ -507,7 +507,7 @@ static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr,
     for (i = 0; i < n; i++) {
         tmp[i] = cpu_to_le32(buf[i]);
     }
-    dma_memory_write(xhci->as, addr, tmp, len);
+    dma_memory_write(&xhci->as, addr, tmp, len);
 }
 
 static XHCIPort *xhci_lookup_port(XHCIState *xhci, struct USBPort *uport)
@@ -618,7 +618,7 @@ static void xhci_write_event(XHCIState *xhci, XHCIEvent *event, int v)
                                ev_trb.status, ev_trb.control);
 
     addr = intr->er_start + TRB_SIZE*intr->er_ep_idx;
-    dma_memory_write(xhci->as, addr, &ev_trb, TRB_SIZE);
+    dma_memory_write(&xhci->as, addr, &ev_trb, TRB_SIZE);
 
     intr->er_ep_idx++;
     if (intr->er_ep_idx >= intr->er_size) {
@@ -679,7 +679,7 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
 
     while (1) {
         TRBType type;
-        dma_memory_read(xhci->as, ring->dequeue, trb, TRB_SIZE);
+        dma_memory_read(&xhci->as, ring->dequeue, trb, TRB_SIZE);
         trb->addr = ring->dequeue;
         trb->ccs = ring->ccs;
         le64_to_cpus(&trb->parameter);
@@ -726,7 +726,7 @@ static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
 
     while (1) {
         TRBType type;
-        dma_memory_read(xhci->as, dequeue, &trb, TRB_SIZE);
+        dma_memory_read(&xhci->as, dequeue, &trb, TRB_SIZE);
         le64_to_cpus(&trb.parameter);
         le32_to_cpus(&trb.status);
         le32_to_cpus(&trb.control);
@@ -781,7 +781,7 @@ static void xhci_er_reset(XHCIState *xhci, int v)
         xhci_die(xhci);
         return;
     }
-    dma_memory_read(xhci->as, erstba, &seg, sizeof(seg));
+    dma_memory_read(&xhci->as, erstba, &seg, sizeof(seg));
     le32_to_cpus(&seg.addr_low);
     le32_to_cpus(&seg.addr_high);
     le32_to_cpus(&seg.size);
@@ -1393,7 +1393,7 @@ static int xhci_xfer_create_sgl(XHCITransfer *xfer, int in_xfer)
     int i;
 
     xfer->int_req = false;
-    qemu_sglist_init(&xfer->sgl, DEVICE(xhci), xfer->trb_count, xhci->as);
+    qemu_sglist_init(&xfer->sgl, DEVICE(xhci), xfer->trb_count, &xhci->as);
     for (i = 0; i < xfer->trb_count; i++) {
         XHCITRB *trb = &xfer->trbs[i];
         dma_addr_t addr;
@@ -2059,7 +2059,7 @@ static TRBCCode xhci_address_slot(XHCIState *xhci, unsigned int slotid,
     assert(slotid >= 1 && slotid <= xhci->numslots);
 
     dcbaap = xhci_addr64(xhci->dcbaap_low, xhci->dcbaap_high);
-    poctx = ldq_le_dma(xhci->as, dcbaap + 8 * slotid);
+    poctx = ldq_le_dma(&xhci->as, dcbaap + 8 * slotid);
     ictx = xhci_mask64(pictx);
     octx = xhci_mask64(poctx);
 
@@ -2397,7 +2397,7 @@ static TRBCCode xhci_get_port_bandwidth(XHCIState *xhci, uint64_t pctx)
     /* TODO: actually implement real values here */
     bw_ctx[0] = 0;
     memset(&bw_ctx[1], 80, xhci->numports); /* 80% */
-    dma_memory_write(xhci->as, ctx, bw_ctx, sizeof(bw_ctx));
+    dma_memory_write(&xhci->as, ctx, bw_ctx, sizeof(bw_ctx));
 
     return CC_SUCCESS;
 }
@@ -3434,7 +3434,7 @@ static int usb_xhci_post_load(void *opaque, int version_id)
             continue;
         }
         slot->ctx =
-            xhci_mask64(ldq_le_dma(xhci->as, dcbaap + 8 * slotid));
+            xhci_mask64(ldq_le_dma(&xhci->as, dcbaap + 8 * slotid));
         xhci_dma_read_u32s(xhci, slot->ctx, slot_ctx, sizeof(slot_ctx));
         slot->uport = xhci_lookup_uport(xhci, slot_ctx);
         if (!slot->uport) {
-- 
2.31.1

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

[Mark Cave-Ayland]

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?

>           sysbus_realize(SYS_BUS_DEVICE(dev), &error_fatal);
>           sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, MICROVM_XHCI_BASE);
>           sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
> diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
> index 24c528d210f..10f5cc374fe 100644
> --- a/hw/usb/hcd-xhci-pci.c
> +++ b/hw/usb/hcd-xhci-pci.c
> @@ -116,6 +116,8 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
>       dev->config[0x60] = 0x30; /* release number */
>   
>       object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), &error_abort);
> +    object_property_set_link(OBJECT(dev), "dma",
> +                             OBJECT(pci_dma_memory_region(dev)), &error_abort);
>       s->xhci.intr_update = xhci_pci_intr_update;
>       s->xhci.intr_raise = xhci_pci_intr_raise;
>       if (!qdev_realize(DEVICE(&s->xhci), NULL, errp)) {
> @@ -161,7 +163,6 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
>                     &s->xhci.mem, 0, OFF_MSIX_PBA,
>                     0x90, NULL);
>       }
> -    s->xhci.as = pci_get_address_space(dev);
>   }
>   
>   static void usb_xhci_pci_exit(PCIDevice *dev)
> diff --git a/hw/usb/hcd-xhci-sysbus.c b/hw/usb/hcd-xhci-sysbus.c
> index a14e4381960..f212ce785bd 100644
> --- a/hw/usb/hcd-xhci-sysbus.c
> +++ b/hw/usb/hcd-xhci-sysbus.c
> @@ -36,6 +36,11 @@ static void xhci_sysbus_realize(DeviceState *dev, Error **errp)
>   {
>       XHCISysbusState *s = XHCI_SYSBUS(dev);
>   
> +    if (!s->xhci.dma_mr) {
> +        error_setg(errp, TYPE_XHCI_SYSBUS " 'dma' link not set");
> +        return;
> +    }
> +
>       object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL);
>       if (!qdev_realize(DEVICE(&s->xhci), NULL, errp)) {
>           return;
> @@ -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. Note that 
xhci_sysbus_finalize() should also not forget to remove the AddressSpace via 
address_space_destroy() to prevent a leak there too.

> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index e01700039b1..011f1233ef3 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -487,7 +487,7 @@ static inline void xhci_dma_read_u32s(XHCIState *xhci, dma_addr_t addr,
>   
>       assert((len % sizeof(uint32_t)) == 0);
>   
> -    dma_memory_read(xhci->as, addr, buf, len);
> +    dma_memory_read(&xhci->as, addr, buf, len);
>   
>       for (i = 0; i < (len / sizeof(uint32_t)); i++) {
>           buf[i] = le32_to_cpu(buf[i]);
> @@ -507,7 +507,7 @@ static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr,
>       for (i = 0; i < n; i++) {
>           tmp[i] = cpu_to_le32(buf[i]);
>       }
> -    dma_memory_write(xhci->as, addr, tmp, len);
> +    dma_memory_write(&xhci->as, addr, tmp, len);
>   }
>   
>   static XHCIPort *xhci_lookup_port(XHCIState *xhci, struct USBPort *uport)
> @@ -618,7 +618,7 @@ static void xhci_write_event(XHCIState *xhci, XHCIEvent *event, int v)
>                                  ev_trb.status, ev_trb.control);
>   
>       addr = intr->er_start + TRB_SIZE*intr->er_ep_idx;
> -    dma_memory_write(xhci->as, addr, &ev_trb, TRB_SIZE);
> +    dma_memory_write(&xhci->as, addr, &ev_trb, TRB_SIZE);
>   
>       intr->er_ep_idx++;
>       if (intr->er_ep_idx >= intr->er_size) {
> @@ -679,7 +679,7 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
>   
>       while (1) {
>           TRBType type;
> -        dma_memory_read(xhci->as, ring->dequeue, trb, TRB_SIZE);
> +        dma_memory_read(&xhci->as, ring->dequeue, trb, TRB_SIZE);
>           trb->addr = ring->dequeue;
>           trb->ccs = ring->ccs;
>           le64_to_cpus(&trb->parameter);
> @@ -726,7 +726,7 @@ static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
>   
>       while (1) {
>           TRBType type;
> -        dma_memory_read(xhci->as, dequeue, &trb, TRB_SIZE);
> +        dma_memory_read(&xhci->as, dequeue, &trb, TRB_SIZE);
>           le64_to_cpus(&trb.parameter);
>           le32_to_cpus(&trb.status);
>           le32_to_cpus(&trb.control);
> @@ -781,7 +781,7 @@ static void xhci_er_reset(XHCIState *xhci, int v)
>           xhci_die(xhci);
>           return;
>       }
> -    dma_memory_read(xhci->as, erstba, &seg, sizeof(seg));
> +    dma_memory_read(&xhci->as, erstba, &seg, sizeof(seg));
>       le32_to_cpus(&seg.addr_low);
>       le32_to_cpus(&seg.addr_high);
>       le32_to_cpus(&seg.size);
> @@ -1393,7 +1393,7 @@ static int xhci_xfer_create_sgl(XHCITransfer *xfer, int in_xfer)
>       int i;
>   
>       xfer->int_req = false;
> -    qemu_sglist_init(&xfer->sgl, DEVICE(xhci), xfer->trb_count, xhci->as);
> +    qemu_sglist_init(&xfer->sgl, DEVICE(xhci), xfer->trb_count, &xhci->as);
>       for (i = 0; i < xfer->trb_count; i++) {
>           XHCITRB *trb = &xfer->trbs[i];
>           dma_addr_t addr;
> @@ -2059,7 +2059,7 @@ static TRBCCode xhci_address_slot(XHCIState *xhci, unsigned int slotid,
>       assert(slotid >= 1 && slotid <= xhci->numslots);
>   
>       dcbaap = xhci_addr64(xhci->dcbaap_low, xhci->dcbaap_high);
> -    poctx = ldq_le_dma(xhci->as, dcbaap + 8 * slotid);
> +    poctx = ldq_le_dma(&xhci->as, dcbaap + 8 * slotid);
>       ictx = xhci_mask64(pictx);
>       octx = xhci_mask64(poctx);
>   
> @@ -2397,7 +2397,7 @@ static TRBCCode xhci_get_port_bandwidth(XHCIState *xhci, uint64_t pctx)
>       /* TODO: actually implement real values here */
>       bw_ctx[0] = 0;
>       memset(&bw_ctx[1], 80, xhci->numports); /* 80% */
> -    dma_memory_write(xhci->as, ctx, bw_ctx, sizeof(bw_ctx));
> +    dma_memory_write(&xhci->as, ctx, bw_ctx, sizeof(bw_ctx));
>   
>       return CC_SUCCESS;
>   }
> @@ -3434,7 +3434,7 @@ static int usb_xhci_post_load(void *opaque, int version_id)
>               continue;
>           }
>           slot->ctx =
> -            xhci_mask64(ldq_le_dma(xhci->as, dcbaap + 8 * slotid));
> +            xhci_mask64(ldq_le_dma(&xhci->as, dcbaap + 8 * slotid));
>           xhci_dma_read_u32s(xhci, slot->ctx, slot_ctx, sizeof(slot_ctx));
>           slot->uport = xhci_lookup_uport(xhci, slot_ctx);
>           if (!slot->uport) {


ATB,

Mark.

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

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.

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

[Peter Maydell]

On Fri, 27 Aug 2021 at 10:14, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> 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.

No, I don't think we have a convention; it might be nice to add one.
Currently a quick git grep for DEFINE_PROP_LINK and eyeballing of
the results shows that we have:

 "memory" x 7
 "dram" x 4
 "downstream" x 3
 "dma-memory" x 3
 "dma" x 2
 "source-memory"
 "dram-mr"
 "ddr"
 "ddr-ram"
 "gic"
 "cpc"
 "port[N]"
 "dma_mr"
 "ahb-bus"
 "system-memory"
 "main-bus"

This list includes all our TYPE_MEMORY_REGION link properties; a few of these
are special-purpose, and reasonably have specialized names. 2 out of 3 users
of "downstream" are devices which pass on (or filter out) memory transactions
from the CPU (tz-msc, tz-mpc), and I think that name makes sense there.
(The 3rd is pl080.c, which is a plain old DMA controller, and the naming
there is not so well-suited.)

"memory" is mostly SoC and CPU objects taking a link to whatever they should
have as the CPU's view of memory.

I don't have a strong view on what we ought to try to standardize on,
except that I don't like the "_mr" or "-mr" suffix -- I don't think we
need to try to encode the type of the link property in the property name.

It is probably reasonable to have different naming conventions for:
 * SoC and CPU objects, which take a link to the MR which represents
   the CPU/SoC's view of the outside world
 * Endpoint devices which can be DMA masters and take a link giving
   them their view of what they can DMA to
 * filtering/control devices which take incoming transactions from
   an upstream port, filter some and pass the rest through to a
   downstream port

In pretty much all cases, these link properties are used only internally
to QEMU, so if we decide on a naming convention we can fairly easily
rename existing properties to match.

-- PMM

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

[Mark Cave-Ayland]

On 27/08/2021 11:14, Peter Maydell wrote:

> On Fri, 27 Aug 2021 at 10:14, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>> 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.
> 
> No, I don't think we have a convention; it might be nice to add one.
> Currently a quick git grep for DEFINE_PROP_LINK and eyeballing of
> the results shows that we have:
> 
>   "memory" x 7
>   "dram" x 4
>   "downstream" x 3
>   "dma-memory" x 3
>   "dma" x 2
>   "source-memory"
>   "dram-mr"
>   "ddr"
>   "ddr-ram"
>   "gic"
>   "cpc"
>   "port[N]"
>   "dma_mr"
>   "ahb-bus"
>   "system-memory"
>   "main-bus"
> 
> This list includes all our TYPE_MEMORY_REGION link properties; a few of these
> are special-purpose, and reasonably have specialized names. 2 out of 3 users
> of "downstream" are devices which pass on (or filter out) memory transactions
> from the CPU (tz-msc, tz-mpc), and I think that name makes sense there.
> (The 3rd is pl080.c, which is a plain old DMA controller, and the naming
> there is not so well-suited.)
> 
> "memory" is mostly SoC and CPU objects taking a link to whatever they should
> have as the CPU's view of memory.
> 
> I don't have a strong view on what we ought to try to standardize on,
> except that I don't like the "_mr" or "-mr" suffix -- I don't think we
> need to try to encode the type of the link property in the property name.
> 
> It is probably reasonable to have different naming conventions for:
>   * SoC and CPU objects, which take a link to the MR which represents
>     the CPU/SoC's view of the outside world
>   * Endpoint devices which can be DMA masters and take a link giving
>     them their view of what they can DMA to
>   * filtering/control devices which take incoming transactions from
>     an upstream port, filter some and pass the rest through to a
>     downstream port
> 
> In pretty much all cases, these link properties are used only internally
> to QEMU, so if we decide on a naming convention we can fairly easily
> rename existing properties to match.

I quite like "cpu-memory" for SoC/CPU objects and "dma-memory" for endpoint devices 
that can be DMA masters. Perhaps the last case is specialised enough that a 
convention there doesn't make sense?


ATB,

Mark.

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

[Philippe Mathieu-Daudé]

On 8/27/21 1:03 PM, Mark Cave-Ayland wrote:
> On 27/08/2021 11:14, Peter Maydell wrote:
> 
>> On Fri, 27 Aug 2021 at 10:14, Mark Cave-Ayland
>> <mark.cave-ayland@ilande.co.uk> wrote:
>>> 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.
>>
>> No, I don't think we have a convention; it might be nice to add one.
>> Currently a quick git grep for DEFINE_PROP_LINK and eyeballing of
>> the results shows that we have:
>>
>>   "memory" x 7
>>   "dram" x 4
>>   "downstream" x 3
>>   "dma-memory" x 3
>>   "dma" x 2
>>   "source-memory"
>>   "dram-mr"
>>   "ddr"
>>   "ddr-ram"
>>   "gic"
>>   "cpc"
>>   "port[N]"
>>   "dma_mr"
>>   "ahb-bus"
>>   "system-memory"
>>   "main-bus"
>>
>> This list includes all our TYPE_MEMORY_REGION link properties; a few
>> of these
>> are special-purpose, and reasonably have specialized names. 2 out of 3
>> users
>> of "downstream" are devices which pass on (or filter out) memory
>> transactions
>> from the CPU (tz-msc, tz-mpc), and I think that name makes sense there.
>> (The 3rd is pl080.c, which is a plain old DMA controller, and the naming
>> there is not so well-suited.)
>>
>> "memory" is mostly SoC and CPU objects taking a link to whatever they
>> should
>> have as the CPU's view of memory.
>>
>> I don't have a strong view on what we ought to try to standardize on,
>> except that I don't like the "_mr" or "-mr" suffix -- I don't think we
>> need to try to encode the type of the link property in the property name.
>>
>> It is probably reasonable to have different naming conventions for:
>>   * SoC and CPU objects, which take a link to the MR which represents
>>     the CPU/SoC's view of the outside world
>>   * Endpoint devices which can be DMA masters and take a link giving
>>     them their view of what they can DMA to
>>   * filtering/control devices which take incoming transactions from
>>     an upstream port, filter some and pass the rest through to a
>>     downstream port

Which category fits IOMMUs?

>>
>> In pretty much all cases, these link properties are used only internally
>> to QEMU, so if we decide on a naming convention we can fairly easily
>> rename existing properties to match.
> 
> I quite like "cpu-memory" for SoC/CPU objects and "dma-memory" for
> endpoint devices that can be DMA masters. Perhaps the last case is
> specialised enough that a convention there doesn't make sense?
So "iommu-memory"?