On Tue, Sep 19, 2023 at 09:28:08AM +0100, Robin Murphy wrote: > On 2023-09-19 09:15, Jean-Philippe Brucker wrote: > > On Mon, Sep 18, 2023 at 05:37:47PM +0100, Robin Murphy wrote: > > > > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c > > > > index 17dcd826f5c2..3649586f0e5c 100644 > > > > --- a/drivers/iommu/virtio-iommu.c > > > > +++ b/drivers/iommu/virtio-iommu.c > > > > @@ -189,6 +189,12 @@ static int viommu_sync_req(struct viommu_dev *viommu) > > > > int ret; > > > > unsigned long flags; > > > > + /* > > > > + * .iotlb_sync_map and .flush_iotlb_all may be called before the viommu > > > > + * is initialized e.g. via iommu_create_device_direct_mappings() > > > > + */ > > > > + if (!viommu) > > > > + return 0; > > > > > > Minor nit: I'd be inclined to make that check explicitly in the places where > > > it definitely is expected, rather than allowing *any* sync to silently do > > > nothing if called incorrectly. Plus then they could use > > > vdomain->nr_endpoints for consistency with the equivalent checks elsewhere > > > (it did take me a moment to figure out how we could get to .iotlb_sync_map > > > with a NULL viommu without viommu_map_pages() blowing up first...) > > > > They're not strictly equivalent: this check works around a temporary issue > > with the IOMMU core, which calls map/unmap before the domain is finalized. > > Once we merge domain_alloc() and finalize(), then this check disappears, > > but we still need to test nr_endpoints in map/unmap to handle detached > > domains (and we still need to fix the synchronization of nr_endpoints > > against attach/detach). That's why I preferred doing this on viommu and > > keeping it in one place. > > Fair enough - it just seems to me that in both cases it's a detached domain, > so its previous history of whether it's ever been otherwise or not shouldn't > matter. Even once viommu is initialised, does it really make sense to send > sync commands for a mapping on a detached domain where we haven't actually > sent any map/unmap commands? If no requests were added by map/unmap, then viommu_sync_req() is essentially a nop because virtio doesn't use sync commands (and virtqueue_kick() only kicks the host when the queue's not empty, if I remember correctly). It still does a bit of work so is less efficient than a preliminary check on nr_endpoints, but it feels nicer to streamline the case where the domain is attached, since map/unmap on detached domains happens rarely, if ever. Either is fine by me. An extra test won't make much difference performance wise, and I guess will look less confusing. Niklas, do you mind resending the version with nr_endpoints check (and updated commit messages)? Thanks, Jean _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
