>> 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 <zuobo...@baidu.com>
>> Signed-off-by: Gao Shiyuan <gaoshiy...@baidu.com>
>> Signed-off-by: Zuo Boqun <zuobo...@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 ®->mr;
>> + MemoryRegionSection mrs = memory_region_find(®->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 ®->mr;
+ mr = ®->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?
