On Wed, Apr 17, 2024 at 08:44:11AM +0000, Tian, Kevin wrote:
> > From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> > Sent: Friday, April 12, 2024 4:15 PM
> >
> > @@ -3332,11 +3333,34 @@ static int __iommu_set_group_pasid(struct
> > iommu_domain *domain,
> > err_revert:
> > last_gdev = device;
> > for_each_group_device(group, device) {
> > - const struct iommu_ops *ops = dev_iommu_ops(device-
> > >dev);
> > + /*
> > + * If no old domain, just undo all the devices/pasid that
> > + * have attached to the new domain.
> > + */
> > + if (!old) {
> > + const struct iommu_ops *ops =
> > + dev_iommu_ops(device-
> > >dev);
> > +
> > + if (device == last_gdev)
> > + break;
> > + ops = dev_iommu_ops(device->dev);
>
> 'ops' is already assigned
>
> > + ops->remove_dev_pasid(device->dev, pasid, domain);
> > + continue;
> > + }
> >
> > - if (device == last_gdev)
> > + /*
> > + * Rollback the devices/pasid that have attached to the new
> > + * domain. And it is a driver bug to fail attaching with a
> > + * previously good domain.
> > + */
> > + if (device == last_gdev) {
> > + WARN_ON(old->ops->set_dev_pasid(old, device-
> > >dev,
> > + pasid, NULL));
>
> do we have a clear definition that @set_dev_pasid callback should
> leave the device detached (as 'NULL' indicates) or we just don't
> care the currently-attached domain at this point?
If set_dev_pasid fails I would expect to to have done nothing so the
failing device should be left in the old config and we should just not
call it at all.
The RID path is wonky here because so many drivers don't do that, so
we poke them again to hopefully get it right. I think for PASID we
should try to make the drivers work properly from the start. Failure
means no change.
I would summarize this in a comment..
Jason
