[PATCH 1/1] virtio-pci: return RAM device MR when set host notifier success

[PATCH 1/1] virtio-pci: return RAM device MR when set host notifier success

[Gao Shiyuan via]

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>
---
 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 &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;
         }
     }
 
-- 
2.39.3 (Apple Git-146)

Re: [PATCH 1/1] virtio-pci: return RAM device MR when set host notifier success

[Gao,Shiyuan via]

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>

Re: [PATCH 1/1] virtio-pci: return RAM device MR when set host notifier success

[Michael S. Tsirkin]

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 &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;
>          }
>      }

Poking at internals of MR like this is not nice.
Doesn't memory_region_find work for this?



>  
> -- 
> 2.39.3 (Apple Git-146)

Re: [PATCH 1/1] virtio-pci: return RAM device MR when set host notifier success

[Jason Wang]

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 &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;
> >          }
> >      }
>
> 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

Re: [PATCH 1/1] virtio-pci: return RAM device MR when set host notifier success

[Gao,Shiyuan via]

> > 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 &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;
> >          }
> >      }
>
> 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.