Re: [PATCH v3 1/1] virtio-pci: Add lookup subregion of VirtIOPCIRegion MR

[Gao,Shiyuan]

>> Now virtio_address_space_lookup only lookup common/isr/device/notify
>> MR and exclude their subregions.
>>
>> When VHOST_USER_PROTOCOL_F_HOST_NOTIFIER enable, the notify MR has
>> host-notifier subregions and we need use host-notifier MR to
>> notify the hardware accelerator directly instead of eventfd notify.
>>
>> Further more, maybe common/isr/device MR also has subregions in
>> the future, so need memory_region_find for each MR incluing
>> their subregions.
>>
>> Add lookup subregion of VirtIOPCIRegion MR instead of only lookup container 
>> MR.
>>
>> Fixes: a93c8d8 ("virtio-pci: Replace modern_as with direct access to 
>> modern_bar")
>>
>> Co-developed-by: Zuo Boqun <zuoboqun@baidu.com>
>> Signed-off-by: Gao Shiyuan <gaoshiyuan@baidu.com>
>> Signed-off-by: Zuo Boqun <zuoboqun@baidu.com>
>> ---
>>   hw/virtio/virtio-pci.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> ---
>> v2 -> v3:
>> * modify commit message
>> * remove unused variable and move mrs to the inner block
>> * replace error_report with assert
>>
>> v1 -> v2:
>> * modify commit message
>> * replace direct iteration over subregions with memory_region_find.
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 524b63e5c7..4d832fe845 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -615,8 +615,12 @@ static MemoryRegion 
>> *virtio_address_space_lookup(VirtIOPCIProxy *proxy,
>>           reg = &proxy->regs[i];
>>           if (*off >= reg->offset &&
>>               *off + len <= reg->offset + reg->size) {
>> -            *off -= reg->offset;
>> -            return &reg->mr;
>> +            MemoryRegionSection mrs = memory_region_find(&reg->mr,
>> +                                        *off - reg->offset, len);
>> +            assert(mrs.mr);
>
>We are able to trigger that assert:
>
>https://gitlab.com/qemu-project/qemu/-/issues/2576
>
>Can you take a look and send a fix?
>
>--
>Cheers,
>
>David / dhildenb

This problem is caused by the incorrect usage of the `memory_region_find` 
function.
`memory_region_find` need find address_space of the search MR, howerver the
VirtIOPCIRegion->regs[i].mr cann't find address_space, such as

memory-region: pci_bridge_pci
  0000000000000000-ffffffffffffffff (prio 0, i/o): pci_bridge_pci
    000000a000000000-000000a000003fff (prio 1, i/o): virtio-pci
      000000a000000000-000000a000000fff (prio 0, i/o): 
virtio-pci-common-virtio-balloon
      000000a000001000-000000a000001fff (prio 0, i/o): 
virtio-pci-isr-virtio-balloon
      000000a000002000-000000a000002fff (prio 0, i/o): 
virtio-pci-device-virtio-balloon
      000000a000003000-000000a000003fff (prio 0, i/o): 
virtio-pci-notify-virtio-balloon

memory_region_find
  --> memory_region_find_rcu
    --> memory_region_to_address_space
      --> return NULL

So memory_region_find cann't find these MR that not under address_space, Is 
this as expected?

There are two ways to solve this problem.

* One is direct iteration over subregions of search MR instead of 
memory_region_find.
If use this method, we will make it more general to handle multi-level
subregion scenarios, even though they do not currently exist.

@@ -610,13 +610,22 @@ static MemoryRegion
*virtio_address_space_lookup(VirtIOPCIProxy *proxy,
 {
     int i;
     VirtIOPCIRegion *reg;
+    MemoryRegion *mr, *submr;

     for (i = 0; i < ARRAY_SIZE(proxy->regs); ++i) {
         reg = &proxy->regs[i];
         if (*off >= reg->offset &&
             *off + len <= reg->offset + reg->size) {
             *off -= reg->offset;
-            return &reg->mr;
+            mr = &reg->mr;
+            QTAILQ_FOREACH(submr, &mr->subregions, subregions_link) {
+                if (*off >= submr->addr &&
+                    *off + len < submr->addr + submr->size) {
+                    *off -= submr->addr;
+                    return submr;
+                }
+            }
+            return mr;
         }
     }

* Another is add address_space for VirtIOPCIProxy and PCIBridge, so 
memory_region_find
will find the address_space of the search MR.

diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 6a4e38856d..07873c478f 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);
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 4d832fe845..83e020c0f6 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;
     }
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;
Maybe revert 1f881ea4a444ef36a8b6907b0b82be4b3af253a2 can also solve this and 
the
orignal problem.

Does anyone have any suggestions?