[PATCH V2 1/1] virtio-pci: Add lookup subregion of VirtIOPCIRegion MR

[PATCH V2 1/1] virtio-pci: Add lookup subregion of VirtIOPCIRegion MR

[Gao Shiyuan via]

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

Assign the address of notify region in the modern bar above 4G, the vp_notify
in SeaBIOS will use PCI Cfg 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 Cfg Capability, it will call virtio_address_space_write.

virtio_queue_set_host_notifier_mr add host notifier subregion of notify region
MR, 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
instead of notify MR.

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 | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

---
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 9534730bba..5d2d27a6a3 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -610,19 +610,29 @@ static MemoryRegion *virtio_address_space_lookup(VirtIOPCIProxy *proxy,
 {
     int i;
     VirtIOPCIRegion *reg;
+    MemoryRegion *mr = NULL;
+    MemoryRegionSection mrs;
 
     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;
+            mrs = memory_region_find(&reg->mr, *off - reg->offset, len);
+            if (!mrs.mr) {
+                error_report("Failed to find memory region for address"
+                             "0x%" PRIx64 "", *off);
+                return NULL;
+            }
+            *off = mrs.offset_within_region;
+            memory_region_unref(mrs.mr);
+            return mrs.mr;
         }
     }
 
     return NULL;
 }
 
