| Message ID | 1458799079-79825-4-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote: > If Device-TLB flush timed out, we would hide the target ATS > device and crash the domain owning this ATS device. If impacted > domain is hardware domain, just throw out a warning. > > The hidden device should be disallowed to be further assigned > to any domain. > What is "should be disallowed" supposed to mean here? Isn't the situation that, by hiding the device, which this patch is doing, we actually disallow any further assignment? If yes, this should rather be (something like): "By hiding the device, we make sure it can't be assigned to any domain any longer." Other than this, the patch looks good to me, but I'll re-review it when the new version comes out (with the other patches from the preliminary series folded in), before saying Reviewed-by. Regards, Dario
On March 24, 2016 11:38pm, <dario.faggioli@citrix.com> wrote: > On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote: > > If Device-TLB flush timed out, we would hide the target ATS device and > > crash the domain owning this ATS device. If impacted domain is > > hardware domain, just throw out a warning. > > > > The hidden device should be disallowed to be further assigned to any > > domain. > > > What is "should be disallowed" supposed to mean here? Isn't the situation that, > by hiding the device, which this patch is doing, we actually disallow any further > assignment? > > If yes, this should rather be (something like): > > "By hiding the device, we make sure it can't be assigned to any domain any > longer." > Let me introduce what happen first. If the ATS device flush timed out, it would be assigned to dom_xen (dummy domain). Then, .e.g, the ATS device is 0000:81:00.0 in my platform. If the ATS device 0000:81:00.0 is hidden and we assign the ATS device 0000:81:00.0 to another domain with xl tool, It would fail and print out: """ libxl: error: libxl_pci.c:1216:libxl__device_pci_add: PCI device 0000:81:00.0 already assigned to a different guest? """ To be honest, I'd prefer your description. do you think? Quan > Other than this, the patch looks good to me, but I'll re-review it when the new > version comes out (with the other patches from the preliminary series folded in), > before saying Reviewed-by.
On Thu, Mar 24, 2016 at 01:57:59PM +0800, Quan Xu wrote: > If Device-TLB flush timed out, we would hide the target ATS > device and crash the domain owning this ATS device. If impacted > domain is hardware domain, just throw out a warning. > > The hidden device should be disallowed to be further assigned > to any domain. > > Signed-off-by: Quan Xu <quan.xu@intel.com> > --- > xen/drivers/passthrough/pci.c | 6 ++-- > xen/drivers/passthrough/vtd/extern.h | 3 +- > xen/drivers/passthrough/vtd/qinval.c | 58 +++++++++++++++++++++++++++++++++-- > xen/drivers/passthrough/vtd/x86/ats.c | 11 ++++--- > xen/include/xen/pci.h | 1 + > 5 files changed, 68 insertions(+), 11 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 6d3187d..94e2c11 100644 > --- a/xen/drivers/passthrough/vtd/extern.h > +++ b/xen/drivers/passthrough/vtd/extern.h > @@ -62,7 +62,8 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did, > int qinval_device_iotlb(struct iommu *iommu, > u32 max_invs_pend, u16 sid, u16 size, u64 addr); > int qinval_device_iotlb_sync(struct iommu *iommu, u32 max_invs_pend, > - u16 sid, u16 size, u64 addr); > + 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 ad9e265..10c5684 100644 > --- a/xen/drivers/passthrough/vtd/qinval.c > +++ b/xen/drivers/passthrough/vtd/qinval.c > @@ -216,6 +216,58 @@ static int queue_invalidate_iotlb_sync(struct iommu *iommu, > return invalidate_sync(iommu); > } > > +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]); > + > + if ( d == NULL ) > + return; > + > + pcidevs_lock(); > + > + for_each_pdev(d, pdev) > + { > + if ( ( pdev->seg == seg ) && > + ( pdev->bus == bus ) && > + ( pdev->devfn == devfn ) ) > + { > + ASSERT ( pdev->domain ); Oddly enough (and I don't see this in the StyleGuide), the ASSERTS do not require the spaces. So it can be: 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); The description said something about 'just throw out a warning' (if the domain owning it is a hardware domain). That seems to be missing?
On Thu, Mar 24, 2016 at 04:38:05PM +0100, Dario Faggioli wrote: > On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote: > > If Device-TLB flush timed out, we would hide the target ATS > > device and crash the domain owning this ATS device. If impacted > > domain is hardware domain, just throw out a warning. > > > > The hidden device should be disallowed to be further assigned > > to any domain. > > > What is "should be disallowed" supposed to mean here? Isn't the > situation that, by hiding the device, which this patch is doing, we > actually disallow any further assignment? Yes. Take a look at device_assigned. This patch reassigns the device to dom_xen so device_assigned will return -EBUSY. Actually that information could be part of the commit to get an idea of the effects of this patch. > > If yes, this should rather be (something like): > > "By hiding the device, we make sure it can't be assigned to any domain > any longer." > > Other than this, the patch looks good to me, but I'll re-review it when > the new version comes out (with the other patches from the preliminary > series folded in), before saying Reviewed-by. > > Regards, > Dario > -- > <<This happens because I choose it to happen!>> (Raistlin Majere) > ----------------------------------------------------------------- > Dario Faggioli, Ph.D, http://about.me/dario.faggioli > Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On March 26, 2016 4:40am, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Thu, Mar 24, 2016 at 04:38:05PM +0100, Dario Faggioli wrote: > > On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote: > > > If Device-TLB flush timed out, we would hide the target ATS device > > > and crash the domain owning this ATS device. If impacted domain is > > > hardware domain, just throw out a warning. > > > > > > The hidden device should be disallowed to be further assigned to any > > > domain. > > > > > What is "should be disallowed" supposed to mean here? Isn't the > > situation that, by hiding the device, which this patch is doing, we > > actually disallow any further assignment? > > Yes. > > Take a look at device_assigned. This patch reassigns the device to dom_xen so > device_assigned will return -EBUSY. > > Actually that information could be part of the commit to get an idea of the > effects of this patch. > > Konrad, Welcome. Yes, I would try to add it in changelog. Quan > > If yes, this should rather be (something like): > > > > "By hiding the device, we make sure it can't be assigned to any domain > > any longer." > > > > Other than this, the patch looks good to me, but I'll re-review it > > when the new version comes out (with the other patches from the > > preliminary series folded in), before saying Reviewed-by. > > > > Regards, > > Dario > > -- > > <<This happens because I choose it to happen!>> (Raistlin Majere) > > ----------------------------------------------------------------- > > Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software > > Engineer, Citrix Systems R&D Ltd., Cambridge (UK) > > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On March 26, 2016 4:32am, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Thu, Mar 24, 2016 at 01:57:59PM +0800, Quan Xu wrote: > > If Device-TLB flush timed out, we would hide the target ATS device and > > crash the domain owning this ATS device. If impacted domain is > > hardware domain, just throw out a warning. > > > > The hidden device should be disallowed to be further assigned to any > > domain. > > > > Signed-off-by: Quan Xu <quan.xu@intel.com> > > --- > > xen/drivers/passthrough/pci.c | 6 ++-- > > xen/drivers/passthrough/vtd/extern.h | 3 +- > > xen/drivers/passthrough/vtd/qinval.c | 58 > > +++++++++++++++++++++++++++++++++-- > > xen/drivers/passthrough/vtd/x86/ats.c | 11 ++++--- > > xen/include/xen/pci.h | 1 + > > 5 files changed, 68 insertions(+), 11 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 6d3187d..94e2c11 100644 > > --- a/xen/drivers/passthrough/vtd/extern.h > > +++ b/xen/drivers/passthrough/vtd/extern.h > > @@ -62,7 +62,8 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 > > did, int qinval_device_iotlb(struct iommu *iommu, > > u32 max_invs_pend, u16 sid, u16 size, u64 > > addr); int qinval_device_iotlb_sync(struct iommu *iommu, u32 > max_invs_pend, > > - u16 sid, u16 size, u64 addr); > > + 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 ad9e265..10c5684 100644 > > --- a/xen/drivers/passthrough/vtd/qinval.c > > +++ b/xen/drivers/passthrough/vtd/qinval.c > > @@ -216,6 +216,58 @@ static int queue_invalidate_iotlb_sync(struct iommu > *iommu, > > return invalidate_sync(iommu); > > } > > > > +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]); > > + > > + if ( d == NULL ) > > + return; > > + > > + pcidevs_lock(); > > + > > + for_each_pdev(d, pdev) > > + { > > + if ( ( pdev->seg == seg ) && > > + ( pdev->bus == bus ) && > > + ( pdev->devfn == devfn ) ) > > + { > > + ASSERT ( pdev->domain ); > > Oddly enough (and I don't see this in the StyleGuide), the ASSERTS do not > require the spaces. So it can be: > > ASSERT(pdev->domain); Got it, I will fix it in next v9. > > + list_del(&pdev->domain_list); > > + pdev->domain = NULL; > > + pci_hide_existing_device(pdev); > > + break; > > + } > > + } > > + > > + pcidevs_unlock(); > > + > > + if ( !is_hardware_domain(d) ) > > + domain_crash(d); > > The description said something about 'just throw out a warning' (if the domain > owning it is a hardware domain). That seems to be missing? > > The warning is in the call path, in queue_invalidate_wait(): """Queue invalidate wait descriptor timed out.""" Then, does it make sense? Quan
+cc Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, who is also reviewing this patch. On March 24, 2016 11:38pm, Dario Faggioli <dario.faggioli@citrix.com> wrote: > On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote: > > If Device-TLB flush timed out, we would hide the target ATS device and > > crash the domain owning this ATS device. If impacted domain is > > hardware domain, just throw out a warning. > > > > The hidden device should be disallowed to be further assigned to any > > domain. > > > What is "should be disallowed" supposed to mean here? Isn't the situation that, > by hiding the device, which this patch is doing, we actually disallow any further > assignment? > > If yes, this should rather be (something like): > > "By hiding the device, we make sure it can't be assigned to any domain any > longer." > > Other than this, the patch looks good to me, but I'll re-review it when the new > version comes out (with the other patches from the preliminary series folded in), > before saying Reviewed-by. Dario, What about this one: """ VT-d: Fix vt-d Device-TLB flush timeout issue If Device-TLB flush timed out, we would hide the target ATS device and crash the domain owning this ATS device. If impacted domain is hardware domain, the error handling is just ignored. By hiding the device, we make sure it can't be assigned to any domain any longer. """ Quan
> > > > + list_del(&pdev->domain_list); > > > + pdev->domain = NULL; > > > + pci_hide_existing_device(pdev); > > > + break; > > > + } > > > + } > > > + > > > + pcidevs_unlock(); > > > + > > > + if ( !is_hardware_domain(d) ) > > > + domain_crash(d); > > > > The description said something about 'just throw out a warning' (if the domain > > owning it is a hardware domain). That seems to be missing? > > > > > > The warning is in the call path, in queue_invalidate_wait(): > """Queue invalidate wait descriptor timed out.""" Aah, right. > > Then, does it make sense? Yes. I would recommend you modify the commit description so that clueless folks like me can see it. You could modify the commit description to say: "just throw out a warning (done in queue_invalidate_wait)." > > Quan
On March 28, 2016 10:11pm, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > > > > > + list_del(&pdev->domain_list); > > > > + pdev->domain = NULL; > > > > + pci_hide_existing_device(pdev); > > > > + break; > > > > + } > > > > + } > > > > + > > > > + pcidevs_unlock(); > > > > + > > > > + if ( !is_hardware_domain(d) ) > > > > + domain_crash(d); > > > > > > The description said something about 'just throw out a warning' (if > > > the domain owning it is a hardware domain). That seems to be missing? > > > > > > > > > > The warning is in the call path, in queue_invalidate_wait(): > > """Queue invalidate wait descriptor timed out.""" > > Aah, right. > > > > Then, does it make sense? > > Yes. I would recommend you modify the commit description so that clueless > folks like me can see it. You could modify the commit description to say: > > "just throw out a warning (done in queue_invalidate_wait)." > > Then, based on Dario/your suggestion, the changelog could be: """ VT-d: Fix vt-d Device-TLB flush timeout issue If Device-TLB flush timed out, we would hide the target ATS device and crash the domain owning this ATS device. If impacted domain is hardware domain, just throw out a warning (done in queue_invalidate_wait). By hiding the device, we make sure it can't be assigned to any domain any longer. """ Quan
On Tue, Mar 29, 2016 at 01:32:02AM +0000, Xu, Quan wrote: > On March 28, 2016 10:11pm, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > > > > > > > + list_del(&pdev->domain_list); > > > > > + pdev->domain = NULL; > > > > > + pci_hide_existing_device(pdev); > > > > > + break; > > > > > + } > > > > > + } > > > > > + > > > > > + pcidevs_unlock(); > > > > > + > > > > > + if ( !is_hardware_domain(d) ) > > > > > + domain_crash(d); > > > > > > > > The description said something about 'just throw out a warning' (if > > > > the domain owning it is a hardware domain). That seems to be missing? > > > > > > > > > > > > > > The warning is in the call path, in queue_invalidate_wait(): > > > """Queue invalidate wait descriptor timed out.""" > > > > Aah, right. > > > > > > Then, does it make sense? > > > > Yes. I would recommend you modify the commit description so that clueless > > folks like me can see it. You could modify the commit description to say: > > > > "just throw out a warning (done in queue_invalidate_wait)." > > > > > > Then, based on Dario/your suggestion, the changelog could be: > """ > VT-d: Fix vt-d Device-TLB flush timeout issue > > If Device-TLB flush timed out, we would hide the target ATS device and crash the domain owning this ATS device. > If impacted domain is hardware domain, just throw out a warning (done in queue_invalidate_wait). > > By hiding the device, we make sure it can't be assigned to any domain any longer. > """ s/to any domain any longer./to any domain any longer (see device_assigned)./
On March 29, 2016 10:21pm, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Tue, Mar 29, 2016 at 01:32:02AM +0000, Xu, Quan wrote: > > On March 28, 2016 10:11pm, Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com> wrote: > > > > > > > > > > + list_del(&pdev->domain_list); > > > > > > + pdev->domain = NULL; > > > > > > + pci_hide_existing_device(pdev); > > > > > > + break; > > > > > > + } > > > > > > + } > > > > > > + > > > > > > + pcidevs_unlock(); > > > > > > + > > > > > > + if ( !is_hardware_domain(d) ) > > > > > > + domain_crash(d); > > > > > > > > > > The description said something about 'just throw out a warning' > > > > > (if the domain owning it is a hardware domain). That seems to be > missing? > > > > > > > > > > > > > > > > > > The warning is in the call path, in queue_invalidate_wait(): > > > > """Queue invalidate wait descriptor timed out.""" > > > > > > Aah, right. > > > > > > > > Then, does it make sense? > > > > > > Yes. I would recommend you modify the commit description so that > > > clueless folks like me can see it. You could modify the commit description to > say: > > > > > > "just throw out a warning (done in queue_invalidate_wait)." > > > > > > > > > > Then, based on Dario/your suggestion, the changelog could be: > > """ > > VT-d: Fix vt-d Device-TLB flush timeout issue > > > > If Device-TLB flush timed out, we would hide the target ATS device and crash > the domain owning this ATS device. > > If impacted domain is hardware domain, just throw out a warning (done in > queue_invalidate_wait). > > > > By hiding the device, we make sure it can't be assigned to any domain any > longer. > > """ > > s/to any domain any longer./to any domain any longer (see device_assigned)./ > Got it, thanks. Quan
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 6d3187d..94e2c11 100644 --- a/xen/drivers/passthrough/vtd/extern.h +++ b/xen/drivers/passthrough/vtd/extern.h @@ -62,7 +62,8 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did, int qinval_device_iotlb(struct iommu *iommu, u32 max_invs_pend, u16 sid, u16 size, u64 addr); int qinval_device_iotlb_sync(struct iommu *iommu, u32 max_invs_pend, - u16 sid, u16 size, u64 addr); + 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 ad9e265..10c5684 100644 --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -216,6 +216,58 @@ static int queue_invalidate_iotlb_sync(struct iommu *iommu, return invalidate_sync(iommu); } +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]); + + 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); + + 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 qinval_device_iotlb(struct iommu *iommu, u32 max_invs_pend, u16 sid, u16 size, u64 addr) { @@ -250,11 +302,13 @@ int qinval_device_iotlb(struct iommu *iommu, } int 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) { + u16 sid = PCI_BDF2(bus, devfn); + qinval_device_iotlb(iommu, max_invs_pend, sid, size, addr); - return invalidate_sync(iommu); + return dev_invalidate_sync(iommu, did, seg, bus, devfn); } static void queue_invalidate_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx) diff --git a/xen/drivers/passthrough/vtd/x86/ats.c b/xen/drivers/passthrough/vtd/x86/ats.c index 7b1c07b..5d1ebea 100644 --- a/xen/drivers/passthrough/vtd/x86/ats.c +++ b/xen/drivers/passthrough/vtd/x86/ats.c @@ -116,7 +116,6 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did, list_for_each_entry( pdev, &ats_devices, list ) { - u16 sid = PCI_BDF2(pdev->bus, pdev->devfn); bool_t sbit; /* Only invalidate devices that belong to this IOMMU */ @@ -133,8 +132,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) & 0x7FFFFFFFFFFFFFFF; - ret = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, - sid, sbit, addr); + ret = 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) ) @@ -153,8 +153,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did, addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K; } - ret = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, - sid, sbit, addr); + ret = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, did, + pdev->seg, pdev->bus, pdev->devfn, + sbit, addr); break; default: dprintk(XENLOG_WARNING VTDPREFIX, "invalid vt-d flush type\n"); diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index 6ed29dd..bb9f791 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -116,6 +116,7 @@ const unsigned long *pci_get_ro_map(u16 seg); int pci_add_device(u16 seg, u8 bus, u8 devfn, const struct pci_dev_info *, nodeid_t node); int pci_remove_device(u16 seg, u8 bus, u8 devfn); +void pci_hide_existing_device(struct pci_dev *pdev); int pci_ro_device(int seg, int bus, int devfn); int pci_hide_device(int bus, int devfn); struct pci_dev *pci_get_pdev(int seg, int bus, int devfn);
If Device-TLB flush timed out, we would hide the target ATS device and crash the domain owning this ATS device. If impacted domain is hardware domain, just throw out a warning. The hidden device should be disallowed to be further assigned to any domain. Signed-off-by: Quan Xu <quan.xu@intel.com> --- xen/drivers/passthrough/pci.c | 6 ++-- xen/drivers/passthrough/vtd/extern.h | 3 +- xen/drivers/passthrough/vtd/qinval.c | 58 +++++++++++++++++++++++++++++++++-- xen/drivers/passthrough/vtd/x86/ats.c | 11 ++++--- xen/include/xen/pci.h | 1 + 5 files changed, 68 insertions(+), 11 deletions(-)