[PATCH] intel-iommu: don't disable ATS for device without page aligned request

[PATCH] intel-iommu: don't disable ATS for device without page aligned request

[Jason Wang]

Commit 61363c1474b1 ("iommu/vt-d: Enable ATS only if the device uses
page aligned address.") disables ATS for device that can do unaligned
page request.

This looks wrong, since the commit log said it's because the page
request descriptor doesn't support reporting unaligned request.

A victim is Qemu's virtio-pci which doesn't advertise the page aligned
address. Fixing by disable PRI instead of ATS if device doesn't have
page aligned request.

Cc: stable@vger.kernel.org
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Jason Wang <jasowang@redhat.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 e9864e52b0e9..ef5214a8a4dd 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1440,10 +1440,11 @@ static void iommu_enable_dev_iotlb(struct device_domain_info *info)
 
 	if (info->pri_supported &&
 	    (info->pasid_enabled ? pci_prg_resp_pasid_required(pdev) : 1)  &&
+	    pci_ats_page_aligned(pdev) &&
 	    !pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32))
 		info->pri_enabled = 1;
 #endif
-	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
+	if (info->ats_supported &&
 	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
 		info->ats_enabled = 1;
 		domain_update_iotlb(info->domain);
-- 
2.20.1

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

Re: [PATCH] intel-iommu: don't disable ATS for device without page aligned request

[Raj, Ashok]

Hi Jason

On Wed, Sep 09, 2020 at 04:34:32PM +0800, Jason Wang wrote:
> Commit 61363c1474b1 ("iommu/vt-d: Enable ATS only if the device uses
> page aligned address.") disables ATS for device that can do unaligned
> page request.

Did you take a look at the PCI specification?
Page Aligned Request is in the ATS capability Register.

ATS Capability Register (Offset 0x04h)

bit (5):
Page Aligned Request - If Set, indicates the Untranslated address is always
aligned to 4096 byte boundary. Setting this field is recommended. This
field permits software to distinguish between implemntations compatible
with this specification and those compatible with an earlier version of
this specification in which a Requester was permitted to supply anything in
bits [11:2].

> 
> This looks wrong, since the commit log said it's because the page
> request descriptor doesn't support reporting unaligned request.

I don't think you can change the definition from ATS to PRI. Both are
orthogonal feature.

> 
> A victim is Qemu's virtio-pci which doesn't advertise the page aligned
> address. Fixing by disable PRI instead of ATS if device doesn't have
> page aligned request.

This is a requirement for the Intel IOMMU's.

You say virtio, so is it all emulated device or you talking about some
hardware that implemented virtio-pci compliant hw? If you are sure the
device actually does comply with the requirement, but just not enumerating
the capability, you can maybe work a quirk to overcome that?

Now PRI also has an alignment requirement, and Intel IOMMU's requires that
as well. If your device supports SRIOV as well, PASID and PRI are
enumerated just on the PF and not the VF. You might want to pay attension
to that. We are still working on a solution for that problem.

I don't think this is the right fix for your problem. 

> 
> Cc: stable@vger.kernel.org
> Cc: Ashok Raj <ashok.raj@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Keith Busch <keith.busch@intel.com>
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/iommu/intel/iommu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] intel-iommu: don't disable ATS for device without page aligned request

[Jason Wang]

----- Original Message -----
> Hi Jason
> 
> On Wed, Sep 09, 2020 at 04:34:32PM +0800, Jason Wang wrote:
> > Commit 61363c1474b1 ("iommu/vt-d: Enable ATS only if the device uses
> > page aligned address.") disables ATS for device that can do unaligned
> > page request.
> 
> Did you take a look at the PCI specification?
> Page Aligned Request is in the ATS capability Register.
> 
> ATS Capability Register (Offset 0x04h)
> 
> bit (5):
> Page Aligned Request - If Set, indicates the Untranslated address is always
> aligned to 4096 byte boundary. Setting this field is recommended. This
> field permits software to distinguish between implemntations compatible
> with this specification and those compatible with an earlier version of
> this specification in which a Requester was permitted to supply anything in
> bits [11:2].

Yes, my understanding is that this is optional not mandatory.

> 
> > 
> > This looks wrong, since the commit log said it's because the page
> > request descriptor doesn't support reporting unaligned request.
> 
> I don't think you can change the definition from ATS to PRI. Both are
> orthogonal feature.

I may miss something, here's my understanding is that:

- page request descriptor will only be used when PRS is enabled
- ATS spec allows unaligned request

So any reason for disabling ATS for unaligned request even if PRS is
not enabled?

> 
> > 
> > A victim is Qemu's virtio-pci which doesn't advertise the page aligned
> > address. Fixing by disable PRI instead of ATS if device doesn't have
> > page aligned request.
> 
> This is a requirement for the Intel IOMMU's.
> 
> You say virtio, so is it all emulated device or you talking about some
> hardware that implemented virtio-pci compliant hw? If you are sure the
> device actually does comply with the requirement, but just not enumerating
> the capability, you can maybe work a quirk to overcome that?

So far only emulated devices. But we are helping some vendor to
implement virtio hardware so  we need to understand the connection
between ATS alignment and page request descriptor.

> 
> Now PRI also has an alignment requirement, and Intel IOMMU's requires that
> as well. If your device supports SRIOV as well, PASID and PRI are
> enumerated just on the PF and not the VF. You might want to pay attension
> to that. We are still working on a solution for that problem.

Thanks for the reminding, but it looks to me according to the ATS
spec, all PRI message is 4096 byte aligned? E.g lower bites were used
for group index etc.

Thanks

> 
> I don't think this is the right fix for your problem.
> 
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Ashok Raj <ashok.raj@intel.com>
> > Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Cc: Keith Busch <keith.busch@intel.com>
> > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  drivers/iommu/intel/iommu.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> 
> 

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

Re: [PATCH] intel-iommu: don't disable ATS for device without page aligned request

[Raj, Ashok]

On Wed, Sep 09, 2020 at 10:17:35PM -0400, Jason Wang wrote:
> 
> 
> ----- Original Message -----
> > Hi Jason
> > 
> > On Wed, Sep 09, 2020 at 04:34:32PM +0800, Jason Wang wrote:
> > > Commit 61363c1474b1 ("iommu/vt-d: Enable ATS only if the device uses
> > > page aligned address.") disables ATS for device that can do unaligned
> > > page request.
> > 
> > Did you take a look at the PCI specification?
> > Page Aligned Request is in the ATS capability Register.
> > 
> > ATS Capability Register (Offset 0x04h)
> > 
> > bit (5):
> > Page Aligned Request - If Set, indicates the Untranslated address is always
> > aligned to 4096 byte boundary. Setting this field is recommended. This
> > field permits software to distinguish between implemntations compatible
> > with this specification and those compatible with an earlier version of
> > this specification in which a Requester was permitted to supply anything in
> > bits [11:2].
> 
> Yes, my understanding is that this is optional not mandatory.

Correct, but optional on the device side. An IOMMU might *require* this for
proper normal operation. Our IOMMU's do not get the low 12 bits. Which is
why the spec gives SW a way to detect if the device is compatible for this
IOMMU implementation.

> 
> > 
> > > 
> > > This looks wrong, since the commit log said it's because the page
> > > request descriptor doesn't support reporting unaligned request.
> > 
> > I don't think you can change the definition from ATS to PRI. Both are
> > orthogonal feature.
> 
> I may miss something, here's my understanding is that:
> 
> - page request descriptor will only be used when PRS is enabled
> - ATS spec allows unaligned request
> 
> So any reason for disabling ATS for unaligned request even if PRS is
> not enabled?

I think you are getting confused between the 2 different PCIe features.

ATS - Address Translation Services. Used by device to simply request the
Host Physical Address for some DMA operation. 

When ATS response indicates failed, then the device can request a
page-request (PRS this is like a device page-fault), and then IOMMU driver
would work with the kernel to fault a page then respond with
(Page-response) success/failure. Then the device will send a new ATS 
to get the new translation. 


> 
> > 
> > > 
> > > A victim is Qemu's virtio-pci which doesn't advertise the page aligned
> > > address. Fixing by disable PRI instead of ATS if device doesn't have
> > > page aligned request.
> > 
> > This is a requirement for the Intel IOMMU's.
> > 
> > You say virtio, so is it all emulated device or you talking about some
> > hardware that implemented virtio-pci compliant hw? If you are sure the
> > device actually does comply with the requirement, but just not enumerating
> > the capability, you can maybe work a quirk to overcome that?
> 
> So far only emulated devices. But we are helping some vendor to
> implement virtio hardware so  we need to understand the connection
> between ATS alignment and page request descriptor.

ATS and PRS are 2 separate orthogonal features. 
PRS requires ATS, but not the other way around. 

> 
> > 
> > Now PRI also has an alignment requirement, and Intel IOMMU's requires that
> > as well. If your device supports SRIOV as well, PASID and PRI are
> > enumerated just on the PF and not the VF. You might want to pay attension
> > to that. We are still working on a solution for that problem.
> 
> Thanks for the reminding, but it looks to me according to the ATS
> spec, all PRI message is 4096 byte aligned? E.g lower bites were used
> for group index etc.

Right, I should have been clear. The issue with PRI is we require responses
to have PASID field set. There is another capability on the device that
exposes that. pci_prg_resp_pasid_required(). This is required to enable PRI
for a device.

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

Re: [PATCH] intel-iommu: don't disable ATS for device without page aligned request

[Jason Wang]

On 2020/9/10 下午11:53, Raj, Ashok wrote:
> On Wed, Sep 09, 2020 at 10:17:35PM -0400, Jason Wang wrote:
>>
>> ----- Original Message -----
>>> Hi Jason
>>>
>>> On Wed, Sep 09, 2020 at 04:34:32PM +0800, Jason Wang wrote:
>>>> Commit 61363c1474b1 ("iommu/vt-d: Enable ATS only if the device uses
>>>> page aligned address.") disables ATS for device that can do unaligned
>>>> page request.
>>> Did you take a look at the PCI specification?
>>> Page Aligned Request is in the ATS capability Register.
>>>
>>> ATS Capability Register (Offset 0x04h)
>>>
>>> bit (5):
>>> Page Aligned Request - If Set, indicates the Untranslated address is always
>>> aligned to 4096 byte boundary. Setting this field is recommended. This
>>> field permits software to distinguish between implemntations compatible
>>> with this specification and those compatible with an earlier version of
>>> this specification in which a Requester was permitted to supply anything in
>>> bits [11:2].
>> Yes, my understanding is that this is optional not mandatory.
> Correct, but optional on the device side. An IOMMU might *require* this for
> proper normal operation. Our IOMMU's do not get the low 12 bits. Which is
> why the spec gives SW a way to detect if the device is compatible for this
> IOMMU implementation.


I see, it's better to clarify this in the spec (or is there already had 
a section for this?)


>
>>>> This looks wrong, since the commit log said it's because the page
>>>> request descriptor doesn't support reporting unaligned request.
>>> I don't think you can change the definition from ATS to PRI. Both are
>>> orthogonal feature.
>> I may miss something, here's my understanding is that:
>>
>> - page request descriptor will only be used when PRS is enabled
>> - ATS spec allows unaligned request
>>
>> So any reason for disabling ATS for unaligned request even if PRS is
>> not enabled?
> I think you are getting confused between the 2 different PCIe features.
>
> ATS - Address Translation Services. Used by device to simply request the
> Host Physical Address for some DMA operation.
>
> When ATS response indicates failed, then the device can request a
> page-request (PRS this is like a device page-fault), and then IOMMU driver
> would work with the kernel to fault a page then respond with
> (Page-response) success/failure. Then the device will send a new ATS
> to get the new translation.


Yes, that's my understanding as well. I think what confuses me is the 
commit log of 61363c1474b1 which ties a PRI queue to ATS features ...


>
>
>>>> A victim is Qemu's virtio-pci which doesn't advertise the page aligned
>>>> address. Fixing by disable PRI instead of ATS if device doesn't have
>>>> page aligned request.
>>> This is a requirement for the Intel IOMMU's.
>>>
>>> You say virtio, so is it all emulated device or you talking about some
>>> hardware that implemented virtio-pci compliant hw? If you are sure the
>>> device actually does comply with the requirement, but just not enumerating
>>> the capability, you can maybe work a quirk to overcome that?
>> So far only emulated devices. But we are helping some vendor to
>> implement virtio hardware so  we need to understand the connection
>> between ATS alignment and page request descriptor.
> ATS and PRS are 2 separate orthogonal features.
> PRS requires ATS, but not the other way around.
>
>>> Now PRI also has an alignment requirement, and Intel IOMMU's requires that
>>> as well. If your device supports SRIOV as well, PASID and PRI are
>>> enumerated just on the PF and not the VF. You might want to pay attension
>>> to that. We are still working on a solution for that problem.
>> Thanks for the reminding, but it looks to me according to the ATS
>> spec, all PRI message is 4096 byte aligned? E.g lower bites were used
>> for group index etc.
> Right, I should have been clear. The issue with PRI is we require responses
> to have PASID field set. There is another capability on the device that
> exposes that. pci_prg_resp_pasid_required(). This is required to enable PRI
> for a device.


Right. Thanks for the clarification, I will withdraw this patch.


>
> Cheers,
> Ashok
>

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