[PATCH v1 1/1] iommu/vt-d: Enable PRI only if the device enables PASID.

[PATCH v1 1/1] iommu/vt-d: Enable PRI only if the device enables PASID.

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Intel IOMMU Page Request Services (PRS) only works with devices which
supports/uses PASID. So enable PRI only if the device also enables
PASID support. For more details, Please check the implementation of PRQ
handler(prq_event_thread()) in intel-svm driver.

Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/iommu/intel-iommu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 1457f931218e..ef1a646e0993 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1402,7 +1402,8 @@ static void iommu_enable_dev_iotlb(struct device_domain_info *info)
 	if (info->pasid_supported && !pci_enable_pasid(pdev, info->pasid_supported & ~1))
 		info->pasid_enabled = 1;
 
-	if (info->pri_supported && !pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32))
+	if (info->pri_supported && info->pasid_enabled &&
+	    !pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32))
 		info->pri_enabled = 1;
 #endif
 	if (!pdev->untrusted && info->ats_supported &&
-- 
2.20.1

Re: [PATCH v1 1/1] iommu/vt-d: Enable PRI only if the device enables PASID.

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]

On Thu, 2019-02-07 at 10:44 -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> Intel IOMMU Page Request Services (PRS) only works with devices which
> supports/uses PASID. So enable PRI only if the device also enables
> PASID support. For more details, Please check the implementation of PRQ
> handler(prq_event_thread()) in intel-svm driver.
> 
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Ashok Raj <ashok.raj@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Hm, that shouldn't be true. PRI and PASID support are orthogonal.

If we get a PRI request without PASID, we should currently report it as
non-serviced which is the right thing to do. We can hook this up to KVM
etc. to actually allow paging of guests with devices attached, *if* the
devices attached to those guests support PRI.

Nothing fundamentally stops us using PRI without PASID support.



[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [PATCH v1 1/1] iommu/vt-d: Enable PRI only if the device enables PASID.

[Raj, Ashok]

On Thu, Feb 07, 2019 at 08:08:06PM +0000, David Woodhouse wrote:
> On Thu, 2019-02-07 at 10:44 -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > 
> > Intel IOMMU Page Request Services (PRS) only works with devices which
> > supports/uses PASID. So enable PRI only if the device also enables
> > PASID support. For more details, Please check the implementation of PRQ
> > handler(prq_event_thread()) in intel-svm driver.
> > 
> > Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Cc: Ashok Raj <ashok.raj@intel.com>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> Hm, that shouldn't be true. PRI and PASID support are orthogonal.
> 
> If we get a PRI request without PASID, we should currently report it as
> non-serviced which is the right thing to do. We can hook this up to KVM
> etc. to actually allow paging of guests with devices attached, *if* the
> devices attached to those guests support PRI.
> 
> Nothing fundamentally stops us using PRI without PASID support.

You are right.. they are completely orthogonal. We just don't have
a way to handle the page-requests for request without PASID's.

There are some of the vIOMMU work to pass the PRI to who owns
the device, and we can certainly relax it then. This is just to reflect
what support exists today. FWIW, even the native driver maybe be able
to resolve this if supported.

> 
> 

Re: [PATCH v1 1/1] iommu/vt-d: Enable PRI only if the device enables PASID.

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

On Thu, 2019-02-07 at 13:09 -0800, Raj, Ashok wrote:
> You are right.. they are completely orthogonal. We just don't have
> a way to handle the page-requests for request without PASID's.
> 
> There are some of the vIOMMU work to pass the PRI to who owns
> the device, and we can certainly relax it then. This is just to reflect
> what support exists today. FWIW, even the native driver maybe be able
> to resolve this if supported.

As things stand, if a device makes a PRI request without a PASID, it'll
get told that we didn't manage to bring the page in for it. Which is
true.

What's the actual problem being fixed by this patch? Yes, we're going
to want to hook up a way to pass the PRI to the right place... but why
add *another* thing that's just going to have to be fixed, by reverting
this patch?


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [PATCH v1 1/1] iommu/vt-d: Enable PRI only if the device enables PASID.

[Raj, Ashok]

On Thu, Feb 07, 2019 at 09:15:24PM +0000, David Woodhouse wrote:
> On Thu, 2019-02-07 at 13:09 -0800, Raj, Ashok wrote:
> > You are right.. they are completely orthogonal. We just don't have
> > a way to handle the page-requests for request without PASID's.
> > 
> > There are some of the vIOMMU work to pass the PRI to who owns
> > the device, and we can certainly relax it then. This is just to reflect
> > what support exists today. FWIW, even the native driver maybe be able
> > to resolve this if supported.
> 
> As things stand, if a device makes a PRI request without a PASID, it'll
> get told that we didn't manage to bring the page in for it. Which is
> true.

That's true...it does seem to be covered already.. I can't remember
why I thought this was required :-(.. 

We can drop this patch.
> 
> What's the actual problem being fixed by this patch? Yes, we're going
> to want to hook up a way to pass the PRI to the right place... but why
> add *another* thing that's just going to have to be fixed, by reverting
> this patch?
> 

Re: [PATCH v1 1/1] iommu/vt-d: Enable PRI only if the device enables PASID.

[sathyanarayanan kuppuswamy]

On 2/7/19 1:15 PM, David Woodhouse wrote:
> On Thu, 2019-02-07 at 13:09 -0800, Raj, Ashok wrote:
>> You are right.. they are completely orthogonal. We just don't have
>> a way to handle the page-requests for request without PASID's.
>>
>> There are some of the vIOMMU work to pass the PRI to who owns
>> the device, and we can certainly relax it then. This is just to reflect
>> what support exists today. FWIW, even the native driver maybe be able
>> to resolve this if supported.
> As things stand, if a device makes a PRI request without a PASID, it'll
> get told that we didn't manage to bring the page in for it. Which is
> true.
>
> What's the actual problem being fixed by this patch?
Since the request is going to fail any way why go through the process of 
enabling it ? Once the functionality (PRI without PASID) is supported, 
then they can revert this patch. Just we are trying to expose whats 
currently supported clearly.
> Yes, we're going
> to want to hook up a way to pass the PRI to the right place... but why
> add *another* thing that's just going to have to be fixed, by reverting
> this patch?
>
-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer