| Message ID | 20240924011156.48252-1-gaoshiyuan@baidu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] virtio-pci: fix memory_region_find for VirtIOPCIRegion's MR | expand |
On 24.09.24 03:11, Gao Shiyuan wrote: Make sure to version your patch series. For example, via $ git format-patch -v1 ... > 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. I'm starting to wonder if memory_region_find() is really the right fun > > 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") Commit id is not unique. Use 12 digits please. I'm still not quite sure if memory_region_find() is really the right thing to use here, but I'm no expert on that so I'm hoping virtio/PCI people can review.
> Make sure to version your patch series. For example, via > $ git format-patch -v1 ... Thanks, the first version forgot to CC qemu-devel, I resent it. I'll add version to next version. > > > 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. > > I'm starting to wonder if memory_region_find() is really the right fun > > > > > 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") > > Commit id is not unique. Use 12 digits please. Thanks, I will make changes in the next version. > > I'm still not quite sure if memory_region_find() is really the right > thing to use here, but I'm no expert on that so I'm hoping virtio/PCI > people can review. > Directly traversing the subregions of VirtIOPCIRegion's MR is a relatively simple method(Now only notify region MR has subregion and only has one layer). But if want to be more general, we need to consider multiple layers and the priority of subregions, which adds complexity. I think it would be better to use memory_region_find. Does anyone have any opinions?
"Gao,Shiyuan" via <qemu-devel@nongnu.org> writes: >> > >> > 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") >> >> Commit id is not unique. Use 12 digits please. > > Thanks, I will make changes in the next version. > >> >> I'm still not quite sure if memory_region_find() is really the right >> thing to use here, but I'm no expert on that so I'm hoping virtio/PCI >> people can review. >> > > Directly traversing the subregions of VirtIOPCIRegion's MR is a relatively > simple method(Now only notify region MR has subregion and only has one layer). > > But if want to be more general, we need to consider multiple layers and > the priority of subregions, which adds complexity. > > I think it would be better to use memory_region_find. > > Does anyone have any opinions? I'm new to QEMU memory API, but here's my two cents. What is special here is that virtio PCI transport provides a "PCI configuration access capability" that allows accessing BAR-mapped virtio config regions *BEFORE* BARs are initialized. At that time, the BAR MemoryRegion is not yet added to the PCI MemoryRegion (and thus the memory AddressSpace), and that prevents memory_region_find() from working because the API requires the given MemoryRegion to be in an AddressSpace. I think it is reasonable to add one (or two, one for mmio and other other port i/o?) per-virtio-device address space for such virtio config regions. The PCI capability can be regarded as a window to a register space that is inaccessible otherwise under certain conditions. Also, device-specific address spaces are not rare today. Directly travering the subregions does not look like a good approach as it breaks abstraction. Accesses to memory regions require extra care about RCU and refcount. Scatter such operations in multiple files will make the abstraction harder to maintain. Adding another API for finding a subregion within a region not in any address space is an alternative, but I'm not sure if that looks like an overkill. -- Best Regards Junjie Mao
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 <gaoshiyuan@baidu.com> > Signed-off-by: Zuo Boqun <zuoboqun@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
On Tue, Sep 24, 2024 at 02:31:20PM +0200, David Hildenbrand wrote: > On 24.09.24 03:11, Gao Shiyuan wrote: > > Make sure to version your patch series. For example, via > $ git format-patch -v1 ... > > > 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. > > I'm starting to wonder if memory_region_find() is really the right fun > > > > > 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") > > Commit id is not unique. Use 12 digits please. > > I'm still not quite sure if memory_region_find() is really the right thing > to use here, but I'm no expert on that so I'm hoping virtio/PCI people can > review. I donnu, what would you use? > -- > Cheers, > > David / dhildenb
On 30.09.24 16:00, Michael S. Tsirkin wrote: > On Tue, Sep 24, 2024 at 02:31:20PM +0200, David Hildenbrand wrote: >> On 24.09.24 03:11, Gao Shiyuan wrote: >> >> Make sure to version your patch series. For example, via >> $ git format-patch -v1 ... >> >>> 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. >> >> I'm starting to wonder if memory_region_find() is really the right fun >> >>> >>> 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") >> >> Commit id is not unique. Use 12 digits please. >> >> I'm still not quite sure if memory_region_find() is really the right thing >> to use here, but I'm no expert on that so I'm hoping virtio/PCI people can >> review. > > I donnu, what would you use? I assume there is currently not better alternative. Scanning all address spaces to find+walk a flatview sounds quite wasteful during each virtio_address_space_lookup ... So this change here looks ok to me for the time being.
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"); 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"); + 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; uint32_t legacy_io_bar_idx; uint32_t msix_bar_idx; uint32_t modern_io_bar_idx;
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 <gaoshiyuan@baidu.com> Signed-off-by: Zuo Boqun <zuoboqun@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(+)