[PATCH 1/1] virtio-pci: fix memory_region_find for VirtIOPCIRegion's MR

[PATCH 1/1] virtio-pci: fix memory_region_find for VirtIOPCIRegion's MR

[Gao Shiyuan via]

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");
     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;
-- 
2.34.1

Re: [PATCH 1/1] virtio-pci: fix memory_region_find for VirtIOPCIRegion's MR

[David Hildenbrand]

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.

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/1] virtio-pci: fix memory_region_find for VirtIOPCIRegion's MR

[Gao,Shiyuan via]

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

Re: [PATCH 1/1] virtio-pci: fix memory_region_find for VirtIOPCIRegion's MR

[Junjie Mao]

"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

Re: [PATCH 1/1] virtio-pci: fix memory_region_find for VirtIOPCIRegion's MR

[Michael S. Tsirkin]

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

Re: [PATCH 1/1] virtio-pci: fix memory_region_find for VirtIOPCIRegion's MR

[David Hildenbrand]

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.

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/1] virtio-pci: fix memory_region_find for VirtIOPCIRegion's MR

[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

Re: [PATCH 1/1] virtio-pci: fix memory_region_find for VirtIOPCIRegion's MR

[Zuo,Boqun via]

On Wed, Sep 25, 2024 8:58 PM Junjie Mao wrote:
> > 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.

This is because memory_region_find_rcu () and memory_region_to_address_space() will find the memory container
on the top level and then get the corresponding address space of it.
If we only add "VirtIOPCIProxy.modern_as AddressSpace", memory_region_find() still cannot find a address space.
Because "pci_bridge_pci" is the memory container on top level and it doesn't have a corresponding address space.

Re: [PATCH 1/1] virtio-pci: fix memory_region_find for VirtIOPCIRegion's MR

[Junjie Mao]

"Zuo,Boqun" <zuoboqun@baidu.com> writes:

> On Wed, Sep 25, 2024 8:58 PM Junjie Mao wrote:
>> > 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.
>
> This is because memory_region_find_rcu () and memory_region_to_address_space() will find the memory container
> on the top level and then get the corresponding address space of it.
> If we only add "VirtIOPCIProxy.modern_as AddressSpace", memory_region_find() still cannot find a address space.
> Because "pci_bridge_pci" is the memory container on top level and it doesn't have a corresponding address space.

Got your point. That address space can help when a so-ill-behaved guest
enables both the bridge and the virtio device, clears the memory base of
secondary bus and then accesses the virtio configurations via the PCI
config access cap. Thanks for the clarification.

As mentioned earlier, you may want to add the I/O space counterparts for
both address spaces, as the cap can target modern I/O BARs (when they
are present) as well.

--
Best Regards
Junjie Mao

Re: [PATCH 1/1] virtio-pci: fix memory_region_find for VirtIOPCIRegion's MR

[Gao,Shiyuan via]

> >> > 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.
> >
> > This is because memory_region_find_rcu () and memory_region_to_address_space() will find the memory container
> > on the top level and then get the corresponding address space of it.
> > If we only add "VirtIOPCIProxy.modern_as AddressSpace", memory_region_find() still cannot find a address space.
> > Because "pci_bridge_pci" is the memory container on top level and it doesn't have a corresponding address space.
>
>
> Got your point. That address space can help when a so-ill-behaved guest
> enables both the bridge and the virtio device, clears the memory base of
> secondary bus and then accesses the virtio configurations via the PCI
> config access cap. Thanks for the clarification.
>
>
> As mentioned earlier, you may want to add the I/O space counterparts for
> both address spaces, as the cap can target modern I/O BARs (when they
> are present) as well.

Thanks, I'll add a address space for I/O BARs in the next version.