Linux Virtualization - Sep 2023 - [PATCH v2 1/2] iommu/virtio: Make use of ops->iotlb_sync_map

On Fri, Sep 22, 2023 at 07:07:40PM +0100, Robin Murphy wrote:
> 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.
Well it can't quite do that since it doesn't know the geometry - it
all is sort of guessing and hoping it doesn't explode on replay. If it
knows the geometry it wouldn't need finalize...
> > 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.
Except we don't detect this in the core code correctly, that is my
point. We should detect the aperture conflict, not pgsize_bitmap to
check if it is the first or second try.

Jason

On 2023-09-23 00:33, Jason Gunthorpe wrote:
> On Fri, Sep 22, 2023 at 07:07:40PM +0100, Robin Murphy wrote:
> 
>> 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.
> 
> Well it can't quite do that since it doesn't know the geometry - it
> all is sort of guessing and hoping it doesn't explode on replay. If it
> knows the geometry it wouldn't need finalize...
I think it's entirely reasonable to assume that any direct mappings 
specified for a device are valid for that device and its IOMMU. However, 
in the particular case of virtio, it really shouldn't ever have direct 
mappings anyway, since even if the underlying hardware did have any, the 
host can enforce the actual direct-mapping aspect itself, and just 
present them as unusable regions to the guest.
>>> 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.
> 
> Except we don't detect this in the core code correctly, that is my
> point. We should detect the aperture conflict, not pgsize_bitmap to
> check if it is the first or second try.
Again, that's irrelevant. It can only be about whether the actual 
->map_pages call succeeds or not. A driver could well know up-front that 
all instances support the same pgsize_bitmap and aperture, and set both 
at ->domain_alloc time, yet still be unable to handle an actual mapping 
without knowing which instance(s) that needs to interact with (e.g. 
omap-iommu).

Thanks,
Robin.