[PATCH v1 0/3] iommu: Fix a few issues related to PRQ

[PATCH v1 0/3] iommu: Fix a few issues related to PRQ

[Yi Sun]

We found a few issues about PRQ. So, three patches are cooked to
fix them. Please have a review. Thanks!

Jacob Pan (1):
  iommu: Fix an issue in iommu_page_response() flags check

Liu Yi L (1):
  iommu/vt-d: Fix prq reporting issues

Liu, Yi L (1):
  iommu/vt-d: Fix a bug for PDP check in prq_event_thread

 drivers/iommu/intel/svm.c | 5 ++++-
 drivers/iommu/iommu.c     | 6 ++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

-- 
2.7.4

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v1 1/3] iommu/vt-d: Fix prq reporting issues

[Yi Sun]

From: Liu Yi L <yi.l.liu@intel.com>

Should get correct sid and set it into sdev. Because we execute
'sdev->sid != req->rid' in the loop of prq_event_thread().

Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
---
 drivers/iommu/intel/svm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index f1861fa..75d9dc9 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -277,6 +277,7 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev,
 			  struct iommu_gpasid_bind_data *data)
 {
 	struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
+	struct device_domain_info *info;
 	struct intel_svm_dev *sdev = NULL;
 	struct dmar_domain *dmar_domain;
 	struct intel_svm *svm = NULL;
@@ -357,6 +358,8 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev,
 		goto out;
 	}
 	sdev->dev = dev;
+	info = get_domain_info(dev);
+	sdev->sid = PCI_DEVID(info->bus, info->devfn);
 
 	/* Only count users if device has aux domains */
 	if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX))
-- 
2.7.4

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v1 2/3] iommu: Fix an issue in iommu_page_response() flags check

[Yi Sun]

From: Jacob Pan <jacob.jun.pan@linux.intel.com>

original code fails when LAST_PAGE is set in flags.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
---
 drivers/iommu/iommu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8c470f4..053cec3 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1200,9 +1200,11 @@ int iommu_page_response(struct device *dev,
 		return -EINVAL;
 
 	if (msg->version != IOMMU_PAGE_RESP_VERSION_1 ||
-	    msg->flags & ~IOMMU_PAGE_RESP_PASID_VALID)
+		!(msg->flags & IOMMU_PAGE_RESP_PASID_VALID)) {
+		dev_warn_ratelimited(dev, "%s:Invalid ver %x: flags %x\n",
+				__func__, msg->version, msg->flags);
 		return -EINVAL;
-
+	}
 	/* Only send response if there is a fault report pending */
 	mutex_lock(&param->fault_param->lock);
 	if (list_empty(&param->fault_param->faults)) {
-- 
2.7.4

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v1 3/3] iommu/vt-d: Fix a bug for PDP check in prq_event_thread

[Yi Sun]

From: "Liu, Yi L" <yi.l.liu@intel.com>

Signed-off-by: Liu, Yi L <yi.l.liu@intel.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
---
 drivers/iommu/intel/svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 75d9dc9..1870248 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -1032,7 +1032,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 			resp.qw0 = QI_PGRP_PASID(req->pasid) |
 				QI_PGRP_DID(req->rid) |
 				QI_PGRP_PASID_P(req->pasid_present) |
-				QI_PGRP_PDP(req->pasid_present) |
+				QI_PGRP_PDP(req->priv_data_present) |
 				QI_PGRP_RESP_CODE(result) |
 				QI_PGRP_RESP_TYPE;
 			resp.qw1 = QI_PGRP_IDX(req->prg_index) |
-- 
2.7.4

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 1/3] iommu/vt-d: Fix prq reporting issues

[Lu Baolu]

Hi Yi,

On 10/28/20 9:36 AM, Yi Sun wrote:
> From: Liu Yi L <yi.l.liu@intel.com>
> 
> Should get correct sid and set it into sdev. Because we execute
> 'sdev->sid != req->rid' in the loop of prq_event_thread().

How about making the title more accurate, how about something like:

iommu/vt-d: Fix use before set issue in intel_svm_bind_gpasid()

> 
> Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> ---
>   drivers/iommu/intel/svm.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index f1861fa..75d9dc9 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -277,6 +277,7 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev,
>   			  struct iommu_gpasid_bind_data *data)
>   {
>   	struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
> +	struct device_domain_info *info;
>   	struct intel_svm_dev *sdev = NULL;
>   	struct dmar_domain *dmar_domain;
>   	struct intel_svm *svm = NULL;
> @@ -357,6 +358,8 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev,
>   		goto out;
>   	}
>   	sdev->dev = dev;
> +	info = get_domain_info(dev);
> +	sdev->sid = PCI_DEVID(info->bus, info->devfn);
>   
>   	/* Only count users if device has aux domains */
>   	if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX))
> 

