> From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Wednesday, March 8, 2023 10:08 AM
>
> On 3/7/23 8:46 PM, Jason Gunthorpe wrote:
> > On Tue, Mar 07, 2023 at 08:42:06AM +0000, Tian, Kevin wrote:
> >>> From: Jason Gunthorpe<jgg@xxxxxxxxxx>
> >>> Sent: Saturday, February 25, 2023 8:28 AM
> >>>
> >> [...]
> >>> The implementation is complicated because we have to introduce some
> >>> per-iommu_group memory in iommufd and redo how we think about
> multi-
> >>> device
> >>> groups to be more explicit. This solves all the locking problems in the
> >>> prior attempts.
> >>>
> >> Now think about the pasid case.
> >>
> >> pasid attach is managed as a device operation today:
> >> iommu_attach_device_pasid()
> >>
> >> Following it naturally we'll have a pasid array per iommufd_device to
> >> track attached HWPT per pasid.
> >>
> >> But internally there is only one pasid table per iommu group. i.e. same
> >> story as RID attach that once dev1 replaces hwpt on pasid1 then it takes
> >> effect on all other devices in the same group.
> > IMHO I can't belive that any actual systems that support PASID have a
> > RID aliasing problem too.
> >
> > I think we should fix the iommu core to make PASID per-device and
> > require systems that have a RID aliasing problem to block PASID.
> >
> > This is a bigger picture, if drivers have to optionally share their
> > PASID tables with other drivers then we can't have per-driver PASID
> > allocators at all either.
>
> This is actually required in PCI and IOMMU core. pci_enable_pasid()
> requires full ACS support on device's upstream path:
>
> if (!pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR | PCI_ACS_UF))
> return -EINVAL;
>
> and, for such PCI topology, iommu core always allocates an exclusive
> iommu group.
>
> The only place where seems to be a little messy is,
>
> static int __iommu_set_group_pasid(struct iommu_domain *domain,
> struct iommu_group *group, ioasid_t
> pasid)
> {
> struct group_device *device;
> int ret = 0;
>
> list_for_each_entry(device, &group->devices, list) {
> ret = domain->ops->set_dev_pasid(domain, device->dev,
> pasid);
> if (ret)
> break;
> }
>
> return ret;
> }
>
yes, this is exactly where my confusion comes. I thought we ever agreed
that PASID usage won't be supported on multi-devices group. Then I saw
about code (but didn't catch the implication of pci acs) which led me to
think about the group mess.
sticking to per-device pasid would certainly simplify the logic in iommufd. 😊
> Perhaps we need a check on singleton group?
>
or move the xarray to device? Storing it in group while adding a singleton
restriction only causes confusion.
