From: Klaus Jensen <k.jensen@samsung.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@samsung.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Joel Granados <joel.granados@kernel.org>
---
drivers/iommu/intel/prq.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/intel/prq.c b/drivers/iommu/intel/prq.c
index d4f18eb46475..3c50c848893f 100644
--- a/drivers/iommu/intel/prq.c
+++ b/drivers/iommu/intel/prq.c
@@ -223,18 +223,12 @@ static irqreturn_t prq_event_thread(int irq, void *d)
req = &iommu->prq[head / sizeof(*req)];
address = (u64)req->addr << VTD_PAGE_SHIFT;
- if (unlikely(!req->pasid_present)) {
- pr_err("IOMMU: %s: Page request without PASID\n",
- iommu->name);
-bad_req:
- handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
- goto prq_advance;
- }
-
if (unlikely(!is_canonical_address(address))) {
pr_err("IOMMU: %s: Address is not canonical\n",
iommu->name);
- goto bad_req;
+bad_req:
+ handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
+ goto prq_advance;
}
if (unlikely(req->pm_req && (req->rd_req | req->wr_req))) {
--
2.43.0On 2024/10/16 05:08, Joel Granados wrote:
> From: Klaus Jensen <k.jensen@samsung.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@samsung.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Joel Granados <joel.granados@kernel.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.
> ---
> drivers/iommu/intel/prq.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iommu/intel/prq.c b/drivers/iommu/intel/prq.c
> index d4f18eb46475..3c50c848893f 100644
> --- a/drivers/iommu/intel/prq.c
> +++ b/drivers/iommu/intel/prq.c
> @@ -223,18 +223,12 @@ static irqreturn_t prq_event_thread(int irq, void *d)
> req = &iommu->prq[head / sizeof(*req)];
> address = (u64)req->addr << VTD_PAGE_SHIFT;
>
> - if (unlikely(!req->pasid_present)) {
> - pr_err("IOMMU: %s: Page request without PASID\n",
> - iommu->name);
> -bad_req:
> - handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
> - goto prq_advance;
> - }
> -
> if (unlikely(!is_canonical_address(address))) {
> pr_err("IOMMU: %s: Address is not canonical\n",
> iommu->name);
> - goto bad_req;
> +bad_req:
> + handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
> + goto prq_advance;
> }
>
> if (unlikely(req->pm_req && (req->rd_req | req->wr_req))) {
>
--
Regards,
Yi Liu
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@samsung.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@samsung.com>
> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> > Signed-off-by: Joel Granados <joel.granados@kernel.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
-> 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.
Does this answer your question? Do you have a specific path that you are
looking at where a specific non-pasid drain is needed?
Best
>
> > ---
> > drivers/iommu/intel/prq.c | 12 +++---------
> > 1 file changed, 3 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/iommu/intel/prq.c b/drivers/iommu/intel/prq.c
> > index d4f18eb46475..3c50c848893f 100644
> > --- a/drivers/iommu/intel/prq.c
> > +++ b/drivers/iommu/intel/prq.c
> > @@ -223,18 +223,12 @@ static irqreturn_t prq_event_thread(int irq, void *d)
> > req = &iommu->prq[head / sizeof(*req)];
> > address = (u64)req->addr << VTD_PAGE_SHIFT;
> >
> > - if (unlikely(!req->pasid_present)) {
> > - pr_err("IOMMU: %s: Page request without PASID\n",
> > - iommu->name);
> > -bad_req:
> > - handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
> > - goto prq_advance;
> > - }
> > -
> > if (unlikely(!is_canonical_address(address))) {
> > pr_err("IOMMU: %s: Address is not canonical\n",
> > iommu->name);
> > - goto bad_req;
> > +bad_req:
> > + handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
> > + goto prq_advance;
> > }
> >
> > if (unlikely(req->pm_req && (req->rd_req | req->wr_req))) {
> >
>
> --
> Regards,
> Yi Liu
--
Joel Granados
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@samsung.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@samsung.com>
>>> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
>>> Signed-off-by: Joel Granados<joel.granados@kernel.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.
> -> 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.
>
> 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);
}
/*
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;
-
if (!info->pri_enabled)
return;
Generally, intel_drain_pasid_prq() should be called if
- a translation is removed from a pasid entry; and
- PRI on this device is enabled.
Thought?
--
baolu
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@samsung.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@samsung.com>
> >>> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> >>> Signed-off-by: Joel Granados<joel.granados@kernel.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
On 2024/10/30 22:28, Joel Granados wrote: > 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@samsung.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@samsung.com> >>>>> Reviewed-by: Kevin Tian<kevin.tian@intel.com> >>>>> Signed-off-by: Joel Granados<joel.granados@kernel.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. I post a patch to address what we are discussing here, so that you don't need to send a new version. https://lore.kernel.org/linux-iommu/20241031095139.44220-1-baolu.lu@linux.intel.com/ -- baolu
On Thu, Oct 31, 2024 at 05:57:01PM +0800, Baolu Lu wrote: > On 2024/10/30 22:28, Joel Granados wrote: > > 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@samsung.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@samsung.com> > >>>>> Reviewed-by: Kevin Tian<kevin.tian@intel.com> > >>>>> Signed-off-by: Joel Granados<joel.granados@kernel.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. > > I post a patch to address what we are discussing here, so that you don't > need to send a new version. > > https://lore.kernel.org/linux-iommu/20241031095139.44220-1-baolu.lu@linux.intel.com/ Thx for that :). A few comments: 1. I see that you have correctly changed the intel/prq.c file. This means that that patch depends on this series. Would it be easier (for upstreaming) to just put them together? I can take your patch into the series leaving you as the author. Tell me what you think. 2. I see the mail in the list and I see that I'm cced, but I have not received it in my mail box yet. I'll wait for it to arrive to see if my comments still apply to that one Best > > -- > baolu -- Joel Granados
On 2024/10/31 19:18, Joel Granados wrote: > On Thu, Oct 31, 2024 at 05:57:01PM +0800, Baolu Lu wrote: >> On 2024/10/30 22:28, Joel Granados wrote: >>> 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@samsung.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@samsung.com> >>>>>>> Reviewed-by: Kevin Tian<kevin.tian@intel.com> >>>>>>> Signed-off-by: Joel Granados<joel.granados@kernel.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. >> I post a patch to address what we are discussing here, so that you don't >> need to send a new version. >> >> https://lore.kernel.org/linux-iommu/20241031095139.44220-1- >> baolu.lu@linux.intel.com/ > Thx for that 🙂. A few comments: > > 1. I see that you have correctly changed the intel/prq.c file. This > means that that patch depends on this series. Would it be easier (for > upstreaming) to just put them together? I can take your patch into > the series leaving you as the author. Tell me what you think. > > 2. I see the mail in the list and I see that I'm cced, but I have not > received it in my mail box yet. I'll wait for it to arrive to see if > my comments still apply to that one No need to put them together and resend a new version if there's no further comment. Then, I'll queue them together for v6.13 through the iommu tree. -- baolu
On 10/30/24 22:28, Joel Granados wrote: >> /* >> 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? WARN_ON(!info) is duplicate as far as I can see. Accessing info->pri_enable when info is NULL will cause a null pointer dereference warning. This appears irrelevant to this patch though. > >> - >> 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? Sorry that I didn't make it clear. It should be "PRI on this device was enabled", a.k.a. info->pri_enabled is true. I didn't meant to say in the PRI enabling path. -- baolu
On 2024/10/29 11:12, 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@samsung.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@samsung.com> >>>> Reviewed-by: Kevin Tian<kevin.tian@intel.com> >>>> Signed-off-by: Joel Granados<joel.granados@kernel.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. > >> -> 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. > >> >> 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); > } > > /* > 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; > - > if (!info->pri_enabled) > return; > > Generally, intel_drain_pasid_prq() should be called if > > - a translation is removed from a pasid entry; and > - PRI on this device is enabled. If the @pasid==IOMMU_NO_PASID, PRQ drain should use the iotlb invalidation and dev-tlb invalidation descriptors. So extra code change is needed in intel_drain_pasid_prq(). Or perhaps it's better to have a separate helper for draining prq for non-pasid case. -- Regards, Yi Liu
On 2024/10/29 13:13, Yi Liu wrote: > On 2024/10/29 11:12, 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@samsung.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@samsung.com> >>>>> Reviewed-by: Kevin Tian<kevin.tian@intel.com> >>>>> Signed-off-by: Joel Granados<joel.granados@kernel.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. >> >>> -> 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. >> >>> >>> 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); >> } >> >> /* >> 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; >> - >> if (!info->pri_enabled) >> return; >> >> Generally, intel_drain_pasid_prq() should be called if >> >> - a translation is removed from a pasid entry; and >> - PRI on this device is enabled. > > If the @pasid==IOMMU_NO_PASID, PRQ drain should use the iotlb invalidation > and dev-tlb invalidation descriptors. So extra code change is needed in > intel_drain_pasid_prq(). Or perhaps it's better to have a separate helper > for draining prq for non-pasid case. According to VT-d spec, section 7.10, "Software Steps to Drain Page Requests & Responses", we can simply replace p_iotlb_inv_dsc and p_dev_tlb_inv_dsc with iotlb_inv_dsc and dev_tlb_inv_dsc. Any significant negative performance impact? -- baolu
On 2024/10/29 13:39, Baolu Lu wrote: > On 2024/10/29 13:13, Yi Liu wrote: >> On 2024/10/29 11:12, 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@samsung.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@samsung.com> >>>>>> Reviewed-by: Kevin Tian<kevin.tian@intel.com> >>>>>> Signed-off-by: Joel Granados<joel.granados@kernel.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. >>> >>>> -> 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. >>> >>>> >>>> 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); >>> } >>> >>> /* >>> 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; >>> - >>> if (!info->pri_enabled) >>> return; >>> >>> Generally, intel_drain_pasid_prq() should be called if >>> >>> - a translation is removed from a pasid entry; and >>> - PRI on this device is enabled. >> >> If the @pasid==IOMMU_NO_PASID, PRQ drain should use the iotlb invalidation >> and dev-tlb invalidation descriptors. So extra code change is needed in >> intel_drain_pasid_prq(). Or perhaps it's better to have a separate helper >> for draining prq for non-pasid case. > > According to VT-d spec, section 7.10, "Software Steps to Drain Page > Requests & Responses", we can simply replace p_iotlb_inv_dsc and > p_dev_tlb_inv_dsc with iotlb_inv_dsc and dev_tlb_inv_dsc. Any > significant negative performance impact? It's not about performance impact. My point is to use iotlb_inv_dsc and dev_tlb_inv_dsc for the @pasid==IOMMU_NO_PASID case. The existing intel_drain_pasid_prq() only uses p_iotlb_inv_dsc and p_dev_tlb_inv_dsc. The way you described in above reply works. But it needs to add if/else to use the correct invalidation descriptor. Since the descriptor composition has several lines, so just an ask if it's better to have a separate helper. :) -- Regards, Yi Liu
On 2024/10/30 13:51, Yi Liu wrote: > On 2024/10/29 13:39, Baolu Lu wrote: >> On 2024/10/29 13:13, Yi Liu wrote: >>> On 2024/10/29 11:12, 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@samsung.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@samsung.com> >>>>>>> Reviewed-by: Kevin Tian<kevin.tian@intel.com> >>>>>>> Signed-off-by: Joel Granados<joel.granados@kernel.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. >>>> >>>>> -> 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. >>>> >>>>> >>>>> 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); >>>> } >>>> >>>> /* >>>> 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; >>>> - >>>> if (!info->pri_enabled) >>>> return; >>>> >>>> Generally, intel_drain_pasid_prq() should be called if >>>> >>>> - a translation is removed from a pasid entry; and >>>> - PRI on this device is enabled. >>> >>> If the @pasid==IOMMU_NO_PASID, PRQ drain should use the iotlb >>> invalidation >>> and dev-tlb invalidation descriptors. So extra code change is needed in >>> intel_drain_pasid_prq(). Or perhaps it's better to have a separate >>> helper >>> for draining prq for non-pasid case. >> >> According to VT-d spec, section 7.10, "Software Steps to Drain Page >> Requests & Responses", we can simply replace p_iotlb_inv_dsc and >> p_dev_tlb_inv_dsc with iotlb_inv_dsc and dev_tlb_inv_dsc. Any >> significant negative performance impact? > > It's not about performance impact. My point is to use iotlb_inv_dsc and > dev_tlb_inv_dsc for the @pasid==IOMMU_NO_PASID case. The existing > intel_drain_pasid_prq() only uses p_iotlb_inv_dsc and p_dev_tlb_inv_dsc. > The way you described in above reply works. But it needs to add if/else > to use the correct invalidation descriptor. Since the descriptor > composition has several lines, so just an ask if it's better to have a > separate helper. :) The spec says (7.10 Software Steps to Drain Page Requests & Responses): " Submit an IOTLB invalidate descriptor (iotlb_inv_dsc or p_iotlb_inv_dsc) followed by DeviceTLB invalidation descriptor (dev_tlb_inv_dsc or p_dev_tlb_inv_dsc) targeting the endpoint device. These invalidation requests can be of any granularity. Per the ordering requirements described in Section 7.8, older page group responses issued by software to the endpoint device before step (a) are guaranteed to be received by the endpoint before the endpoint receives this Device-TLB invalidation request. " The purpose of the cache invalidation requests sent to the device is to leverage PCI ordering requirements to ensure that all page group responses are received by the device before it processes the TLB invalidation request. Therefore, the specification doesn't mandate the type and granularity of invalidation requests, as long as they are valid. Given that the Page Request Interface (PRI) is only supported when the IOMMU operates in scalable mode, I don't believe we need to make any changes to the invalidation types at this time. -- baolu
On 2024/10/28 15:50, Yi Liu wrote: > On 2024/10/16 05:08, Joel Granados wrote: >> From: Klaus Jensen <k.jensen@samsung.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@samsung.com> >> Reviewed-by: Kevin Tian <kevin.tian@intel.com> >> Signed-off-by: Joel Granados <joel.granados@kernel.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. Perhaps we can move intel_drain_pasid_prq() into intel_pasid_tear_down_entry(), indicating that once a translation is removed from the pasid and PRI is enabled on the device, the page requests for the pasid should be flushed. Thanks, baolu
On 2024/10/28 16:23, Baolu Lu wrote: > On 2024/10/28 15:50, Yi Liu wrote: >> On 2024/10/16 05:08, Joel Granados wrote: >>> From: Klaus Jensen <k.jensen@samsung.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@samsung.com> >>> Reviewed-by: Kevin Tian <kevin.tian@intel.com> >>> Signed-off-by: Joel Granados <joel.granados@kernel.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. > > Perhaps we can move intel_drain_pasid_prq() into > intel_pasid_tear_down_entry(), indicating that once a translation is > removed from the pasid and PRI is enabled on the device, the page > requests for the pasid should be flushed. > yes, something like the below patch but no flag to opt-out the PRQ drain. https://lore.kernel.org/linux-iommu/20241018055402.23277-3-yi.l.liu@intel.com/ -- Regards, Yi Liu
© 2016 - 2024 Red Hat, Inc.