| Message ID | 20240820115631.52522-1-gaoshiyuan@baidu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2,1/1] virtio-pci: Add lookup subregion of VirtIOPCIRegion MR | expand |
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 ®->mr; >+ mrs = memory_region_find(®->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) >
> >--- 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 ®->mr; > >+ mrs = memory_region_find(®->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 ®->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; } }
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 ®->mr; >> >+ mrs = memory_region_find(®->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 ®->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; > } > } >
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 ®->mr; > + mrs = memory_region_find(®->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)
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 ®->mr; + mrs = memory_region_find(®->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. *
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.