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

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.

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