Gao Shiyuan via <qemu-devel@nongnu.org> writes:
> As shown below, if a virtio PCI device is attached under a pci-bridge, the MR > of VirtIOPCIRegion does not belong to any address space. So memory_region_find > cannot be used to search for this MR. > > Introduce the virtio-pci and pci_bridge_pci address spaces to solve this > problem. > > Before: > memory-region: pci_bridge_pci > 0000000000000000-ffffffffffffffff (prio 0, i/o): pci_bridge_pci > 00000000fe200000-00000000fe200fff (prio 1, i/o): virtio-blk-pci-msix > 00000000fe200000-00000000fe20016f (prio 0, i/o): msix-table > 00000000fe200800-00000000fe200807 (prio 0, i/o): msix-pba > 000000a000400000-000000a000403fff (prio 1, i/o): virtio-pci > 000000a000400000-000000a000400fff (prio 0, i/o): > virtio-pci-common-virtio-blk > 000000a000401000-000000a000401fff (prio 0, i/o): > virtio-pci-isr-virtio-blk > 000000a000402000-000000a000402fff (prio 0, i/o): > virtio-pci-device-virtio-blk > 000000a000403000-000000a000403fff (prio 0, i/o): > virtio-pci-notify-virtio-blk > > After: > address-space: pci_bridge_pci > 0000000000000000-ffffffffffffffff (prio 0, i/o): pci_bridge_pci > 00000000fe200000-00000000fe200fff (prio 1, i/o): virtio-blk-pci-msix > 00000000fe200000-00000000fe20016f (prio 0, i/o): msix-table > 00000000fe200800-00000000fe200807 (prio 0, i/o): msix-pba > 000000a000400000-000000a000403fff (prio 1, i/o): virtio-pci > 000000a000400000-000000a000400fff (prio 0, i/o): > virtio-pci-common-virtio-blk > 000000a000401000-000000a000401fff (prio 0, i/o): > virtio-pci-isr-virtio-blk > 000000a000402000-000000a000402fff (prio 0, i/o): > virtio-pci-device-virtio-blk > 000000a000403000-000000a000403fff (prio 0, i/o): > virtio-pci-notify-virtio-blk > > address-space: virtio-pci > 000000a000400000-000000a000403fff (prio 1, i/o): virtio-pci > 000000a000400000-000000a000400fff (prio 0, i/o): > virtio-pci-common-virtio-blk > 000000a000401000-000000a000401fff (prio 0, i/o): virtio-pci-isr-virtio-blk > 000000a000402000-000000a000402fff (prio 0, i/o): > virtio-pci-device-virtio-blk > 000000a000403000-000000a000403fff (prio 0, i/o): > virtio-pci-notify-virtio-blk > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2576 > Fixes: ffa8a3e ("virtio-pci: Add lookup subregion of VirtIOPCIRegion MR") > > Signed-off-by: Gao Shiyuan <gaoshiy...@baidu.com> > Signed-off-by: Zuo Boqun <zuobo...@baidu.com> > --- > hw/pci/pci_bridge.c | 2 ++ > hw/virtio/virtio-pci.c | 3 +++ > include/hw/pci/pci_bridge.h | 1 + > include/hw/virtio/virtio-pci.h | 1 + > 4 files changed, 7 insertions(+) > > diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c > index 6a4e38856d..74683e7445 100644 > --- a/hw/pci/pci_bridge.c > +++ b/hw/pci/pci_bridge.c > @@ -380,6 +380,7 @@ void pci_bridge_initfn(PCIDevice *dev, const char > *typename) > sec_bus->map_irq = br->map_irq ? br->map_irq : pci_swizzle_map_irq_fn; > sec_bus->address_space_mem = &br->address_space_mem; > memory_region_init(&br->address_space_mem, OBJECT(br), "pci_bridge_pci", > UINT64_MAX); > + address_space_init(&br->as_mem, &br->address_space_mem, > "pci_bridge_pci"); I don't think this "pci_bridge_pci" address space is necessary. The VirtIOPCIProxy.modern_as AddressSpace is sufficient for memory_region_add() to work. > sec_bus->address_space_io = &br->address_space_io; > memory_region_init(&br->address_space_io, OBJECT(br), "pci_bridge_io", > 4 * GiB); > @@ -399,6 +400,7 @@ void pci_bridge_exitfn(PCIDevice *pci_dev) > PCIBridge *s = PCI_BRIDGE(pci_dev); > assert(QLIST_EMPTY(&s->sec_bus.child)); > QLIST_REMOVE(&s->sec_bus, sibling); > + address_space_destroy(&s->as_mem); > pci_bridge_region_del(s, &s->windows); > pci_bridge_region_cleanup(s, &s->windows); > /* object_unparent() is called automatically during device deletion */ > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 4d832fe845..502b9751dc 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -2180,6 +2180,8 @@ static void virtio_pci_realize(PCIDevice *pci_dev, > Error **errp) > /* PCI BAR regions must be powers of 2 */ > pow2ceil(proxy->notify.offset + proxy->notify.size)); > > + address_space_init(&proxy->modern_as, &proxy->modern_bar, "virtio-pci"); Can we add some suffix to the address space name so that we can tell them apart when there are multiple virtio devices created? > + > if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) { > proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF; > } > @@ -2275,6 +2277,7 @@ static void virtio_pci_exit(PCIDevice *pci_dev) > pci_is_express(pci_dev)) { > pcie_aer_exit(pci_dev); > } > + address_space_destroy(&proxy->modern_as); > } > > static void virtio_pci_reset(DeviceState *qdev) > diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h > index 5cd452115a..2e807760e4 100644 > --- a/include/hw/pci/pci_bridge.h > +++ b/include/hw/pci/pci_bridge.h > @@ -72,6 +72,7 @@ struct PCIBridge { > */ > MemoryRegion address_space_mem; > MemoryRegion address_space_io; > + AddressSpace as_mem; > > PCIBridgeWindows windows; > > diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h > index 9e67ba38c7..fddceaaa47 100644 > --- a/include/hw/virtio/virtio-pci.h > +++ b/include/hw/virtio/virtio-pci.h > @@ -147,6 +147,7 @@ struct VirtIOPCIProxy { > }; > MemoryRegion modern_bar; > MemoryRegion io_bar; > + AddressSpace modern_as; How about naming it "config_as" or "config_mem_as"? While the PCI configuration access capability is specific to modern devices, what it maps (and only maps) are the virtio config regions. Also, we may need another "config_io_as" for the port I/O mapped notification config region. > uint32_t legacy_io_bar_idx; > uint32_t msix_bar_idx; > uint32_t modern_io_bar_idx; -- Best Regards Junjie Mao
