On 22/09/2023 5:27 pm, Jason Gunthorpe wrote:
On Fri, Sep 22, 2023 at 02:13:18PM +0100, Robin Murphy wrote:
On 22/09/2023 1:41 pm, Jason Gunthorpe wrote:
On Fri, Sep 22, 2023 at 08:57:19AM +0100, Jean-Philippe Brucker wrote:
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.
Where? The above points to iommu_create_device_direct_mappings() but
it doesn't because the pgsize_bitmap == 0:
__iommu_domain_alloc() sets pgsize_bitmap in this case:
/*
* If not already set, assume all sizes by default; the driver
* may override this later
*/
if (!domain->pgsize_bitmap)
domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
Dirver's shouldn't do that.
The core code was fixed to try again with mapping reserved regions to
support these kinds of drivers.
This is still the "normal" code path, really; I think it's only AMD that
started initialising the domain bitmap "early" and warranted making it
conditional.
My main point was that iommu_create_device_direct_mappings() should
fail for unfinalized domains, setting pgsize_bitmap to allow it to
succeed is not a nice hack, and not necessary now.
Sure, but it's the whole "unfinalised domains" and rewriting
domain->pgsize_bitmap after attach thing that is itself the massive
hack. AMD doesn't do that, and doesn't need to; it knows the appropriate
format at allocation time and can quite happily return a fully working
domain which allows map before attach, but the old ops->pgsize_bitmap
mechanism fundamentally doesn't work for multiple formats with different
page sizes. The only thing I'd accuse it of doing wrong is the weird
half-and-half thing of having one format as a default via one mechanism,
and the other as an override through the other, rather than setting both
explicitly.
virtio isn't setting ops->pgsize_bitmap for the sake of direct mappings
either; it sets it once it's discovered any instance, since apparently
it's assuming that all instances must support identical page sizes, and
thus once it's seen one it can work "normally" per the core code's
assumptions. It's also I think the only driver which has a "finalise"
bodge but *can* still properly support map-before-attach, by virtue of
having to replay mappings to every new endpoint anyway.
What do you think about something like this to replace
iommu_create_device_direct_mappings(), that does enforce things
properly?
I fail to see how that would make any practical difference. Either the
mappings can be correctly set up in a pagetable *before* the relevant
device is attached to that pagetable, or they can't (if the driver
doesn't have enough information to be able to do so) and we just have to
really hope nothing blows up in the race window between attaching the
device to an empty pagetable and having a second try at
iommu_create_device_direct_mappings(). That's a driver-level issue and
has nothing to do with pgsize_bitmap either way.
Thanks,
Robin.
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
