Re: [PATCH v5 6/7] iommu/vt-d: Implement set_dev_pasid domain op

[Baolu Lu]

On 5/5/23 7:03 AM, Jacob Pan wrote:
> > > +static int intel_iommu_attach_device_pasid(struct iommu_domain *domain,
> > > +					   struct device *dev,
> > > ioasid_t pasid) +{
> > > +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> > > +	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> > > +	struct intel_iommu *iommu = info->iommu;
> > > +	int ret;
> > > +
> > > +	if (!pasid_supported(iommu))
> > > +		return -ENODEV;
> > > +
> > > +	ret = prepare_domain_attach_device(domain, dev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * Most likely the device context has already been set up,
> > > will only
> > > +	 * take a domain ID reference. Otherwise, device context will
> > > be set
> > > +	 * up here.
> > The "otherwise" case is only default domain deferred attaching case,
> > right?
> it might be the only case so far, but my intention is to be general. i.e.
> no ordering requirements. I believe it is more future proof in case
> device_attach_pasid called before device_attach.

Let's put aside deferred attach and talk about it later.

The device's context entry is configured when the default domain is
being attached to the device. And, the default domain attaching is in
the iommu probe_device path. It always happens before set_device_pasid
which is designed to be called by the device driver. So the real
situation is that when the device driver calls set_device_pasid, the
context entry should already have been configured.

Then let's pick up the deferred attach case. It is a workaround for
kdump (Documentation/admin-guide/kdump/kdump.rst). I don't think PASID
feature is functionally required by any kdump capture kernel as its
main purpose is to dump the memory of a panic kernel.

In summary, it seems to be reasonable for the vt-d driver to return
-EBUSY when a device's context was copied. The idxd driver should
continue to work without kernel DMA with PASID support.

	if (context_copied(iommu, bus, devfn))
		return -EBUSY;

Make things general is always good, but this doesn't mean that we need
to make the code complex to support cases that do not exist or are not
used. Thoughts?

Best regards,
baolu