+
 /* Below are generic functions to do memcpy from/to an address space,
  * without byteswaps, with input validation.
  *
-- 
2.39.3 (Apple Git-146)

Re: [PATCH V2 1/1] virtio-pci: Add lookup subregion of VirtIOPCIRegion MR

[Stefano Garzarella]

On Tue, Aug 20, 2024 at 07:56:31PM GMT, Gao Shiyuan wrote:
>When VHOST_USER_PROTOCOL_F_HOST_NOTIFIER 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.
>
>Assign the address of notify region in the modern bar above 4G, the vp_notify
>in SeaBIOS will use PCI Cfg 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 Cfg Capability, it will call virtio_address_space_write.
>
>virtio_queue_set_host_notifier_mr add host notifier subregion of notify region
>MR, 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
>instead of notify MR.
>
>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 | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
>---
>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 9534730bba..5d2d27a6a3 100644
>--- a/hw/virtio/virtio-pci.c
>+++ b/hw/virtio/virtio-pci.c
>@@ -610,19 +610,29 @@ static MemoryRegion *virtio_address_space_lookup(VirtIOPCIProxy *proxy,
> {
>     int i;
>     VirtIOPCIRegion *reg;
>+    MemoryRegion *mr = NULL;

`mr` looks unused.

>+    MemoryRegionSection mrs;

Please, can you move this declaration in the inner block where it's 
used?

>
>     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;
>+            mrs = memory_region_find(&reg->mr, *off - reg->offset, 
>len);
>+            if (!mrs.mr) {
>+                error_report("Failed to find memory region for address"
>+                             "0x%" PRIx64 "", *off);
>+                return NULL;
>+            }
>+            *off = mrs.offset_within_region;
>+            memory_region_unref(mrs.mr);
>+            return mrs.mr;
>         }
>     }
>
>     return NULL;
> }
>
>+

Unrelated change.

Thanks,
Stefano

> /* Below are generic functions to do memcpy from/to an address space,
>  * without byteswaps, with input validation.
>  *
>-- 
>2.39.3 (Apple Git-146)
>

Re: [PATCH V2 1/1] virtio-pci: Add lookup subregion of VirtIOPCIRegion MR

[Gao,Shiyuan via]

> >--- a/hw/virtio/virtio-pci.c
> >+++ b/hw/virtio/virtio-pci.c
> >@@ -610,19 +610,29 @@ static MemoryRegion *virtio_address_space_lookup(VirtIOPCIProxy *proxy,
> > {
> >     int i;
> >     VirtIOPCIRegion *reg;
> >+    MemoryRegion *mr = NULL;
>
> `mr` looks unused.
>
> >+    MemoryRegionSection mrs;
>
> Please, can you move this declaration in the inner block where it's
> used?

ok, I will move to inner block and remove unused `mr`.

>
> >
> >     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;
> >+            mrs = memory_region_find(&reg->mr, *off - reg->offset,
> >len);
> >+            if (!mrs.mr) {
> >+                error_report("Failed to find memory region for address"
> >+                             "0x%" PRIx64 "", *off);
> >+                return NULL;
> >+            }
> >+            *off = mrs.offset_within_region;
> >+            memory_region_unref(mrs.mr);
> >+            return mrs.mr;
> >         }
> >     }
> >
> >     return NULL;
> > }
> >
> >+
>
> Unrelated change.

Perhaps this would be clearer but not universal in Version 1.

Without this patch, 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.

Further more, maybe common/isr/device MR also has subregions in
the future, so need memory_region_find for each MR incluing
their subregions and this will be more universal.

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

Re: [PATCH V2 1/1] virtio-pci: Add lookup subregion of VirtIOPCIRegion MR

[Stefano Garzarella]

On Thu, Aug 29, 2024 at 01:13:43PM GMT, Gao,Shiyuan wrote:
>> >--- a/hw/virtio/virtio-pci.c
>> >+++ b/hw/virtio/virtio-pci.c
>> >@@ -610,19 +610,29 @@ static MemoryRegion *virtio_address_space_lookup(VirtIOPCIProxy *proxy,
>> > {
>> >     int i;
>> >     VirtIOPCIRegion *reg;
>> >+    MemoryRegion *mr = NULL;
>>
>> `mr` looks unused.
>>
>> >+    MemoryRegionSection mrs;
>>
>> Please, can you move this declaration in the inner block where it's
>> used?
>
>ok, I will move to inner block and remove unused `mr`.
>
>>
>> >
>> >     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;
>> >+            mrs = memory_region_find(&reg->mr, *off - reg->offset,
>> >len);
>> >+            if (!mrs.mr) {
>> >+                error_report("Failed to find memory region for address"
>> >+                             "0x%" PRIx64 "", *off);
>> >+                return NULL;
>> >+            }
>> >+            *off = mrs.offset_within_region;
>> >+            memory_region_unref(mrs.mr);
>> >+            return mrs.mr;
>> >         }
>> >     }
>> >
>> >     return NULL;
>> > }
>> >
>> >+
>>
>> Unrelated change.
>
>Perhaps this would be clearer but not universal in Version 1.
>
>Without this patch, 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.
>
>Further more, maybe common/isr/device MR also has subregions in
>the future, so need memory_region_find for each MR incluing
>their subregions and this will be more universal.

I see, I don't have much experience with this, but what you say I think
makes sense. I would wait for a comment from Michael or Jason.

Thanks,
Stefano

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

Re: [PATCH V2 1/1] virtio-pci: Add lookup subregion of VirtIOPCIRegion MR

[Michael S. Tsirkin]

On Tue, Aug 20, 2024 at 07:56:31PM +0800, Gao Shiyuan wrote:
> When VHOST_USER_PROTOCOL_F_HOST_NOTIFIER 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.
> 
> Assign the address of notify region in the modern bar above 4G, the vp_notify
> in SeaBIOS will use PCI Cfg 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 Cfg Capability, it will call virtio_address_space_write.
> 
> virtio_queue_set_host_notifier_mr add host notifier subregion of notify region
> MR, 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
> instead of notify MR.
> 
> 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 | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> ---
> 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 9534730bba..5d2d27a6a3 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -610,19 +610,29 @@ static MemoryRegion *virtio_address_space_lookup(VirtIOPCIProxy *proxy,
>  {
>      int i;
>      VirtIOPCIRegion *reg;
> +    MemoryRegion *mr = NULL;
> +    MemoryRegionSection mrs;
>  
>      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;
> +            mrs = memory_region_find(&reg->mr, *off - reg->offset, len);
> +            if (!mrs.mr) {
> +                error_report("Failed to find memory region for address"
> +                             "0x%" PRIx64 "", *off);
> +                return NULL;
> +            }


I'm not sure when can this happen. If it can't assert will do.


> +            *off = mrs.offset_within_region;
> +            memory_region_unref(mrs.mr);
> +            return mrs.mr;
>          }
>      }
>  
>      return NULL;
>  }
>  
> +
>  /* Below are generic functions to do memcpy from/to an address space,
>   * without byteswaps, with input validation.
>   *
> -- 
> 2.39.3 (Apple Git-146)