| Message ID | 20240812122027.65600-1-gaoshiyuan@baidu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] virtio-pci: return RAM device MR when set host notifier success | expand |
ping. When VHOST_USER_PROTOCOL_F_HOST_NOTIFIER this feature negotiated and virtio_queue_set_host_notifier_mr success on system blk device's queue, the VM can't load MBR if the notify region's address above 4GB. Because the vp_notify in seabios maybe cann't notify the hardware accelerator by kickfd from qemu notify_ops. Thanks. > -----Original Message----- > From: Gao Shiyuan <gaoshiyuan@baidu.com> > Date: Mon, 12 Aug 2024 20:09:01 +0800 > Subject: [PATCH 1/1] virtio-pci: return RAM device MR when set host notifier > success > > When vhost-user backend register memory region based host notifiers, > we should return RAM device MR of notify region MR's subregion in > virtio_address_space_lookup. > > In seabios, it will use virtio PCI Configration Access Capability > access notify region when assign notify region above 4GB. This will > exit to QEMU and invoke virtio_address_space_write. When vhost-user > backend register memory region based host notifiers, return RAM device > MR instead of notify region MR is suitable. > > Co-developed-by: Zuo Boqun <zuoboqun@baidu.com> > Signed-off-by: Gao Shiyuan <gaoshiyuan@baidu.com> > Signed-off-by: Zuo Boqun <zuoboqun@baidu.com>
On Mon, Aug 12, 2024 at 08:20:27PM +0800, Gao Shiyuan wrote: > When vhost-user backend register memory region based host notifiers, > we should return RAM device MR of notify region MR's subregion in > virtio_address_space_lookup. > > In seabios, it will use virtio PCI Configration Access Capability > access notify region when assign notify region above 4GB. This will > exit to QEMU and invoke virtio_address_space_write. When vhost-user > backend register memory region based host notifiers, return RAM device > MR instead of notify region MR is suitable. I can't really parse this. > Co-developed-by: Zuo Boqun <zuoboqun@baidu.com> > Signed-off-by: Gao Shiyuan <gaoshiyuan@baidu.com> > Signed-off-by: Zuo Boqun <zuoboqun@baidu.com> CC Jason > --- > hw/virtio/virtio-pci.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 9534730bba..167ac9718a 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -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; > } > } Poking at internals of MR like this is not nice. Doesn't memory_region_find work for this? > > -- > 2.39.3 (Apple Git-146)
On Fri, Aug 16, 2024 at 6:29 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Aug 12, 2024 at 08:20:27PM +0800, Gao Shiyuan wrote: > > When vhost-user backend register memory region based host notifiers, > > we should return RAM device MR of notify region MR's subregion in > > virtio_address_space_lookup. > > > > In seabios, it will use virtio PCI Configration Access Capability > > access notify region when assign notify region above 4GB. This will > > exit to QEMU and invoke virtio_address_space_write. When vhost-user > > backend register memory region based host notifiers, return RAM device > > MR instead of notify region MR is suitable. > > > I can't really parse this. > > > Co-developed-by: Zuo Boqun <zuoboqun@baidu.com> > > Signed-off-by: Gao Shiyuan <gaoshiyuan@baidu.com> > > Signed-off-by: Zuo Boqun <zuoboqun@baidu.com> > > CC Jason > > > --- > > hw/virtio/virtio-pci.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index 9534730bba..167ac9718a 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -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; > > } > > } > > Poking at internals of MR like this is not nice. > Doesn't memory_region_find work for this? Or I wonder if this is a side effect of: commit a93c8d828af186d9a6a1c915a1be8ba22fb89849 Author: Alexey Kardashevskiy <aik@ozlabs.ru> Date: Mon Oct 9 14:19:41 2017 +1100 virtio-pci: Replace modern_as with direct access to modern_bar The modern bar is accessed now via yet another address space created just for that purpose and it does not really need FlatView and dispatch tree as it has a single memory region so it is just a waste of memory. Things get even worse when there are dozens or hundreds of virtio-pci devices - since these address spaces are global, changing any of them triggers rebuilding all address spaces. This replaces indirect accesses to the modern BAR with a simple lookup and direct calls to memory_region_dispatch_read/write. This is expected to save lots of memory at boot time after applying: [Qemu-devel] [PULL 00/32] Misc changes for 2017-09-22 Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Thanks
> > When vhost-user backend register memory region based host notifiers, > > we should return RAM device MR of notify region MR's subregion in > > virtio_address_space_lookup. > > > > In seabios, it will use virtio PCI Configration Access Capability > > access notify region when assign notify region above 4GB. This will > > exit to QEMU and invoke virtio_address_space_write. When vhost-user > > backend register memory region based host notifiers, return RAM device > > MR instead of notify region MR is suitable. > > > I can't really parse this. > When booting from disk, SeaBIOS will invoke the INT 0x13 interrupt handler to load the MBR. The interrupt handler will eventually utilize the virtio block driver in SeaBIOS when the disk is a virtio block device, and after notifying the backend via `vp_notify`, the MBR will be loaded. When assign the address of notify region in the modern bar above 4G, the `vp_notify` in SeaBIOS will use PCI Configuration Access Capability to write notify region. This will trap into QEMU and be handled by the host bridge when we don't enable mmconfig. QEMU will call `virtio_write_config`, and since it writes to the BAR region through the PCI Configuration Access capability, it will call `virtio_address_space_write`. When VHOST_USER_PROTOCOL_F_HOST_NOTIFIER protocol feature has been successfully negotiated and vhost-user backend registers memory region based host notifiers, QEMU need write the mmap address instead of eventfd notify the hardware accelerator at the vhost-user backend. So virtio_address_space_lookup in virtio_address_space_write need return a host-notifier subregion of notify MR. > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -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; > > } > > } > > Poking at internals of MR like this is not nice. > Doesn't memory_region_find work for this? It seems fine, I’ll try it out.
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 9534730bba..167ac9718a 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -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; } }