Re: [PATCH RESEND 03/10] iommu/amd: Initial SVA support for AMD IOMMU

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Sep 11, 2023 at 05:46:39PM +0530, Vasant Hegde wrote:
> Jason,
> 
> On 9/5/2023 11:44 PM, Jason Gunthorpe wrote:
> > On Tue, Sep 05, 2023 at 08:09:39PM +0530, Vasant Hegde wrote:
> >>>>   - set_dev_pasid() will check the compatibility and bind device/pasid only if
> >>>> its compatibility. In AMD case we will check against protection domain. Ex:
> >>>>  If we have two devices (devA and devB) in two different protection domain then:
> >>>>    set_dev_pasid(sva_domain, devA, pasidX) - SUCCESS
> >>>>    set_dev_pasid(sva_domain, devB, pasidX) - Compatibility check fail
> >>
> >> Here I expect it to fails as devB is in different protection domain. From
> >> invalidation point of view, devA and devB are not compatible. Hence I think it
> >> should fail binding.
> > 
> > No, that isn't the best thing to do - you could do this, but it will
> > be more inefficient.
> 
> What inefficiency are you referring? Allocating extra SVA domain -OR- mmu
> notifier getting called for each SVA domain -OR- something else?

Primarily multiple mmu notifiers is not so great

> Then I am wondering why not just create single SVA domain for a PASID and let
> individual driver handle underneath requirement like target invalidation? Am I
> missing here?

domains are not connected to PASIDs - PASID is part of the attachment.

We want drivers to support one domain per IOPTE table and allow all
possible combinations of compatible attachments.

> > We have use cases to share a KVM page table with PRI.
> > 
> > Google apparently has some usecase since they are fixing it in ARM.
> > 
> > Besides that, it is the IOMMU API, drivers have to implement it, you
> > don't get to pick and choose.
> 
> Is there any documentation on the mentioned use case and the
> required IOMMU API because this is new to me. I would like to
> understand things better before making the change in AMD IOMMU
> driver.

Just the API definitions we have been working toward and the
dicussions on the list.

It is not a complex concept, UNMANAGED paging domains need to support
PRI and PASID.

> > You are not going to get SVA without also properly doing all
> > infrastructure to enable paging and unmanaged - I won't support
> > another shortcut hackjob for SVA that needs unwinding like ARM has.
> 
> The goal of this series is to introduce the SVA support for AMD IOMMU driver.
> 
> So far we have been accommodating all the review comments.
>   - Dropping iommu_v2 module before getting SVA
>   - Reworking SVA top of Tina's series
>   - Reworking IOPF on top of Baolu's IOPF enhancement

If you would prefer to have a more stable base to work on then wait a
few months until ARM and Intel drviers are fully fixed. Otherwise you
are going to have to keep pace with the advancing work.

> Adding PASID support for the unmanaged domain is a new requirement
> and not relevant to this series.

It is not. It is what the iommu API is designed to do, ARM got
grandfathered in with their half implementation because it already
existed. New drivers must implement the API properly and not take
shortcuts.

> It will also require a considerable amount of changes
> in AMD IOMMU driver that we need to further investigate to better
> understand.

Exactly. I do not want to see new drivers follow down the path of
ARM at exctly the same time we are trying to fix ARM to work properly!
This undermines all the progress we are making! The API has been
defined, please follow it!

On ARM unwinding the bad design without breaking SVA is proving very
hard after the fact. You even say it will be hard on AMD, so NO, do it
right to start with.

Every single thing you need to implement to support SVA is also needed
to support generic PRI+PASID. The request here is to layer the code
correctly so that the PASID and PRI bits are not linked to SVA.

You don't need to *test* PRI+PASID but you do need to layer the code
correctly so that it is trivial to implement.

Jason