Re: [Xen-devel] [PATCH v10 3/3] vt-d: fix vt-d Device-TLB flush timeout issue
On May 20, 2016 5:59 PM, Jan Beulich wrote:
> >>> On 20.05.16 at 09:15, wrote:
> > On May 17, 2016 10:00 PM, Jan Beulich wrote:
> >> >>> On 22.04.16 at 12:54, wrote:
> >> > --- a/xen/drivers/passthrough/vtd/qinval.c
> >> > +++ b/xen/drivers/passthrough/vtd/qinval.c
> >> > @@ -206,10 +206,71 @@ static int invalidate_sync(struct iommu
> *iommu)
> >> > return 0;
> >> > }
> >> >
> >> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
> >> > + u16 seg, u8 bus, u8 devfn) {
> >> > +struct domain *d = NULL;
> >> > +struct pci_dev *pdev;
> >> > +
> >> > +if ( test_bit(did, iommu->domid_bitmap) )
> >> > +d = rcu_lock_domain_by_id(iommu->domid_map[did]);
> >> > +
> >> > +/*
> >> > + * In case the domain has been freed or the IOMMU domid bitmap is
> >> > + * not valid, the device no longer belongs to this domain.
> >> > + */
> >> > +if ( d == NULL )
> >> > +return;
> >> > +
> >> > +pcidevs_lock();
> >> > +
> >> > +for_each_pdev(d, pdev)
> >> > +{
> >> > +if ( (pdev->seg == seg) &&
> >> > + (pdev->bus == bus) &&
> >> > + (pdev->devfn == devfn) )
> >> > +{
> >> > +ASSERT(pdev->domain);
> >> > +list_del(&pdev->domain_list);
> >> > +pdev->domain = NULL;
> >> > +pci_hide_existing_device(pdev);
> >> > +break;
> >> > +}
> >> > +}
> >>
> >> A loop like this is of course not ideal (especially for Dom0, which
> >> may have many devices). And I wonder why you, ...
> >>
> >> > @@ -134,8 +133,9 @@ int dev_invalidate_iotlb(struct iommu *iommu,
> >> > u16
> >> did,
> >> > /* invalidate all translations:
> >> > sbit=1,bit_63=0,bit[62:12]=1
> > */
> >> > sbit = 1;
> >> > addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFF;
> >> > -rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> >> > - sid, sbit, addr);
> >> > +rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> did,
> >> > + pdev->seg, pdev->bus,
> >> > pdev->devfn,
> >> > + sbit, addr);
> >> > break;
> >> > case DMA_TLB_PSI_FLUSH:
> >> > if ( !device_in_domain(iommu, pdev, did) ) @@ -154,8
> >> > +154,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
> >> > addr |= (((u64)1 << (size_order - 1)) - 1) <<
> >> > PAGE_SHIFT_4K;
> >> > }
> >> >
> >> > -rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> >> > - sid, sbit, addr);
> >> > +rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> did,
> >> > + pdev->seg, pdev->bus,
> >> > pdev->devfn,
> >> > + sbit, addr);
> >> > break;
> >>
> >> ... holding pdev in your hands here, don't just pass it down (which
> >> at once would make the function signature less convoluted: you could
> >> even eliminate the currently 2nd parameter that way).
> >
> > I am afraid we need to leave it as is.. this pdev , in
> > dev_invalidate_iotlb(), is 'struct pci_ats_dev', but we need a 'struct
> > pci_dev' to hide device in dev_invalidate_iotlb_timeout().
> >
> > 'struct pci_ats_dev' and 'struct pci_dev' are quite different,
> > however, SBDF is connection between them..
>
> Oh, indeed. Yet - can't enable_ats_device() be passed a struct pci_dev *, and
> that be stored instead of SBDF inside struct pci_ats_dev?
>
Make sense. I appreciate your specific advice.
Quan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v10 3/3] vt-d: fix vt-d Device-TLB flush timeout issue
>>> On 20.05.16 at 09:15, wrote:
> On May 17, 2016 10:00 PM, Jan Beulich wrote:
>> >>> On 22.04.16 at 12:54, wrote:
>> > --- a/xen/drivers/passthrough/vtd/qinval.c
>> > +++ b/xen/drivers/passthrough/vtd/qinval.c
>> > @@ -206,10 +206,71 @@ static int invalidate_sync(struct iommu *iommu)
>> > return 0;
>> > }
>> >
>> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
>> > + u16 seg, u8 bus, u8 devfn) {
>> > +struct domain *d = NULL;
>> > +struct pci_dev *pdev;
>> > +
>> > +if ( test_bit(did, iommu->domid_bitmap) )
>> > +d = rcu_lock_domain_by_id(iommu->domid_map[did]);
>> > +
>> > +/*
>> > + * In case the domain has been freed or the IOMMU domid bitmap is
>> > + * not valid, the device no longer belongs to this domain.
>> > + */
>> > +if ( d == NULL )
>> > +return;
>> > +
>> > +pcidevs_lock();
>> > +
>> > +for_each_pdev(d, pdev)
>> > +{
>> > +if ( (pdev->seg == seg) &&
>> > + (pdev->bus == bus) &&
>> > + (pdev->devfn == devfn) )
>> > +{
>> > +ASSERT(pdev->domain);
>> > +list_del(&pdev->domain_list);
>> > +pdev->domain = NULL;
>> > +pci_hide_existing_device(pdev);
>> > +break;
>> > +}
>> > +}
>>
>> A loop like this is of course not ideal (especially for Dom0, which may have
>> many devices). And I wonder why you, ...
>>
>> > @@ -134,8 +133,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16
>> did,
>> > /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1
> */
>> > sbit = 1;
>> > addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFF;
>> > -rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
>> > - sid, sbit, addr);
>> > +rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
>> > did,
>> > + pdev->seg, pdev->bus,
>> > pdev->devfn,
>> > + sbit, addr);
>> > break;
>> > case DMA_TLB_PSI_FLUSH:
>> > if ( !device_in_domain(iommu, pdev, did) ) @@ -154,8
>> > +154,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
>> > addr |= (((u64)1 << (size_order - 1)) - 1) <<
>> > PAGE_SHIFT_4K;
>> > }
>> >
>> > -rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
>> > - sid, sbit, addr);
>> > +rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
>> > did,
>> > + pdev->seg, pdev->bus,
>> > pdev->devfn,
>> > + sbit, addr);
>> > break;
>>
>> ... holding pdev in your hands here, don't just pass it down (which at once
>> would make the function signature less convoluted: you could even eliminate
>> the currently 2nd parameter that way).
>
> I am afraid we need to leave it as is.. this pdev , in
> dev_invalidate_iotlb(), is 'struct pci_ats_dev',
> but we need a 'struct pci_dev' to hide device in
> dev_invalidate_iotlb_timeout().
>
> 'struct pci_ats_dev' and 'struct pci_dev' are quite different, however, SBDF
> is connection between them..
Oh, indeed. Yet - can't enable_ats_device() be passed a
struct pci_dev *, and that be stored instead of SBDF inside
struct pci_ats_dev?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v10 3/3] vt-d: fix vt-d Device-TLB flush timeout issue
On May 17, 2016 10:00 PM, Jan Beulich wrote:
> >>> On 22.04.16 at 12:54, wrote:
> > --- a/xen/drivers/passthrough/vtd/qinval.c
> > +++ b/xen/drivers/passthrough/vtd/qinval.c
> > @@ -206,10 +206,71 @@ static int invalidate_sync(struct iommu *iommu)
> > return 0;
> > }
> >
> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
> > + u16 seg, u8 bus, u8 devfn) {
> > +struct domain *d = NULL;
> > +struct pci_dev *pdev;
> > +
> > +if ( test_bit(did, iommu->domid_bitmap) )
> > +d = rcu_lock_domain_by_id(iommu->domid_map[did]);
> > +
> > +/*
> > + * In case the domain has been freed or the IOMMU domid bitmap is
> > + * not valid, the device no longer belongs to this domain.
> > + */
> > +if ( d == NULL )
> > +return;
> > +
> > +pcidevs_lock();
> > +
> > +for_each_pdev(d, pdev)
> > +{
> > +if ( (pdev->seg == seg) &&
> > + (pdev->bus == bus) &&
> > + (pdev->devfn == devfn) )
> > +{
> > +ASSERT(pdev->domain);
> > +list_del(&pdev->domain_list);
> > +pdev->domain = NULL;
> > +pci_hide_existing_device(pdev);
> > +break;
> > +}
> > +}
>
> A loop like this is of course not ideal (especially for Dom0, which may have
> many devices). And I wonder why you, ...
>
> > @@ -134,8 +133,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16
> did,
> > /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
> > sbit = 1;
> > addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFF;
> > -rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> > - sid, sbit, addr);
> > +rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> > did,
> > + pdev->seg, pdev->bus,
> > pdev->devfn,
> > + sbit, addr);
> > break;
> > case DMA_TLB_PSI_FLUSH:
> > if ( !device_in_domain(iommu, pdev, did) ) @@ -154,8
> > +154,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
> > addr |= (((u64)1 << (size_order - 1)) - 1) <<
> > PAGE_SHIFT_4K;
> > }
> >
> > -rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> > - sid, sbit, addr);
> > +rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> > did,
> > + pdev->seg, pdev->bus,
> > pdev->devfn,
> > + sbit, addr);
> > break;
>
> ... holding pdev in your hands here, don't just pass it down (which at once
> would make the function signature less convoluted: you could even eliminate
> the currently 2nd parameter that way).
>
Jan,
I am afraid we need to leave it as is.. this pdev , in
dev_invalidate_iotlb(), is 'struct pci_ats_dev',
but we need a 'struct pci_dev' to hide device in dev_invalidate_iotlb_timeout().
'struct pci_ats_dev' and 'struct pci_dev' are quite different, however, SBDF is
connection between them..
Quan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v10 3/3] vt-d: fix vt-d Device-TLB flush timeout issue
On May 17, 2016 10:00 PM, Jan Beulich wrote:
> >>> On 22.04.16 at 12:54, wrote:
> > --- a/xen/drivers/passthrough/vtd/qinval.c
> > +++ b/xen/drivers/passthrough/vtd/qinval.c
> > @@ -206,10 +206,71 @@ static int invalidate_sync(struct iommu *iommu)
> > return 0;
> > }
> >
> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
> > + u16 seg, u8 bus, u8 devfn) {
> > +struct domain *d = NULL;
> > +struct pci_dev *pdev;
> > +
> > +if ( test_bit(did, iommu->domid_bitmap) )
> > +d = rcu_lock_domain_by_id(iommu->domid_map[did]);
> > +
> > +/*
> > + * In case the domain has been freed or the IOMMU domid bitmap is
> > + * not valid, the device no longer belongs to this domain.
> > + */
> > +if ( d == NULL )
> > +return;
> > +
> > +pcidevs_lock();
> > +
> > +for_each_pdev(d, pdev)
> > +{
> > +if ( (pdev->seg == seg) &&
> > + (pdev->bus == bus) &&
> > + (pdev->devfn == devfn) )
> > +{
> > +ASSERT(pdev->domain);
> > +list_del(&pdev->domain_list);
> > +pdev->domain = NULL;
> > +pci_hide_existing_device(pdev);
> > +break;
> > +}
> > +}
>
> A loop like this is of course not ideal (especially for Dom0, which may have
> many devices). And I wonder why you, ...
>
> > @@ -134,8 +133,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16
> did,
> > /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
> > sbit = 1;
> > addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFF;
> > -rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> > - sid, sbit, addr);
> > +rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> > did,
> > + pdev->seg, pdev->bus,
> > pdev->devfn,
> > + sbit, addr);
> > break;
> > case DMA_TLB_PSI_FLUSH:
> > if ( !device_in_domain(iommu, pdev, did) ) @@ -154,8
> > +154,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
> > addr |= (((u64)1 << (size_order - 1)) - 1) <<
> > PAGE_SHIFT_4K;
> > }
> >
> > -rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> > - sid, sbit, addr);
> > +rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> > did,
> > + pdev->seg, pdev->bus,
> > pdev->devfn,
> > + sbit, addr);
> > break;
>
> ... holding pdev in your hands here, don't just pass it down (which at once
> would make the function signature less convoluted: you could even eliminate
> the currently 2nd parameter that way).
>
Good idea!!
Quan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v10 3/3] vt-d: fix vt-d Device-TLB flush timeout issue
>>> On 22.04.16 at 12:54, wrote:
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -206,10 +206,71 @@ static int invalidate_sync(struct iommu *iommu)
> return 0;
> }
>
> +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
> + u16 seg, u8 bus, u8 devfn)
> +{
> +struct domain *d = NULL;
> +struct pci_dev *pdev;
> +
> +if ( test_bit(did, iommu->domid_bitmap) )
> +d = rcu_lock_domain_by_id(iommu->domid_map[did]);
> +
> +/*
> + * In case the domain has been freed or the IOMMU domid bitmap is
> + * not valid, the device no longer belongs to this domain.
> + */
> +if ( d == NULL )
> +return;
> +
> +pcidevs_lock();
> +
> +for_each_pdev(d, pdev)
> +{
> +if ( (pdev->seg == seg) &&
> + (pdev->bus == bus) &&
> + (pdev->devfn == devfn) )
> +{
> +ASSERT(pdev->domain);
> +list_del(&pdev->domain_list);
> +pdev->domain = NULL;
> +pci_hide_existing_device(pdev);
> +break;
> +}
> +}
A loop like this is of course not ideal (especially for Dom0, which may
have many devices). And I wonder why you, ...
> @@ -134,8 +133,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
> /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
> sbit = 1;
> addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFF;
> -rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> - sid, sbit, addr);
> +rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, did,
> + pdev->seg, pdev->bus, pdev->devfn,
> + sbit, addr);
> break;
> case DMA_TLB_PSI_FLUSH:
> if ( !device_in_domain(iommu, pdev, did) )
> @@ -154,8 +154,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
> addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
> }
>
> -rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> - sid, sbit, addr);
> +rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, did,
> + pdev->seg, pdev->bus, pdev->devfn,
> + sbit, addr);
> break;
... holding pdev in your hands here, don't just pass it down (which
at once would make the function signature less convoluted: you
could even eliminate the currently 2nd parameter that way).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v10 3/3] vt-d: fix vt-d Device-TLB flush timeout issue
If Device-TLB flush timed out, we hide the target ATS device
immediately and crash the domain owning this ATS device. If
impacted domain is hardware domain, just throw out a warning.
By hiding the device, we make sure it can't be assigned to any
domain any longer (see device_assigned).
Signed-off-by: Quan Xu
---
xen/drivers/passthrough/pci.c | 6 +--
xen/drivers/passthrough/vtd/extern.h | 5 ++-
xen/drivers/passthrough/vtd/qinval.c | 71 ---
xen/drivers/passthrough/vtd/x86/ats.c | 11 +++---
xen/include/xen/pci.h | 1 +
5 files changed, 79 insertions(+), 15 deletions(-)
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 9f1716a..9a214c6 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -420,7 +420,7 @@ static void free_pdev(struct pci_seg *pseg, struct pci_dev
*pdev)
xfree(pdev);
}
-static void _pci_hide_device(struct pci_dev *pdev)
+void pci_hide_existing_device(struct pci_dev *pdev)
{
if ( pdev->domain )
return;
@@ -437,7 +437,7 @@ int __init pci_hide_device(int bus, int devfn)
pdev = alloc_pdev(get_pseg(0), bus, devfn);
if ( pdev )
{
-_pci_hide_device(pdev);
+pci_hide_existing_device(pdev);
rc = 0;
}
pcidevs_unlock();
@@ -467,7 +467,7 @@ int __init pci_ro_device(int seg, int bus, int devfn)
}
__set_bit(PCI_BDF2(bus, devfn), pseg->ro_map);
-_pci_hide_device(pdev);
+pci_hide_existing_device(pdev);
return 0;
}
diff --git a/xen/drivers/passthrough/vtd/extern.h
b/xen/drivers/passthrough/vtd/extern.h
index ab7ecad..b54a15c 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -60,8 +60,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
u64 addr, unsigned int size_order, u64 type);
int __must_check qinval_device_iotlb_sync(struct iommu *iommu,
- u32 max_invs_pend,
- u16 sid, u16 size, u64 addr);
+ u32 max_invs_pend, u16 did,
+ u16 seg, u8 bus, u8 devfn,
+ u16 size, u64 addr);
unsigned int get_cache_line_size(void);
void cacheline_flush(char *);
diff --git a/xen/drivers/passthrough/vtd/qinval.c
b/xen/drivers/passthrough/vtd/qinval.c
index 69cc6bf..c795e6b 100644
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -206,10 +206,71 @@ static int invalidate_sync(struct iommu *iommu)
return 0;
}
+static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
+ u16 seg, u8 bus, u8 devfn)
+{
+struct domain *d = NULL;
+struct pci_dev *pdev;
+
+if ( test_bit(did, iommu->domid_bitmap) )
+d = rcu_lock_domain_by_id(iommu->domid_map[did]);
+
+/*
+ * In case the domain has been freed or the IOMMU domid bitmap is
+ * not valid, the device no longer belongs to this domain.
+ */
+if ( d == NULL )
+return;
+
+pcidevs_lock();
+
+for_each_pdev(d, pdev)
+{
+if ( (pdev->seg == seg) &&
+ (pdev->bus == bus) &&
+ (pdev->devfn == devfn) )
+{
+ASSERT(pdev->domain);
+list_del(&pdev->domain_list);
+pdev->domain = NULL;
+pci_hide_existing_device(pdev);
+break;
+}
+}
+
+pcidevs_unlock();
+
+if ( !is_hardware_domain(d) )
+domain_crash(d);
+else
+printk(XENLOG_WARNING VTDPREFIX
+ " dom%d: ATS device %04x:%02x:%02x.%u flush failed.\n",
+ d->domain_id,
+ seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
+
+rcu_unlock_domain(d);
+}
+
+int dev_invalidate_sync(struct iommu *iommu, u16 did,
+u16 seg, u8 bus, u8 devfn)
+{
+struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
+int rc = 0;
+
+if ( qi_ctrl->qinval_maddr )
+{
+rc = queue_invalidate_wait(iommu, 0, 1, 1);
+if ( rc == -ETIMEDOUT )
+dev_invalidate_iotlb_timeout(iommu, did, seg, bus, devfn);
+}
+
+return rc;
+}
+
int __must_check qinval_device_iotlb_sync(struct iommu *iommu,
- u32 max_invs_pend,
- u16 sid, u16 size,
- u64 addr)
+ u32 max_invs_pend, u16 did,
+ u16 seg, u8 bus, u8 devfn,
+ u16 size, u64 addr)
{
unsigned long flags;
unsigned int index;
@@ -227,7 +288,7 @@ int __must_check qinval_device_iotlb_sync(struct iommu
*iommu,
qinval_entry->q.dev_iotlb_inv_dsc.lo.res_1 = 0;
qinval_entry->q.dev_iotlb_inv_dsc.lo.max_in
