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

[PATCH v2 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.

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 v2 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

[PATCH v2 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..71f6629ccde 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_fatal);
     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 v2 2/3] hw/usb/hcd-xhci-pci: Abort if setting link property failed

[Eduardo Habkost]

+Markus

On Thu, Aug 19, 2021 at 07:15:46PM +0200, 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..71f6629ccde 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_fatal);

If this fails, it's due to programmer error, isn't?  Shouldn't we
use &error_abort on that case?

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

-- 
Eduardo

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> +Markus
>
> On Thu, Aug 19, 2021 at 07:15:46PM +0200, 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..71f6629ccde 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_fatal);
>
> If this fails, it's due to programmer error, isn't?  Shouldn't we
> use &error_abort on that case?

I think so.

In functions with an Error **errp parameter, use of &error_fatal is
almost always wrong.

>>      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 v2 2/3] hw/usb/hcd-xhci-pci: Abort if setting link property failed

[Philippe Mathieu-Daudé]

On 8/24/21 10:13 AM, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
>> +Markus
>>
>> On Thu, Aug 19, 2021 at 07:15:46PM +0200, 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..71f6629ccde 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_fatal);
>>
>> If this fails, it's due to programmer error, isn't?  Shouldn't we
>> use &error_abort on that case?
> 
> I think so.
> 
> In functions with an Error **errp parameter, use of &error_fatal is
> almost always wrong.

OK, thanks!

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

[Philippe Mathieu-Daudé]

On 8/24/21 10:35 AM, Philippe Mathieu-Daudé wrote:
> On 8/24/21 10:13 AM, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>
>>> +Markus
>>>
>>> On Thu, Aug 19, 2021 at 07:15:46PM +0200, 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..71f6629ccde 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_fatal);
>>>
>>> If this fails, it's due to programmer error, isn't?  Shouldn't we
>>> use &error_abort on that case?
>>
>> I think so.
>>
>> In functions with an Error **errp parameter, use of &error_fatal is
>> almost always wrong.

Having used 'abort' in the subject, no clue why I used &error_fatal
then...

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

[Peter Maydell]

On Tue, 24 Aug 2021 at 09:14, Markus Armbruster <armbru@redhat.com> wrote:
> In functions with an Error **errp parameter, use of &error_fatal is
> almost always wrong.

What are the cases where it is not wrong? My guess is "in board
code and other places where the error handling would have been
'print a message and call exit()' anyway".

-- PMM

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

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 24 Aug 2021 at 09:14, Markus Armbruster <armbru@redhat.com> wrote:
>> In functions with an Error **errp parameter, use of &error_fatal is
>> almost always wrong.
>
> What are the cases where it is not wrong?

I can't think of a use that isn't wrong.  Doesn't mean no such use could
exist.  Most rules have exceptions...

>                                           My guess is "in board
> code and other places where the error handling would have been
> 'print a message and call exit()' anyway".

When you know that all callers handle errors like &error_fatal does, use
of &error_fatal doesn't produce wrong behavior.  It's still kind of
wrong, because relying on such a non-local argument without a genuine
need is.

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

[Peter Maydell]

On Tue, 24 Aug 2021 at 13:05, Markus Armbruster <armbru@redhat.com> wrote:
> When you know that all callers handle errors like &error_fatal does, use
> of &error_fatal doesn't produce wrong behavior.  It's still kind of
> wrong, because relying on such a non-local argument without a genuine
> need is.

Not using error_fatal results in quite a bit of extra boilerplate
for all those extra explicit "check for failure, print the error
message and exit" points. If the MachineState init function took
an Error** that might be a mild encouragement to "pass an Error
upward rather than exiting"; but it doesn't.

Right now we have nearly a thousand instances of error_fatal
in the codebase, incidentally.

-- PMM

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

[Eduardo Habkost]

On Tue, Aug 24, 2021 at 01:16:40PM +0100, Peter Maydell wrote:
> On Tue, 24 Aug 2021 at 13:05, Markus Armbruster <armbru@redhat.com> wrote:
> > When you know that all callers handle errors like &error_fatal does, use
> > of &error_fatal doesn't produce wrong behavior.  It's still kind of
> > wrong, because relying on such a non-local argument without a genuine
> > need is.
> 
> Not using error_fatal results in quite a bit of extra boilerplate
> for all those extra explicit "check for failure, print the error
> message and exit" points.

I don't get it.  There's no need for extra boilerplate if the
caller is using &error_fatal.

> If the MachineState init function took
> an Error** that might be a mild encouragement to "pass an Error
> upward rather than exiting"; but it doesn't.

Agreed.

> 
> Right now we have nearly a thousand instances of error_fatal
> in the codebase, incidentally.