How about moving get_domain_info() up to the sanity check part?

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index f1861fa3d0e4..75846692f2f2 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -279,6 +279,7 @@ int intel_svm_bind_gpasid(struct iommu_domain 
*domain, struct device *dev,
         struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
         struct intel_svm_dev *sdev = NULL;
         struct dmar_domain *dmar_domain;
+       struct device_domain_info *info;
         struct intel_svm *svm = NULL;
         int ret = 0;

@@ -310,6 +311,10 @@ int intel_svm_bind_gpasid(struct iommu_domain 
*domain, struct device *dev,
         if (data->hpasid <= 0 || data->hpasid >= PASID_MAX)
                 return -EINVAL;

+       info = get_domain_info(dev);
+       if (!info)
+               return -EINVAL;
+
         dmar_domain = to_dmar_domain(domain);

         mutex_lock(&pasid_mutex);
@@ -357,6 +362,7 @@ int intel_svm_bind_gpasid(struct iommu_domain 
*domain, struct device *dev,
                 goto out;
         }
         sdev->dev = dev;
+       sdev->sid = PCI_DEVID(info->bus, info->devfn);

         /* Only count users if device has aux domains */
         if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX))

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 3/3] iommu/vt-d: Fix a bug for PDP check in prq_event_thread

[Lu Baolu]

Hi Yi,

On 10/28/20 9:36 AM, Yi Sun wrote:
> From: "Liu, Yi L" <yi.l.liu@intel.com>

Can you please add some description here? How far should this patch back
ported? A Fixes tag?

Best regards,
baolu

> 
> Signed-off-by: Liu, Yi L <yi.l.liu@intel.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> ---
>   drivers/iommu/intel/svm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 75d9dc9..1870248 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -1032,7 +1032,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
>   			resp.qw0 = QI_PGRP_PASID(req->pasid) |
>   				QI_PGRP_DID(req->rid) |
>   				QI_PGRP_PASID_P(req->pasid_present) |
> -				QI_PGRP_PDP(req->pasid_present) |
> +				QI_PGRP_PDP(req->priv_data_present) |
>   				QI_PGRP_RESP_CODE(result) |
>   				QI_PGRP_RESP_TYPE;
>   			resp.qw1 = QI_PGRP_IDX(req->prg_index) |
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 1/3] iommu/vt-d: Fix prq reporting issues

[Yi Sun]

Thanks! Will do the modifications according to your comments in next
version.

On 20-10-28 12:50:37, Lu Baolu wrote:
> Hi Yi,
> 
> On 10/28/20 9:36 AM, Yi Sun wrote:
> >From: Liu Yi L <yi.l.liu@intel.com>
> >
> >Should get correct sid and set it into sdev. Because we execute
> >'sdev->sid != req->rid' in the loop of prq_event_thread().
> 
> How about making the title more accurate, how about something like:
> 
> iommu/vt-d: Fix use before set issue in intel_svm_bind_gpasid()
> 
> >
> >Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> >Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> >---
> >  drivers/iommu/intel/svm.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> >diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> >index f1861fa..75d9dc9 100644
> >--- a/drivers/iommu/intel/svm.c
> >+++ b/drivers/iommu/intel/svm.c
> >@@ -277,6 +277,7 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev,
> >  			  struct iommu_gpasid_bind_data *data)
> >  {
> >  	struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
> >+	struct device_domain_info *info;
> >  	struct intel_svm_dev *sdev = NULL;
> >  	struct dmar_domain *dmar_domain;
> >  	struct intel_svm *svm = NULL;
> >@@ -357,6 +358,8 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev,
> >  		goto out;
> >  	}
> >  	sdev->dev = dev;
> >+	info = get_domain_info(dev);
> >+	sdev->sid = PCI_DEVID(info->bus, info->devfn);
> >  	/* Only count users if device has aux domains */
> >  	if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX))
> >
> 
> How about moving get_domain_info() up to the sanity check part?
> 
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index f1861fa3d0e4..75846692f2f2 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -279,6 +279,7 @@ int intel_svm_bind_gpasid(struct iommu_domain
> *domain, struct device *dev,
>         struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
>         struct intel_svm_dev *sdev = NULL;
>         struct dmar_domain *dmar_domain;
> +       struct device_domain_info *info;
>         struct intel_svm *svm = NULL;
>         int ret = 0;
> 
> @@ -310,6 +311,10 @@ int intel_svm_bind_gpasid(struct iommu_domain
> *domain, struct device *dev,
>         if (data->hpasid <= 0 || data->hpasid >= PASID_MAX)
>                 return -EINVAL;
> 
> +       info = get_domain_info(dev);
> +       if (!info)
> +               return -EINVAL;
> +
>         dmar_domain = to_dmar_domain(domain);
> 
>         mutex_lock(&pasid_mutex);
> @@ -357,6 +362,7 @@ int intel_svm_bind_gpasid(struct iommu_domain
> *domain, struct device *dev,
>                 goto out;
>         }
>         sdev->dev = dev;
> +       sdev->sid = PCI_DEVID(info->bus, info->devfn);
> 
>         /* Only count users if device has aux domains */
>         if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX))
> 
> Best regards,
> baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 3/3] iommu/vt-d: Fix a bug for PDP check in prq_event_thread

