On Tue, Mar 07, 2023 at 02:42:23AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Sent: Tuesday, March 7, 2023 4:45 AM
> >
> > On Thu, Mar 02, 2023 at 08:20:05AM +0000, Tian, Kevin wrote:
> > > > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > > Sent: Saturday, February 25, 2023 8:28 AM
> > > >
> > > > +static struct iommufd_hw_pagetable *
> > > > +iommufd_device_do_replace_locked(struct iommufd_device *idev,
> > > > + struct iommufd_hw_pagetable *hwpt)
> > > > +{
> > > > + struct iommufd_hw_pagetable *old_hwpt;
> > > > + int rc;
> > > > +
> > > > + lockdep_assert_held(&idev->igroup->lock);
> > > > +
> > > > + /* Try to upgrade the domain we have */
> > > > + if (idev->enforce_cache_coherency) {
> > > > + rc = iommufd_hw_pagetable_enforce_cc(hwpt);
> > > > + if (rc)
> > > > + return ERR_PTR(-EINVAL);
> > > > + }
> > > > +
> > > > + rc = iommufd_device_setup_msi(idev, hwpt);
> > > > + if (rc)
> > > > + return ERR_PTR(rc);
> > > > +
> > > > + old_hwpt = idev->igroup->hwpt;
> > > > + if (hwpt->ioas != old_hwpt->ioas) {
> > > > + rc = iopt_table_enforce_group_resv_regions(
> > > > + &hwpt->ioas->iopt, idev->igroup->group, NULL);
> > > > + if (rc)
> > > > + return ERR_PTR(rc);
> > > > + }
> > >
> > > This is inconsistent with the earlier cleanup in the attach path
> > > where setup_msi/enforce_group_resv_region are done only
> > > once per group (if that is the right thing to do).
> >
> > Logically replace is 'detach every device in the group' - which makes
> > devices 0 - then 'reattach them all' to the new ioas.
> >
> > So at this point it is still being done only once per group.
> >
> > The 2nd idevs to call this function will see hwpt->ioas ==
> > old_hwpt->ioas
> >
>
> but setup_msi() is still done for every device which is inconsistent
> with what patch5 does.
There is a missing check to do nothing if the hwpt is already set
If the hwpt is not set and the ioas is the same then we still have to
do the setup_msi
Jason
