Message-ID: <pdslu36mhfxbzs254tlte2wavfkmecm53xhdtdelm4nfnemt3f@m5ed4hn6zmbl>
Date: Wed, 30 Oct 2024 15:28:19 +0100
From: Joel Granados <joel.granados@...nel.org>
To: Baolu Lu <baolu.lu@...ux.intel.com>
Cc: Yi Liu <yi.l.liu@...el.com>, David Woodhouse <dwmw2@...radead.org>, 
	Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>, 
	Robin Murphy <robin.murphy@....com>, Jason Gunthorpe <jgg@...pe.ca>, 
	Kevin Tian <kevin.tian@...el.com>, Klaus Jensen <its@...elevant.dk>, linux-kernel@...r.kernel.org, 
	iommu@...ts.linux.dev, Klaus Jensen <k.jensen@...sung.com>
Subject: Re: Re: [PATCH v4 2/5] iommu/vt-d: Remove the pasid present check in
 prq_event_thread

On Tue, Oct 29, 2024 at 11:12:49AM +0800, Baolu Lu wrote:
> On 2024/10/28 18:24, Joel Granados wrote:
> > On Mon, Oct 28, 2024 at 03:50:46PM +0800, Yi Liu wrote:
> >> On 2024/10/16 05:08, Joel Granados wrote:
> >>> From: Klaus Jensen<k.jensen@...sung.com>
> >>>
> >>> PASID is not strictly needed when handling a PRQ event; remove the check
> >>> for the pasid present bit in the request. This change was not included
> >>> in the creation of prq.c to emphasize the change in capability checks
> >>> when handing PRQ events.
> >>>
> >>> Signed-off-by: Klaus Jensen<k.jensen@...sung.com>
> >>> Reviewed-by: Kevin Tian<kevin.tian@...el.com>
> >>> Signed-off-by: Joel Granados<joel.granados@...nel.org>
> >> looks like the PRQ draining is missed for the PRI usage. When a pasid
> >> entry is destroyed, it might need to add helper similar to the
> >> intel_drain_pasid_prq() to drain PRQ for the non-pasid usage.
> > These types of user space PRIs (non-pasid, non-svm) are created by
> > making use of iommufd_hwpt_replace_device. Which adds an entry to the
> > pasid_array indexed on IOMMU_NO_PASID (0U) via the following path:
> > 
> > iommufd_hwpt_replace_device
> >    -> iommufd_fault_domain_repalce_dev
> >      -> __fault_domain_replace_dev
> >        -> iommu_replace_group_handle
>             -> __iommu_group_set_domain
>               -> intel_iommu_attach_device
>                  -> device_block_translation
>                    -> intel_pasid_tear_down_entry(IOMMU_NO_PASID)
> 
> Here a domain is removed from the pasid entry, hence we need to flush
> all page requests that are pending in the IOMMU page request queue or
> the PCI fabric.
This make a lot of sense: To use iommufd_hwpt_replace_device to replace
the existing hwpt with a iopf enabled one, the soon to be irrelevant
page requests from the existing hwpt need to be flushed. And we were not
doing that here.

> 
> >          -> xa_reserve(&group->pasid_array, IOMMU_NO_PASID, GFP_KERNEL);
> > 
> > It is my understanding that this will provide the needed relation
> > between the device and the prq in such a way that when  remove_dev_pasid
> > is called, intel_iommu_drain_pasid_prq will be called with the
> > appropriate pasid value set to IOMMU_NO_PASID. Please correct me if I'm
> > mistaken.
> 
> Removing a domain from a RID and a PASID are different paths.
> Previously, this IOMMU driver only supported page requests on PASID
> (non-IOMMU_NO_PASID). It is acceptable that it does not flush the PRQ in
> the domain-removing RID path.
> 
> With the changes made in this series, the driver now supports page
> requests for RID. It should also flush the PRQ when removing a domain
> from a PASID entry for IOMMU_NO_PASID.

Thank you for your explanation. Clarifies where I lacked understanding.

> 
> > 
> > Does this answer your question? Do you have a specific path that you are
> > looking at where a specific non-pasid drain is needed?
> 
> Perhaps we can simply add below change.
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index e860bc9439a2..a24a42649621 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4283,7 +4283,6 @@ static void intel_iommu_remove_dev_pasid(struct 
> device *dev, ioasid_t pasid,
>          intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
>          kfree(dev_pasid);
>          intel_pasid_tear_down_entry(iommu, dev, pasid, false);
> -       intel_drain_pasid_prq(dev, pasid);
>   }
> 
>   static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index 2e5fa0a23299..8639f3eb4264 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -265,6 +265,7 @@ void intel_pasid_tear_down_entry(struct intel_iommu 
> *iommu, struct device *dev,
>                  iommu->flush.flush_iotlb(iommu, did, 0, 0, 
> DMA_TLB_DSI_FLUSH);
> 
>          devtlb_invalidation_with_pasid(iommu, dev, pasid);
> +       intel_drain_pasid_prq(dev, pasid);
>   }
This make sense logically as the intel_drain_pasid_prq keeps being
called at the end of intel_iommu_remove_dev_pasid, but it is now also
included in the intel_pasid_tear_down_entry call which adds it to the
case discussed.

> 
>   /*
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 078d1e32a24e..ff88f31053d1 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -304,9 +304,6 @@ void intel_drain_pasid_prq(struct device *dev, u32 
> pasid)
>          int qdep;
> 
>          info = dev_iommu_priv_get(dev);
> -       if (WARN_ON(!info || !dev_is_pci(dev)))
> -               return;
Did you mean to take out both checks?:
  1. The info pointer check
  2. the dev_is_pci check

I can understand the dev_is_pci check, but we should definitely take
action if info is NULL. Right?

> -
>          if (!info->pri_enabled)
>                  return;
> 
> Generally, intel_drain_pasid_prq() should be called if
> 
> - a translation is removed from a pasid entry; and
This is the path that is already mentiond

> - PRI on this device is enabled.
And this path is:
  -> intel_iommu_enable_iopf
    -> context_flip_pri
      -> intel_context_flush_present
        -> qi_flush_pasid_cache

Right?

I'll put this in my next version if I see that there is a consensus in
the current discussion.

thx again for the feedback.

Best
-- 

Joel Granados

Powered by blists - more mailing lists