Re: [PATCH 1/4] iommu/vt-d: Implement set device pasid op for default domain

[Baolu Lu]

On 3/3/23 11:02 AM, Tian, Kevin wrote:
> > From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Friday, March 3, 2023 10:49 AM
> >
> > On 3/3/23 10:36 AM, Tian, Kevin wrote:
> > > > From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> > > > Sent: Thursday, March 2, 2023 10:07 PM
> > > > > +
> > > > > +	if (!sm_supported(iommu) || !info)
> > > >
> > > > @info has been referenced. !info check makes no sense.
> > > >
> > > > Add pasid_supported(iommu).
> > > >
> > > > Do you need to check whether the domain is compatible for this rid
> > > > pasid?
> > >
> > > what kind of compatibility is concerned here? In concept a pasid
> > > can be attached to any domain if it has been successfully attached
> > > to rid. Probably we can add a check here that RID2PASID must
> > > point to the domain already.
> >
> > "...if it has been successfully attached to rid..."
> >
> > We should not have this assumption in iommu driver's callback. The iommu
> > driver has no (and should not have) knowledge about the history of any
> > domain.
>
> but this is an op for default domain which must have been attached
> to RID2PASID and any compatibility check between this domain and device
> should be passed.

This is an op for DMA, DMA-FQ and UNMANAGED domain. The IOMMU driver
doesn't need to interpret the default domain concept. :-)

> We can have another set_pasid for unmanaged which then need similar
> check as prepare_domain_attach_device() does.

From the perspective of the iommu driver, there's no essential
difference between DMA and UNMANAGED domains. So almost all IOMMU
drivers maintain a single set of domain ops for them.

> > > > > +		return -ENODEV;
> > > > > +
> > > > > +	if (WARN_ON(pasid == PASID_RID2PASID))
> > > > > +		return -EINVAL;
> > > >
> > > > Add a call to domain_attach_iommu() here to get a refcount of the
> > domain
> > > > ID. And call domain_detach_iommu() in
> > intel_iommu_remove_dev_pasid().
> > > >
> > >
> > > Is it necessary? iommu core doesn't allow taking ownership
> > > if !xa_empty(&group->pasid_array) so if this pasid attach succeeds
> > > this device cannot be attached to another domain before pasid
> > > detach is done on the current domain.
> >
> > It's not about the pasid, but the domain id.
> >
> > This domain's id will be set to a field of the device's pasid entry. It
> > must get a refcount of that domain id to avoid use after free.
>
> If the domain still has attached device (due to this pasid usage) how could
> domain id be freed?

The Intel IOMMU driver uses a user counter to determine when the domain
id could be freed.

Best regards,
baolu