It looks like 73 of them are in functions that take an Error**
argument.

Coccinelle patch for finding them:

@@
typedef Error;
type T;
identifier errp, fn;
@@
 T fn(..., Error **errp)
 {
 ...
*&error_fatal
 ...
 }


Coccinelle output:

  diff -u -p ./hw/pci-host/pnv_phb3.c /tmp/nothing/hw/pci-host/pnv_phb3.c
  --- ./hw/pci-host/pnv_phb3.c
  +++ /tmp/nothing/hw/pci-host/pnv_phb3.c
  @@ -1054,7 +1054,6 @@ static void pnv_phb3_realize(DeviceState
       /* Add a single Root port */
       qdev_prop_set_uint8(DEVICE(&phb->root), "chassis", phb->chip_id);
       qdev_prop_set_uint16(DEVICE(&phb->root), "slot", phb->phb_id);
  -    qdev_realize(DEVICE(&phb->root), BUS(pci->bus), &error_fatal);
   }
   
   void pnv_phb3_update_regions(PnvPHB3 *phb)
  diff -u -p ./hw/pci-host/q35.c /tmp/nothing/hw/pci-host/q35.c
  --- ./hw/pci-host/q35.c
  +++ /tmp/nothing/hw/pci-host/q35.c
  @@ -67,7 +67,6 @@ static void q35_host_realize(DeviceState
       PC_MACHINE(qdev_get_machine())->bus = pci->bus;
       pci->bypass_iommu =
           PC_MACHINE(qdev_get_machine())->default_bus_bypass_iommu;
  -    qdev_realize(DEVICE(&s->mch), BUS(pci->bus), &error_fatal);
   }
   
   static const char *q35_host_root_bus_path(PCIHostState *host_bridge,
  diff -u -p ./hw/pci-host/mv64361.c /tmp/nothing/hw/pci-host/mv64361.c
  --- ./hw/pci-host/mv64361.c
  +++ /tmp/nothing/hw/pci-host/mv64361.c
  @@ -875,7 +875,6 @@ static void mv64361_realize(DeviceState
                                   TYPE_MV64361_PCI);
           DeviceState *pci = DEVICE(&s->pci[i]);
           qdev_prop_set_uint8(pci, "index", i);
  -        sysbus_realize_and_unref(SYS_BUS_DEVICE(pci), &error_fatal);
       }
       sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->cpu_irq);
       qdev_init_gpio_in_named(dev, mv64361_gpp_irq, "gpp", 32);
  diff -u -p ./hw/pci-host/designware.c /tmp/nothing/hw/pci-host/designware.c
  --- ./hw/pci-host/designware.c
  +++ /tmp/nothing/hw/pci-host/designware.c
  @@ -707,7 +707,6 @@ static void designware_pcie_host_realize
                          "pcie-bus-address-space");
       pci_setup_iommu(pci->bus, designware_pcie_host_set_iommu, s);
   
  -    qdev_realize(DEVICE(&s->root), BUS(pci->bus), &error_fatal);
   }
   
   static const VMStateDescription vmstate_designware_pcie_host = {
  diff -u -p ./hw/pci-host/raven.c /tmp/nothing/hw/pci-host/raven.c
  --- ./hw/pci-host/raven.c
  +++ /tmp/nothing/hw/pci-host/raven.c
  @@ -335,7 +335,6 @@ static void raven_realize(PCIDevice *d,
       d->config[0x34] = 0x00; // capabilities_pointer
   
       memory_region_init_rom_nomigrate(&s->bios, OBJECT(s), "bios", BIOS_SIZE,
  -                                     &error_fatal);
       memory_region_add_subregion(get_system_memory(), (uint32_t)(-BIOS_SIZE),
                                   &s->bios);
       if (s->bios_name) {
  diff -u -p ./hw/pci-host/gpex.c /tmp/nothing/hw/pci-host/gpex.c
  --- ./hw/pci-host/gpex.c
  +++ /tmp/nothing/hw/pci-host/gpex.c
  @@ -138,7 +138,6 @@ static void gpex_host_realize(DeviceStat
                                        &s->io_ioport, 0, 4, TYPE_PCIE_BUS);
   
       pci_bus_set_route_irq_fn(pci->bus, gpex_route_intx_pin_to_irq);
  -    qdev_realize(DEVICE(&s->gpex_root), BUS(pci->bus), &error_fatal);
   }
   
   static const char *gpex_host_root_bus_path(PCIHostState *host_bridge,
  diff -u -p ./hw/pci-host/xilinx-pcie.c /tmp/nothing/hw/pci-host/xilinx-pcie.c
  --- ./hw/pci-host/xilinx-pcie.c
  +++ /tmp/nothing/hw/pci-host/xilinx-pcie.c
  @@ -137,7 +137,6 @@ static void xilinx_pcie_host_realize(Dev
                                        pci_swizzle_map_irq_fn, s, &s->mmio,
                                        &s->io, 0, 4, TYPE_PCIE_BUS);
   
  -    qdev_realize(DEVICE(&s->root), BUS(pci->bus), &error_fatal);
   }
   
   static const char *xilinx_pcie_host_root_bus_path(PCIHostState *host_bridge,
  diff -u -p ./hw/ppc/spapr_irq.c /tmp/nothing/hw/ppc/spapr_irq.c
  --- ./hw/ppc/spapr_irq.c
  +++ /tmp/nothing/hw/ppc/spapr_irq.c
  @@ -337,7 +337,6 @@ void spapr_irq_init(SpaprMachineState *s
           qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3);
           object_property_set_link(OBJECT(dev), "xive-fabric", OBJECT(spapr),
                                    &error_abort);
  -        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
   
           spapr->xive = SPAPR_XIVE(dev);
   
  diff -u -p ./hw/ppc/pnv.c /tmp/nothing/hw/ppc/pnv.c
  --- ./hw/ppc/pnv.c
  +++ /tmp/nothing/hw/ppc/pnv.c
  @@ -1144,7 +1144,6 @@ static void pnv_chip_power8_realize(Devi
   
       /* Processor Service Interface (PSI) Host Bridge */
       object_property_set_int(OBJECT(&chip8->psi), "bar", PNV_PSIHB_BASE(chip),
  -                            &error_fatal);
       object_property_set_link(OBJECT(&chip8->psi), ICS_PROP_XICS,
                                OBJECT(chip8->xics), &error_abort);
       if (!qdev_realize(DEVICE(&chip8->psi), NULL, errp)) {
  @@ -1587,7 +1586,6 @@ static void pnv_chip_power10_realize(Dev
   
       /* Processor Service Interface (PSI) Host Bridge */
       object_property_set_int(OBJECT(&chip10->psi), "bar",
  -                            PNV10_PSIHB_BASE(chip), &error_fatal);
       if (!qdev_realize(DEVICE(&chip10->psi), NULL, errp)) {
           return;
       }
  diff -u -p ./hw/intc/exynos4210_gic.c /tmp/nothing/hw/intc/exynos4210_gic.c
  --- ./hw/intc/exynos4210_gic.c
  +++ /tmp/nothing/hw/intc/exynos4210_gic.c
  @@ -301,7 +301,6 @@ static void exynos4210_gic_realize(Devic
       qdev_prop_set_uint32(s->gic, "num-cpu", s->num_cpu);
       qdev_prop_set_uint32(s->gic, "num-irq", EXYNOS4210_GIC_NIRQ);
       gicbusdev = SYS_BUS_DEVICE(s->gic);
  -    sysbus_realize_and_unref(gicbusdev, &error_fatal);
   
       /* Pass through outbound IRQ lines from the GIC */
       sysbus_pass_irq(sbd, gicbusdev);
  diff -u -p ./hw/intc/spapr_xive.c /tmp/nothing/hw/intc/spapr_xive.c
  --- ./hw/intc/spapr_xive.c
  +++ /tmp/nothing/hw/intc/spapr_xive.c
  @@ -311,7 +311,6 @@ static void spapr_xive_realize(DeviceSta
        * Initialize the internal sources, for IPIs and virtual devices.
        */
       object_property_set_int(OBJECT(xsrc), "nr-irqs", xive->nr_irqs,
  -                            &error_fatal);
       object_property_set_link(OBJECT(xsrc), "xive", OBJECT(xive), &error_abort);
       if (!qdev_realize(DEVICE(xsrc), NULL, errp)) {
           return;
  diff -u -p ./hw/intc/pnv_xive.c /tmp/nothing/hw/intc/pnv_xive.c
  --- ./hw/intc/pnv_xive.c
  +++ /tmp/nothing/hw/intc/pnv_xive.c
  @@ -1833,7 +1833,6 @@ static void pnv_xive_realize(DeviceState
        * to limit accesses to resources not provisioned.
        */
       object_property_set_int(OBJECT(xsrc), "nr-irqs", PNV_XIVE_NR_IRQS,
  -                            &error_fatal);
       object_property_set_link(OBJECT(xsrc), "xive", OBJECT(xive), &error_abort);
       if (!qdev_realize(DEVICE(xsrc), NULL, errp)) {
           return;
  diff -u -p ./hw/riscv/shakti_c.c /tmp/nothing/hw/riscv/shakti_c.c
  --- ./hw/riscv/shakti_c.c
  +++ /tmp/nothing/hw/riscv/shakti_c.c
  @@ -137,7 +137,6 @@ static void shakti_c_soc_state_realize(D
   
       /* ROM */
       memory_region_init_rom(&sss->rom, OBJECT(dev), "riscv.shakti.c.rom",
  -                           shakti_c_memmap[SHAKTI_C_ROM].size, &error_fatal);
       memory_region_add_subregion(system_memory,
           shakti_c_memmap[SHAKTI_C_ROM].base, &sss->rom);
   }
  diff -u -p ./hw/riscv/sifive_e.c /tmp/nothing/hw/riscv/sifive_e.c
  --- ./hw/riscv/sifive_e.c
  +++ /tmp/nothing/hw/riscv/sifive_e.c
  @@ -192,7 +192,6 @@ static void sifive_e_soc_realize(DeviceS
   
       /* Mask ROM */
       memory_region_init_rom(&s->mask_rom, OBJECT(dev), "riscv.sifive.e.mrom",
  -                           memmap[SIFIVE_E_DEV_MROM].size, &error_fatal);
       memory_region_add_subregion(sys_mem,
           memmap[SIFIVE_E_DEV_MROM].base, &s->mask_rom);
   
  diff -u -p ./hw/display/bochs-display.c /tmp/nothing/hw/display/bochs-display.c
  --- ./hw/display/bochs-display.c
  +++ /tmp/nothing/hw/display/bochs-display.c
  @@ -280,7 +280,6 @@ static void bochs_display_realize(PCIDev
       s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s);
   
       memory_region_init_ram(&s->vram, obj, "bochs-display-vram", s->vgamem,
  -                           &error_fatal);
       memory_region_init_io(&s->vbe, obj, &bochs_display_vbe_ops, s,
                             "bochs dispi interface", PCI_VGA_BOCHS_SIZE);
       memory_region_init_io(&s->qext, obj, &bochs_display_qext_ops, s,
  diff -u -p ./hw/display/tcx.c /tmp/nothing/hw/display/tcx.c
  --- ./hw/display/tcx.c
  +++ /tmp/nothing/hw/display/tcx.c
  @@ -818,7 +818,6 @@ static void tcx_realizefn(DeviceState *d
       char *fcode_filename;
   
       memory_region_init_ram_nomigrate(&s->vram_mem, OBJECT(s), "tcx.vram",
  -                           s->vram_size * (1 + 4 + 4), &error_fatal);
       vmstate_register_ram_global(&s->vram_mem);
       memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA);
       vram_base = memory_region_get_ram_ptr(&s->vram_mem);
  diff -u -p ./hw/display/next-fb.c /tmp/nothing/hw/display/next-fb.c
  --- ./hw/display/next-fb.c
  +++ /tmp/nothing/hw/display/next-fb.c
  @@ -108,7 +108,6 @@ static void nextfb_realize(DeviceState *
       NeXTFbState *s = NEXTFB(dev);
   
       memory_region_init_ram(&s->fb_mr, OBJECT(dev), "next-video", 0x1CB100,
  -                           &error_fatal);
       sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->fb_mr);
   
       s->invalidate = 1;
  diff -u -p ./hw/display/qxl.c /tmp/nothing/hw/display/qxl.c
  --- ./hw/display/qxl.c
  +++ /tmp/nothing/hw/display/qxl.c
  @@ -2232,7 +2232,6 @@ static void qxl_realize_secondary(PCIDev
   
       qxl_init_ramsize(qxl);
       memory_region_init_ram(&qxl->vga.vram, OBJECT(dev), "qxl.vgavram",
  -                           qxl->vga.vram_size, &error_fatal);
       qxl->vga.vram_ptr = memory_region_get_ram_ptr(&qxl->vga.vram);
       qxl->vga.con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
       qxl->ssd.dcl.con = qxl->vga.con;
  diff -u -p ./hw/display/cg3.c /tmp/nothing/hw/display/cg3.c
  --- ./hw/display/cg3.c
  +++ /tmp/nothing/hw/display/cg3.c
  @@ -311,7 +311,6 @@ static void cg3_realizefn(DeviceState *d
       }
   
       memory_region_init_ram(&s->vram_mem, NULL, "cg3.vram", s->vram_size,
  -                           &error_fatal);
       memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA);
       sysbus_init_mmio(sbd, &s->vram_mem);
   
  diff -u -p ./hw/dma/sparc32_dma.c /tmp/nothing/hw/dma/sparc32_dma.c
  --- ./hw/dma/sparc32_dma.c
  +++ /tmp/nothing/hw/dma/sparc32_dma.c
  @@ -309,7 +309,6 @@ static void sparc32_espdma_device_realiz
       esp->dma_opaque = SPARC32_DMA_DEVICE(dev);
       sysbus->it_shift = 2;
       esp->dma_enabled = 1;
  -    sysbus_realize(SYS_BUS_DEVICE(sysbus), &error_fatal);
   }
   
   static void sparc32_espdma_device_class_init(ObjectClass *klass, void *data)
  @@ -344,7 +343,6 @@ static void sparc32_ledma_device_realize
       SysBusPCNetState *lance = SYSBUS_PCNET(&s->lance);
   
       object_property_set_link(OBJECT(lance), "dma", OBJECT(dev), &error_abort);
  -    sysbus_realize(SYS_BUS_DEVICE(lance), &error_fatal);
   }
   
   static void sparc32_ledma_device_class_init(ObjectClass *klass, void *data)
  diff -u -p ./hw/block/onenand.c /tmp/nothing/hw/block/onenand.c
  --- ./hw/block/onenand.c
  +++ /tmp/nothing/hw/block/onenand.c
  @@ -812,7 +812,6 @@ static void onenand_realize(DeviceState
       s->otp = memset(g_malloc((64 + 2) << PAGE_SHIFT),
                       0xff, (64 + 2) << PAGE_SHIFT);
       memory_region_init_ram_nomigrate(&s->ram, OBJECT(s), "onenand.ram",
  -                           0xc000 << s->shift, &error_fatal);
       vmstate_register_ram_global(&s->ram);
       ram = memory_region_get_ram_ptr(&s->ram);
       s->boot[0] = ram + (0x0000 << s->shift);
  diff -u -p ./hw/isa/vt82c686.c /tmp/nothing/hw/isa/vt82c686.c
  --- ./hw/isa/vt82c686.c
  +++ /tmp/nothing/hw/isa/vt82c686.c
  @@ -618,7 +618,6 @@ static void vt82c686b_realize(PCIDevice
       qdev_init_gpio_out(dev, &s->cpu_intr, 1);
       isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
       isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
  -                          &error_fatal);
       isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
       i8254_pit_init(isa_bus, 0x40, 0, NULL);
       i8257_dma_init(isa_bus, 0);
  @@ -699,7 +698,6 @@ static void vt8231_realize(PCIDevice *d,
       qdev_init_gpio_out(dev, &s->cpu_intr, 1);
       isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
       isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
  -                          &error_fatal);
       isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
       i8254_pit_init(isa_bus, 0x40, 0, NULL);
       i8257_dma_init(isa_bus, 0);
  diff -u -p ./hw/isa/isa-superio.c /tmp/nothing/hw/isa/isa-superio.c
  --- ./hw/isa/isa-superio.c
  +++ /tmp/nothing/hw/isa/isa-superio.c
  @@ -141,7 +141,6 @@ static void isa_superio_realize(DeviceSt
       /* Keyboard, mouse */
       isa = isa_new(TYPE_I8042);
       object_property_add_child(OBJECT(sio), TYPE_I8042, OBJECT(isa));
  -    isa_realize_and_unref(isa, bus, &error_fatal);
       sio->kbc = isa;
   
       /* IDE */
  diff -u -p ./hw/isa/isa-bus.c /tmp/nothing/hw/isa/isa-bus.c
  --- ./hw/isa/isa-bus.c
  +++ /tmp/nothing/hw/isa/isa-bus.c
  @@ -61,7 +61,6 @@ ISABus *isa_bus_new(DeviceState *dev, Me
       }
       if (!dev) {
           dev = qdev_new("isabus-bridge");
  -        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
       }
   
       isabus = ISA_BUS(qbus_create(TYPE_ISA_BUS, dev, NULL));
  diff -u -p ./hw/misc/macio/macio.c /tmp/nothing/hw/misc/macio/macio.c
  --- ./hw/misc/macio/macio.c
  +++ /tmp/nothing/hw/misc/macio/macio.c
  @@ -286,7 +286,6 @@ static void macio_newworld_realize(PCIDe
       /* OpenPIC */
       qdev_prop_set_uint32(pic_dev, "model", OPENPIC_MODEL_KEYLARGO);
       sysbus_dev = SYS_BUS_DEVICE(&ns->pic);
  -    sysbus_realize_and_unref(sysbus_dev, &error_fatal);
       memory_region_add_subregion(&s->bar, 0x40000,
                                   sysbus_mmio_get_region(sysbus_dev, 0));
   
  diff -u -p ./hw/misc/xlnx-versal-xramc.c /tmp/nothing/hw/misc/xlnx-versal-xramc.c
  --- ./hw/misc/xlnx-versal-xramc.c
  +++ /tmp/nothing/hw/misc/xlnx-versal-xramc.c
  @@ -182,7 +182,6 @@ static void xram_ctrl_realize(DeviceStat
   
       memory_region_init_ram(&s->ram, OBJECT(s),
                              object_get_canonical_path_component(OBJECT(s)),
  -                           s->cfg.size, &error_fatal);
       sysbus_init_mmio(sbd, &s->ram);
   }
   
  diff -u -p ./hw/sparc64/sun4u.c /tmp/nothing/hw/sparc64/sun4u.c
  --- ./hw/sparc64/sun4u.c
  +++ /tmp/nothing/hw/sparc64/sun4u.c
  @@ -509,7 +509,6 @@ static void ram_realize(DeviceState *dev
       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
   
       memory_region_init_ram_nomigrate(&d->ram, OBJECT(d), "sun4u.ram", d->size,
  -                           &error_fatal);
       vmstate_register_ram_global(&d->ram);
       sysbus_init_mmio(sbd, &d->ram);
   }
  diff -u -p ./hw/nvram/eeprom_at24c.c /tmp/nothing/hw/nvram/eeprom_at24c.c
  --- ./hw/nvram/eeprom_at24c.c
  +++ /tmp/nothing/hw/nvram/eeprom_at24c.c
  @@ -135,7 +135,6 @@ static void at24c_eeprom_realize(DeviceS
           }
   
           if (blk_set_perm(ee->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
  -                         BLK_PERM_ALL, &error_fatal) < 0)
           {
               error_setg(errp, "%s: Backing file incorrect permission",
                          TYPE_AT24C_EE);
  diff -u -p ./hw/arm/xlnx-zynqmp.c /tmp/nothing/hw/arm/xlnx-zynqmp.c
  --- ./hw/arm/xlnx-zynqmp.c
  +++ /tmp/nothing/hw/arm/xlnx-zynqmp.c
  @@ -219,7 +219,6 @@ static void xlnx_zynqmp_create_rpu(Machi
           }
       }
   
  -    qdev_realize(DEVICE(&s->rpu_cluster), NULL, &error_fatal);
   }
   
   static void xlnx_zynqmp_init(Object *obj)
  @@ -352,7 +351,6 @@ static void xlnx_zynqmp_realize(DeviceSt
       qdev_prop_set_bit(DEVICE(&s->gic),
                         "has-virtualization-extensions", s->virt);
   
  -    qdev_realize(DEVICE(&s->apu_cluster), NULL, &error_fatal);
   
       /* Realize APUs before realizing the GIC. KVM requires this.  */
       for (i = 0; i < num_apus; i++) {
  diff -u -p ./hw/arm/allwinner-a10.c /tmp/nothing/hw/arm/allwinner-a10.c
  --- ./hw/arm/allwinner-a10.c
  +++ /tmp/nothing/hw/arm/allwinner-a10.c
  @@ -99,7 +99,6 @@ static void aw_a10_realize(DeviceState *
       sysbus_connect_irq(sysbusdev, 5, qdev_get_gpio_in(dev, 68));
   
       memory_region_init_ram(&s->sram_a, OBJECT(dev), "sram A", 48 * KiB,
  -                           &error_fatal);
       memory_region_add_subregion(get_system_memory(), 0x00000000, &s->sram_a);
       create_unimplemented_device("a10-sram-ctrl", 0x01c00000, 4 * KiB);
   
  diff -u -p ./hw/arm/xlnx-versal.c /tmp/nothing/hw/arm/xlnx-versal.c
  --- ./hw/arm/xlnx-versal.c
  +++ /tmp/nothing/hw/arm/xlnx-versal.c
  @@ -403,7 +403,6 @@ static void versal_realize(DeviceState *
   
       /* Create the On Chip Memory (OCM).  */
       memory_region_init_ram(&s->lpd.mr_ocm, OBJECT(s), "ocm",
  -                           MM_OCM_SIZE, &error_fatal);
   
       memory_region_add_subregion_overlap(&s->mr_ps, MM_OCM, &s->lpd.mr_ocm, 0);
       memory_region_add_subregion_overlap(&s->fpd.apu.mr, 0, &s->mr_ps, 0);
  diff -u -p ./hw/pci/pci.c /tmp/nothing/hw/pci/pci.c
  --- ./hw/pci/pci.c
  +++ /tmp/nothing/hw/pci/pci.c
  @@ -2412,7 +2412,6 @@ static void pci_add_option_rom(PCIDevice
           snprintf(name, sizeof(name), "%s.rom", object_get_typename(OBJECT(pdev)));
       }
       pdev->has_rom = true;
  -    memory_region_init_rom(&pdev->rom, OBJECT(pdev), name, pdev->romsize, &error_fatal);
       ptr = memory_region_get_ram_ptr(&pdev->rom);
       if (load_image_size(path, ptr, size) < 0) {
           error_setg(errp, "failed to load romfile \"%s\"", pdev->romfile);
  diff -u -p ./hw/pci-bridge/pci_expander_bridge.c /tmp/nothing/hw/pci-bridge/pci_expander_bridge.c
  --- ./hw/pci-bridge/pci_expander_bridge.c
  +++ /tmp/nothing/hw/pci-bridge/pci_expander_bridge.c
  @@ -264,7 +264,6 @@ static void pxb_dev_realize_common(PCIDe
           goto err_register_bus;
       }
   
  -    sysbus_realize_and_unref(SYS_BUS_DEVICE(ds), &error_fatal);
       if (bds) {
           qdev_realize_and_unref(bds, &bus->qbus, &error_fatal);
       }
  diff -u -p ./softmmu/vl.c /tmp/nothing/softmmu/vl.c
  --- ./softmmu/vl.c
  +++ /tmp/nothing/softmmu/vl.c
  @@ -2189,7 +2189,6 @@ static void qemu_record_config_group(con
           assert(!from_json);
           keyval_merge(machine_opts_dict, dict, errp);
       } else if (g_str_equal(group, "smp-opts")) {
  -        machine_merge_property("smp", dict, &error_fatal);
       } else {
           abort();
       }
  @@ -2309,7 +2308,6 @@ static int do_configure_accelerator(void
       object_apply_compat_props(OBJECT(accel));
       qemu_opt_foreach(opts, accelerator_set_property,
                        accel,
  -                     &error_fatal);
   
       ret = accel_init_machine(accel, current_machine);
       if (ret < 0) {

-- 
Eduardo

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

[Peter Maydell]

On Tue, 24 Aug 2021 at 14:58, Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Tue, Aug 24, 2021 at 01:16:40PM +0100, Peter Maydell wrote:
> > On Tue, 24 Aug 2021 at 13:05, Markus Armbruster <armbru@redhat.com> wrote:
> > > When you know that all callers handle errors like &error_fatal does, use
> > > of &error_fatal doesn't produce wrong behavior.  It's still kind of
> > > wrong, because relying on such a non-local argument without a genuine
> > > need is.
> >
> > Not using error_fatal results in quite a bit of extra boilerplate
> > for all those extra explicit "check for failure, print the error
> > message and exit" points.
>
> I don't get it.  There's no need for extra boilerplate if the
> caller is using &error_fatal.

Yes, that is what I mean: if you do not use error_fatal,
then there is extra boilerplate. Markus is suggesting that
we should avoid using error_fatal, and my response is "the
cost of that would be that we add boilerplate".

> > Right now we have nearly a thousand instances of error_fatal
> > in the codebase, incidentally.
>
> It looks like 73 of them are in functions that take an Error**
> argument.

Those could probably be fairly easily adjusted to pass the error
back instead. Still leaves nearly 900 to go :-)

-- PMM

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Tue, Aug 24, 2021 at 01:16:40PM +0100, Peter Maydell wrote:
>> On Tue, 24 Aug 2021 at 13:05, Markus Armbruster <armbru@redhat.com> wrote:
>> > When you know that all callers handle errors like &error_fatal does, use
>> > of &error_fatal doesn't produce wrong behavior.  It's still kind of
>> > wrong, because relying on such a non-local argument without a genuine
>> > need is.
>> 
>> Not using error_fatal results in quite a bit of extra boilerplate
>> for all those extra explicit "check for failure, print the error
>> message and exit" points.
>
> I don't get it.  There's no need for extra boilerplate if the
> caller is using &error_fatal.
>
>> If the MachineState init function took
>> an Error** that might be a mild encouragement to "pass an Error
>> upward rather than exiting"; but it doesn't.
>
> Agreed.
>
>> 
>> Right now we have nearly a thousand instances of error_fatal
>> in the codebase, incidentally.
>
> It looks like 73 of them are in functions that take an Error**
> argument.
>
> Coccinelle patch for finding them:
>
> @@
> typedef Error;
> type T;
> identifier errp, fn;
> @@
>  T fn(..., Error **errp)
>  {
>  ...
> *&error_fatal
>  ...
>  }
>
>
> Coccinelle output:

[...]

These do look suspicious to me.

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

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 24 Aug 2021 at 13:05, Markus Armbruster <armbru@redhat.com> wrote:
>> When you know that all callers handle errors like &error_fatal does, use
>> of &error_fatal doesn't produce wrong behavior.  It's still kind of
>> wrong, because relying on such a non-local argument without a genuine
>> need is.
>
> Not using error_fatal results in quite a bit of extra boilerplate
> for all those extra explicit "check for failure, print the error
> message and exit" points. If the MachineState init function took
> an Error** that might be a mild encouragement to "pass an Error
> upward rather than exiting"; but it doesn't.
>
> Right now we have nearly a thousand instances of error_fatal
> in the codebase, incidentally.

Use of &error_fatal is clearly superior to accomplishing the same
behavior the way you describe.

My point was this behavior is usually wrong in functions with an Error
**errp parameter.

When a function is only ever used in contexts where errors are handled
the way &error_fatal does, then you can keep things a bit more readable
with &error_fatal and without an Error **errp parameter.  Nothing wrong
with that.

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

[Peter Maydell]

On Tue, 24 Aug 2021 at 15:27, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Tue, 24 Aug 2021 at 13:05, Markus Armbruster <armbru@redhat.com> wrote:
> >> When you know that all callers handle errors like &error_fatal does, use
> >> of &error_fatal doesn't produce wrong behavior.  It's still kind of
> >> wrong, because relying on such a non-local argument without a genuine
> >> need is.
> >
> > Not using error_fatal results in quite a bit of extra boilerplate
> > for all those extra explicit "check for failure, print the error
> > message and exit" points. If the MachineState init function took
> > an Error** that might be a mild encouragement to "pass an Error
> > upward rather than exiting"; but it doesn't.
> >
> > Right now we have nearly a thousand instances of error_fatal
> > in the codebase, incidentally.
>
> Use of &error_fatal is clearly superior to accomplishing the same
> behavior the way you describe.
>
> My point was this behavior is usually wrong in functions with an Error
> **errp parameter.

Right, but as Eduardo has noted, only about 8% of our use of
error_fatal is like that. The vast bulk is other cases, so
if you want to call it "kind of wrong" we ought to have a view
of how it could be done otherwise.

-- PMM

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

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 24 Aug 2021 at 15:27, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > On Tue, 24 Aug 2021 at 13:05, Markus Armbruster <armbru@redhat.com> wrote:
>> >> When you know that all callers handle errors like &error_fatal does, use
>> >> of &error_fatal doesn't produce wrong behavior.  It's still kind of
>> >> wrong, because relying on such a non-local argument without a genuine
>> >> need is.
>> >
>> > Not using error_fatal results in quite a bit of extra boilerplate
>> > for all those extra explicit "check for failure, print the error
>> > message and exit" points. If the MachineState init function took
>> > an Error** that might be a mild encouragement to "pass an Error
>> > upward rather than exiting"; but it doesn't.
>> >
>> > Right now we have nearly a thousand instances of error_fatal
>> > in the codebase, incidentally.
>>
>> Use of &error_fatal is clearly superior to accomplishing the same
>> behavior the way you describe.
>>
>> My point was this behavior is usually wrong in functions with an Error
>> **errp parameter.
>
> Right, but as Eduardo has noted, only about 8% of our use of
> error_fatal is like that. The vast bulk is other cases, so
> if you want to call it "kind of wrong" we ought to have a view
> of how it could be done otherwise.

True, except when I called it "kind of wrong", I was still talking about
functions with an Error **errp parameter.

Many (most?) existing uses of &error_fatal are just fine.  Which pleases
me, having created it in commit a29a37b994c.

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

[Peter Maydell]

On Tue, 24 Aug 2021 at 16:15, Markus Armbruster <armbru@redhat.com> wrote:
> True, except when I called it "kind of wrong", I was still talking about
> functions with an Error **errp parameter.

Oh yes, so you were. I even quoted your sentence starting
"In functions with an Error **errp parameter ...".
I must have been half-asleep still this morning.

Apologies for starting an unnecessary thread after which we all
turn out to be in complete agreement :-)

-- PMM

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

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 24 Aug 2021 at 16:15, Markus Armbruster <armbru@redhat.com> wrote:
>> True, except when I called it "kind of wrong", I was still talking about
>> functions with an Error **errp parameter.
>
> Oh yes, so you were. I even quoted your sentence starting
> "In functions with an Error **errp parameter ...".
> I must have been half-asleep still this morning.
>
> Apologies for starting an unnecessary thread after which we all
> turn out to be in complete agreement :-)

No problem at all :)

[PATCH v2 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..642bf0d6811 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_fatal);
         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 71f6629ccde..a959943d856 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_fatal);
+    object_property_set_link(OBJECT(dev), "dma",
+                             OBJECT(pci_dma_memory_region(dev)), &error_fatal);
     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