[Yi Sun]

On 20-10-28 13:05:05, Lu Baolu wrote:
> Hi Yi,
> 
> On 10/28/20 9:36 AM, Yi Sun wrote:
> >From: "Liu, Yi L" <yi.l.liu@intel.com>
> 
> Can you please add some description here? How far should this patch back
> ported? A Fixes tag?
> 
Sure. Will add description and Fixes tag.

> Best regards,
> baolu
> 
> >
> >Signed-off-by: Liu, Yi L <yi.l.liu@intel.com>
> >Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> >---
> >  drivers/iommu/intel/svm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> >index 75d9dc9..1870248 100644
> >--- a/drivers/iommu/intel/svm.c
> >+++ b/drivers/iommu/intel/svm.c
> >@@ -1032,7 +1032,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
> >  			resp.qw0 = QI_PGRP_PASID(req->pasid) |
> >  				QI_PGRP_DID(req->rid) |
> >  				QI_PGRP_PASID_P(req->pasid_present) |
> >-				QI_PGRP_PDP(req->pasid_present) |
> >+				QI_PGRP_PDP(req->priv_data_present) |
> >  				QI_PGRP_RESP_CODE(result) |
> >  				QI_PGRP_RESP_TYPE;
> >  			resp.qw1 = QI_PGRP_IDX(req->prg_index) |
> >
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 2/3] iommu: Fix an issue in iommu_page_response() flags check

[Jean-Philippe Brucker]

Hi,

On Wed, Oct 28, 2020 at 09:36:57AM +0800, Yi Sun wrote:
> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> 
> original code fails when LAST_PAGE is set in flags.

LAST_PAGE is not documented to be a valid flags for page_response.
So isn't failing the right thing to do?

> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 8c470f4..053cec3 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1200,9 +1200,11 @@ int iommu_page_response(struct device *dev,
>  		return -EINVAL;
>  
>  	if (msg->version != IOMMU_PAGE_RESP_VERSION_1 ||
> -	    msg->flags & ~IOMMU_PAGE_RESP_PASID_VALID)
> +		!(msg->flags & IOMMU_PAGE_RESP_PASID_VALID)) {

It should be OK not to have PASID_VALID set, we're just checking for
undefined flags here.

Thanks,
Jean

> +		dev_warn_ratelimited(dev, "%s:Invalid ver %x: flags %x\n",
> +				__func__, msg->version, msg->flags);
>  		return -EINVAL;
> -
> +	}
>  	/* Only send response if there is a fault report pending */
>  	mutex_lock(&param->fault_param->lock);
>  	if (list_empty(&param->fault_param->faults)) {
> -- 
> 2.7.4
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 2/3] iommu: Fix an issue in iommu_page_response() flags check

[Yi Sun]

On 20-10-28 10:13:56, Jean-Philippe Brucker wrote:
> Hi,
> 
> On Wed, Oct 28, 2020 at 09:36:57AM +0800, Yi Sun wrote:
> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > 
> > original code fails when LAST_PAGE is set in flags.
> 
> LAST_PAGE is not documented to be a valid flags for page_response.
> So isn't failing the right thing to do?
> 
> > 
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> > ---
> >  drivers/iommu/iommu.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 8c470f4..053cec3 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1200,9 +1200,11 @@ int iommu_page_response(struct device *dev,
> >  		return -EINVAL;
> >  
> >  	if (msg->version != IOMMU_PAGE_RESP_VERSION_1 ||
> > -	    msg->flags & ~IOMMU_PAGE_RESP_PASID_VALID)
> > +		!(msg->flags & IOMMU_PAGE_RESP_PASID_VALID)) {
> 
> It should be OK not to have PASID_VALID set, we're just checking for
> undefined flags here.
> 
Thanks! You are right. Per published spec, we should not care LAST_PAGE
for page_response. I will remove this patch in next version.
 
> Thanks,
> Jean
> 
> > +		dev_warn_ratelimited(dev, "%s:Invalid ver %x: flags %x\n",
> > +				__func__, msg->version, msg->flags);
> >  		return -EINVAL;
> > -
> > +	}
> >  	/* Only send response if there is a fault report pending */
> >  	mutex_lock(&param->fault_param->lock);
> >  	if (list_empty(&param->fault_param->faults)) {
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > iommu mailing list
> > iommu@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/iommu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu