Message-ID: <BN9PR11MB527694446B401EF9761529738C729@BN9PR11MB5276.namprd11.prod.outlook.com>
Date:   Sat, 11 Dec 2021 03:57:45 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Jason Gunthorpe <jgg@...dia.com>
CC:     "peter.maydell@...aro.org" <peter.maydell@...aro.org>,
        "lushenming@...wei.com" <lushenming@...wei.com>,
        "robin.murphy@....com" <robin.murphy@....com>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        Jean-Philippe Brucker <jean-philippe@...aro.org>,
        "maz@...nel.org" <maz@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "vsethi@...dia.com" <vsethi@...dia.com>,
        "alex.williamson@...hat.com" <alex.williamson@...hat.com>,
        "wangxingang5@...wei.com" <wangxingang5@...wei.com>,
        "vivek.gautam@....com" <vivek.gautam@....com>,
        "zhangfei.gao@...aro.org" <zhangfei.gao@...aro.org>,
        "eric.auger.pro@...il.com" <eric.auger.pro@...il.com>,
        "will@...nel.org" <will@...nel.org>,
        "kvmarm@...ts.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>
Subject: RE: [RFC v16 1/9] iommu: Introduce attach/detach_pasid_table API

> From: Jason Gunthorpe <jgg@...dia.com>
> Sent: Friday, December 10, 2021 9:23 PM
> 
> On Fri, Dec 10, 2021 at 08:56:56AM +0000, Tian, Kevin wrote:
> > > So, something like vfio pci would implement three uAPI operations:
> > >  - Attach page table to RID
> > >  - Attach page table to PASID
> > >  - Attach page table to RID and all PASIDs
> > >    And here 'page table' is everything below the STE in SMMUv3
> > >
> > > While mdev can only support:
> > >  - Access emulated page table
> > >  - Attach page table to PASID
> >
> > mdev is a pci device from user p.o.v, having its vRID and vPASID. From
> > this angle the uAPI is no different from vfio-pci (except the ARM one):
> 
> No, it isn't. The internal operation is completely different, and it
> must call different iommufd APIs than vfio-pci does.

Well, you mentioned "uAPI operations" thus my earlier comment 
is purely from uAPI p.o.v instead of internal iommufd APIs (not meant
I didn't think of them). I think this is the main divergence in this 
discussion as when I saw you said "while mdev can only support" 
I assume it's still about uAPI (more specifically VFIO uAPI as it carries 
the attach call to iommufd).

> 
> This is user visible - mdev can never be attached to an ARM user page
> table, for instance.

sure. the iommu driver will fail the attach request when seeing
incompatible way is used.

> 
> For iommufd there is no vRID, vPASID or any confusing stuff like
> that. You'll have an easier time if you stop thinking in these terms.

I don't have a difficulty here as from vfio uAPI p.o.v it's about
vRID and vPASID. But there is NO any confusion on iommufd which
should only deal with physical thing. This has been settled down
long time ago in high level design discussion. 😊

> 
> We probably end up with three iommufd calls:
>  int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id,
> unsigned int flags)
>  int iommufd_device_attach_pasid(struct iommufd_device *idev, u32 *pt_id,
> unsigned int flags, ioasid_t *pasid)
>  int iommufd_device_attach_sw_iommu(struct iommufd_device *idev, u32
> pt_id);

this is aligned with previous design.

> 
> And the uAPI from VFIO must map onto them.
> 
> vfio-pci:
>   - 'VFIO_SET_CONTAINER' does
>     iommufd_device_attach(idev, &pt_id, IOMMUFD_FULL_DEVICE);
>     # IOMMU specific if this captures PASIDs or cause them to fail,
>     # but IOMMUFD_FULL_DEVICE will prevent attaching any PASID
>     # later on all iommu's.
> 
> vfio-mdev:
>   - 'VFIO_SET_CONTAINER' does one of:
>     iommufd_device_attach_pasid(idev, &pt_id, IOMMUFD_ASSIGN_PASID,
> &pasid);
>     iommufd_device_attach_sw_iommu(idev, pt_id);
> 
> That is three of the cases.
> 
> Then we have new ioctls for the other cases:
> 
> vfio-pci:
>   - 'bind only the RID, so we can use PASID'
>     iommufd_device_attach(idev, &pt_id, 0);
>   - 'bind to a specific PASID'
>     iommufd_device_attach_pasid(idev, &pt_id, 0, &pasid);
> 
> vfio-mdev:
>   - 'like VFIO_SET_CONTAINER but bind to a specific PASID'
>     iommufd_device_attach_pasid(idev, &pt_id, 0, &pasid);
> 
> The iommu driver will block attachments that are incompatible, ie ARM
> user page tables only work with:
>  iommufd_device_attach(idev, &pt_id, IOMMUFD_FULL_DEVICE)
> all other calls fail.

Above are all good except the FULL_DEVICE thing.

This might be the only open as I still didn't see why we need an
explicit flag to claim a 'full device' thing. From kernel p.o.v the
ARM case is no different from Intel that both allows an user
page table attached to vRID, just with different format and
addr width (Intel is 64bit, ARM is 84bit where PASID can be
considered a sub-handle in the 84bit address space and not
the kernel's business).

and ARM doesn't support explicit PASID attach then those calls
will fail for sure.

> 
> How exactly we put all of this into new ioctls, I'm not sure, but it
> does seem pretty clear this is what the iommufd kAPI will need to look
> like to cover the cases we know about already.
> 
> As you can see, userpace needs to understand what mode it is operating
> in. If it does IOMMUFD_FULL_DEVICE and manages PASID somehow in
> userspace, or it doesn't and can use the iommufd_device_attach_pasid()
> paths.
> 
> > Is modeling like above considered ambiguous?
> 
> You've skipped straight to the ioctls without designing the kernel API
> to meet all the requirements  :)
> 

No problem on this. Just we focus on different matter in this discussion.
As I replied I think the only open is whether ARM thing needs to be
specialized via a new ioctl or flag. Otherwise all other things are aligned. 

Thanks
Kevin

Powered by blists - more mailing lists