On Tue, 14 Nov 2023, Juergen Gross wrote: > On 13.11.23 21:24, David Woodhouse wrote: > > On Fri, 2023-10-27 at 07:27 +0200, Juergen Gross wrote: > > > On 26.10.23 22:56, Stefano Stabellini wrote: > > > > On Thu, 26 Oct 2023, David Woodhouse wrote: > > > > > On Thu, 2023-10-26 at 13:36 -0700, Stefano Stabellini wrote: > > > > > > > > > > > > > This seems like a lot of code to replace that simpler option... is > > > > > > > there a massive performance win from doing it this way? Would we > > > > > > > want > > > > > > > to use this trick for the Xen PV backends (qdisk, qnic) *too*? > > > > > > > Might it > > > > > > > make sense to introduce the simple version and *then* the > > > > > > > optimisation, > > > > > > > with some clear benchmarking to show the win? > > > > > > > > > > > > This is not done for performance but for safety (as in safety > > > > > > certifications, ISO 26262, etc.). This is to enable unprivileged > > > > > > virtio > > > > > > backends running in a DomU. By unprivileged I mean a virtio backend > > > > > > that > > > > > > is unable to map arbitrary memory (the xenforeignmemory interface is > > > > > > prohibited). > > > > > > > > > > > > The goal is to run Xen on safety-critical systems such as cars, > > > > > > industrial robots and more. In this configuration there is no > > > > > > traditional Dom0 in the system at all. If you would like to know > > > > > > more: > > > > > > https://www.youtube.com/watch?v=tisljY6Bqv0&list=PLYyw7IQjL-zHtpYtMpFR3KYdRn0rcp5Xn&index=8 > > > > > > > > > > Yeah, I understand why we're using grant mappings instead of just > > > > > directly having access via foreignmem mappings. That wasn't what I was > > > > > confused about. > > > > > > > > > > What I haven't worked out is why we're implementing this through an > > > > > automatically-populated MemoryRegion in QEMU, rather than just using > > > > > grant mapping ops like we always have. > > > > > > > > > > It seems like a lot of complexity just to avoid calling > > > > > qemu_xen_gnttab_map_refs() from the virtio backend. > > > > > > > > I think there are two questions here. One question is "Why do we need > > > > all the new grant mapping code added to xen-mapcache.c in patch #7? > > > > Can't we use qemu_xen_gnttab_map_refs() instead?" > > > > > > The main motivation was to _avoid_ having to change all the backends. > > > > > > My implementation enables _all_ qemu based virtio backends to use grant > > > mappings. And if a new backend is added to qemu, there will be no change > > > required to make it work with grants. > > > > I'm not really convinced I buy that. This is a lot of complexity, and > > don't backends need to call an appropriate mapping function to map via > > an IOMMU if it's present anyway? Make then call a helper where you can > > do this in one place directly instead of through a fake MemoryRegion, > > and you're done, surely? > > That was tested with unmodified block and net backends in qemu. > > Maybe I missed something, but I think the IOMMU accesses are _not_ covering > accesses to the virtio rings from qemu. And this is something you really > want for driver domains.
Hi Juergen, David, I don't think we should use the IOMMU accesses and I don't think we should change the virtio backends. My preference would be to either use the patch as is, or (better) to reuse the original Xen hooks already in place (qemu_ram_ptr_length and xen_invalidate_map_cache_entry, see https://marc.info/?l=qemu-devel&m=169828434927778). Juergen did a good job at adding new hooks in a clean way, but from my point of view I would still prefer to only have the two existing hooks instead of having 4 (2 old, 2 new).
