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