[PATCH] pci: advertise a page aligned ATS

[PATCH] pci: advertise a page aligned ATS

[Jason Wang]

After Linux kernel commit 61363c1474b1 ("iommu/vt-d: Enable ATS only
if the device uses page aligned address."), ATS will be only enabled
if device advertises a page aligned request.

Unfortunately, vhost-net is the only user and we don't advertise the
aligned request capability in the past since both vhost IOTLB and
address_space_get_iotlb_entry() can support non page aligned request.

Though it's not clear that if the above kernel commit makes
sense. Let's advertise a page aligned ATS here to make vhost device
IOTLB work with Intel IOMMU again.

Note that in the future we may extend pcie_ats_init() to accept
parameters like queue depth and page alignment.

Cc: qemu-stable@nongnu.org
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/pci/pcie.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 5b48bae0f6..d4010cf8f3 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -971,8 +971,9 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset)
 
     dev->exp.ats_cap = offset;
 
-    /* Invalidate Queue Depth 0, Page Aligned Request 0 */
-    pci_set_word(dev->config + offset + PCI_ATS_CAP, 0);
+    /* Invalidate Queue Depth 0, Page Aligned Request 1 */
+    pci_set_word(dev->config + offset + PCI_ATS_CAP,
+                 PCI_ATS_CAP_PAGE_ALIGNED);
     /* STU 0, Disabled by default */
     pci_set_word(dev->config + offset + PCI_ATS_CTRL, 0);
 
-- 
2.20.1

Re: [PATCH] pci: advertise a page aligned ATS

[Peter Xu]

On Wed, Sep 09, 2020 at 04:17:31PM +0800, Jason Wang wrote:
> After Linux kernel commit 61363c1474b1 ("iommu/vt-d: Enable ATS only
> if the device uses page aligned address."), ATS will be only enabled
> if device advertises a page aligned request.
> 
> Unfortunately, vhost-net is the only user and we don't advertise the
> aligned request capability in the past since both vhost IOTLB and
> address_space_get_iotlb_entry() can support non page aligned request.
> 
> Though it's not clear that if the above kernel commit makes
> sense. Let's advertise a page aligned ATS here to make vhost device
> IOTLB work with Intel IOMMU again.

IIUC the kernel commit should be needed because the VT-d Page Request
Descriptor used the rest bits of the address for other use (bits 11-0), so
logically an unaligned address can be mis-recognized with special meanings.
I'd guess some other archs (with its own IOMMU) might support unaligned
addresses and has different layout of page request descriptor, but not vt-d.

> 
> Note that in the future we may extend pcie_ats_init() to accept
> parameters like queue depth and page alignment.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

Maybe it would be good too that vhost provides real 4k-aligned addresses (in
vhost_iotlb_miss)?  My understanding is that PCI_ATS_CAP_PAGE_ALIGNED will be
more compatible than without the bit set.  E.g., so far vt-d emulation always
cut the address with 4k no matter what iova was passed in.  However not sure
whether this will stop working with some new vIOMMUs joining.

Thanks,

-- 
Peter Xu

Re: [PATCH] pci: advertise a page aligned ATS

[Jason Wang]

On 2020/9/9 下午11:43, Peter Xu wrote:
> On Wed, Sep 09, 2020 at 04:17:31PM +0800, Jason Wang wrote:
>> After Linux kernel commit 61363c1474b1 ("iommu/vt-d: Enable ATS only
>> if the device uses page aligned address."), ATS will be only enabled
>> if device advertises a page aligned request.
>>
>> Unfortunately, vhost-net is the only user and we don't advertise the
>> aligned request capability in the past since both vhost IOTLB and
>> address_space_get_iotlb_entry() can support non page aligned request.
>>
>> Though it's not clear that if the above kernel commit makes
>> sense. Let's advertise a page aligned ATS here to make vhost device
>> IOTLB work with Intel IOMMU again.
> IIUC the kernel commit should be needed because the VT-d Page Request
> Descriptor used the rest bits of the address for other use (bits 11-0), so
> logically an unaligned address can be mis-recognized with special meanings.


Yes but it looks to me the problem is that Page Request Descriptor is 
only used when PRI is enabled. And according to ATS spec 1.1, PRI 
request is always page aligned...


> I'd guess some other archs (with its own IOMMU) might support unaligned
> addresses and has different layout of page request descriptor, but not vt-d.


I guess so since I don't find any other IOMMU that does similar check.


>
>> Note that in the future we may extend pcie_ats_init() to accept
>> parameters like queue depth and page alignment.
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> Maybe it would be good too that vhost provides real 4k-aligned addresses (in
> vhost_iotlb_miss)?  My understanding is that PCI_ATS_CAP_PAGE_ALIGNED will be
> more compatible than without the bit set.


Yes, I've considered this. But the problem is that:

1) vhost itself can generate unaligned request (since its IOTLB doesn't 
have any alignment requirement)
2) the IOTLB miss processing in qemu doesn't do anything with ATS, we 
shortcut PCI by calling the address_space_get_iotlb_entry()

So I'm not quite sure it's worth to do that consider we don't emulate 
ATS via PCI actually :)


>    E.g., so far vt-d emulation always
> cut the address with 4k no matter what iova was passed in.  However not sure
> whether this will stop working with some new vIOMMUs joining.


Yes.

Thanks


>
> Thanks,
>

Re: [PATCH] pci: advertise a page aligned ATS

[Peter Xu]

On Thu, Sep 10, 2020 at 09:53:03AM +0800, Jason Wang wrote:
> > Maybe it would be good too that vhost provides real 4k-aligned addresses (in
> > vhost_iotlb_miss)?  My understanding is that PCI_ATS_CAP_PAGE_ALIGNED will be
> > more compatible than without the bit set.
> 
> 
> Yes, I've considered this. But the problem is that:
> 
> 1) vhost itself can generate unaligned request (since its IOTLB doesn't have
> any alignment requirement)
> 2) the IOTLB miss processing in qemu doesn't do anything with ATS, we
> shortcut PCI by calling the address_space_get_iotlb_entry()
> 
> So I'm not quite sure it's worth to do that consider we don't emulate ATS
> via PCI actually :)

True. :) Though we still need to make sure e.g. each translate() iommu op will
drop those bits properly, but I agree that should be trivial.

-- 
Peter Xu

Re: [PATCH] pci: advertise a page aligned ATS

[Jason Wang]

On 2020/9/9 下午4:17, Jason Wang wrote:
> After Linux kernel commit 61363c1474b1 ("iommu/vt-d: Enable ATS only
> if the device uses page aligned address."), ATS will be only enabled
> if device advertises a page aligned request.
>
> Unfortunately, vhost-net is the only user and we don't advertise the
> aligned request capability in the past since both vhost IOTLB and
> address_space_get_iotlb_entry() can support non page aligned request.
>
> Though it's not clear that if the above kernel commit makes
> sense. Let's advertise a page aligned ATS here to make vhost device
> IOTLB work with Intel IOMMU again.
>
> Note that in the future we may extend pcie_ats_init() to accept
> parameters like queue depth and page alignment.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>   hw/pci/pcie.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 5b48bae0f6..d4010cf8f3 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -971,8 +971,9 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset)
>   
>       dev->exp.ats_cap = offset;
>   
> -    /* Invalidate Queue Depth 0, Page Aligned Request 0 */
> -    pci_set_word(dev->config + offset + PCI_ATS_CAP, 0);
> +    /* Invalidate Queue Depth 0, Page Aligned Request 1 */
> +    pci_set_word(dev->config + offset + PCI_ATS_CAP,
> +                 PCI_ATS_CAP_PAGE_ALIGNED);
>       /* STU 0, Disabled by default */
>       pci_set_word(dev->config + offset + PCI_ATS_CTRL, 0);
>   


Ping, Michael, want to pick this patch?

Thanks

Re: [PATCH] pci: advertise a page aligned ATS

[Michael S. Tsirkin]

On Thu, Oct 15, 2020 at 03:47:09PM +0800, Jason Wang wrote:
> 
> On 2020/9/9 下午4:17, Jason Wang wrote:
> > After Linux kernel commit 61363c1474b1 ("iommu/vt-d: Enable ATS only
> > if the device uses page aligned address."), ATS will be only enabled
> > if device advertises a page aligned request.
> > 
> > Unfortunately, vhost-net is the only user and we don't advertise the
> > aligned request capability in the past since both vhost IOTLB and
> > address_space_get_iotlb_entry() can support non page aligned request.
> > 
> > Though it's not clear that if the above kernel commit makes
> > sense. Let's advertise a page aligned ATS here to make vhost device
> > IOTLB work with Intel IOMMU again.
> > 
> > Note that in the future we may extend pcie_ats_init() to accept
> > parameters like queue depth and page alignment.
> > 
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >   hw/pci/pcie.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 5b48bae0f6..d4010cf8f3 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -971,8 +971,9 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset)
> >       dev->exp.ats_cap = offset;
> > -    /* Invalidate Queue Depth 0, Page Aligned Request 0 */
> > -    pci_set_word(dev->config + offset + PCI_ATS_CAP, 0);
> > +    /* Invalidate Queue Depth 0, Page Aligned Request 1 */
> > +    pci_set_word(dev->config + offset + PCI_ATS_CAP,
> > +                 PCI_ATS_CAP_PAGE_ALIGNED);
> >       /* STU 0, Disabled by default */
> >       pci_set_word(dev->config + offset + PCI_ATS_CTRL, 0);
> 
> 
> Ping, Michael, want to pick this patch?
> 
> Thanks

Tagged, thanks!