| Message ID | 20240903120304.97833-1-gaoshiyuan@baidu.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/1] virtio-pci: Add lookup subregion of VirtIOPCIRegion MR | expand |
On 03.09.24 14:03, Gao Shiyuan via wrote: > 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 ®->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?
> > 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 ®->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 > Sorry for this, Peter and David's Command line can reproduce in my environment. But this patch works fine in my testing environment, it can start VM(both q35 and i440fx) success and I will compare the differences in command line parameters with Peter's. As for qtest, When I use this command line(Change the machine from q35 to i440fx), it looks ok cat << EOF | ./qemu-system-x86_64 -display none -machine accel=qtest, -m \ 512M -machine pc -nodefaults -device virtio-balloon -qtest stdio outl 0xcf8 0x80000890 outl 0xcfc 0x2 outl 0xcf8 0x80000891 inl 0xcfc EOF So I think there might exists other bug, and this patch just happens to trigger them. I will find it as soon as possible.
>> 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 ®->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?
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); + *off = mrs.offset_within_region; + memory_region_unref(mrs.mr); + return mrs.mr; } }
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.