Re: [Xen-devel] [PATCH v8 3/3] VT-d: Fix vt-d Device-TLB flush timeout issue

["Xu, Quan" <quan.xu@xxxxxxxxx>]

On March 26, 2016 4:32am, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel