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