Re: [Xen-devel] [PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Xu, Quan
> Sent: Friday, June 24, 2016 1:52 PM
> 
> From: Quan Xu <quan.xu@xxxxxxxxx>
> 
> 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 <quan.xu@xxxxxxxxx>
> 
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> CC: Feng Wu <feng.wu@xxxxxxxxx>
> 
> ---
> v12:
>    1. a forward declaration struct pci_ats_dev*, instead of
>       including ats.h.
>    2. eliminate the loop.
>    3. use the same logic for logging and crashing as I did in
>       other series (despite I have moved the domain crash logic
>       up to the generic IOMMU layer, I think I am better still
>       leave it as is).
>    4. enhance dev_invalidate_sync() with ASSERT().
> ---
>  xen/drivers/passthrough/pci.c         |  6 +--
>  xen/drivers/passthrough/vtd/extern.h  |  5 ++-
>  xen/drivers/passthrough/vtd/qinval.c  | 75
> +++++++++++++++++++++++++++++------
>  xen/drivers/passthrough/vtd/x86/ats.c |  9 +----
>  xen/include/xen/pci.h                 |  1 +
>  5 files changed, 71 insertions(+), 25 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 98936f55c..843dc88 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -419,7 +419,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)

Don't understand what is the meaning of "hiding existing device".
Is there case where you may want to hide a non-existing device?

>  {
>      if ( pdev->domain )
>          return;
> @@ -436,7 +436,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();
> @@ -466,7 +466,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 45357f2..efaff28 100644
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -25,6 +25,7 @@
> 
>  #define VTDPREFIX "[VT-D]"
> 
> +struct pci_ats_dev;
>  extern bool_t rwbf_quirk;
> 
>  void print_iommu_regs(struct acpi_drhd_unit *drhd);
> @@ -60,8 +61,8 @@ 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);
> +                                          struct pci_ats_dev *ats_dev,
> +                                          u16 did, 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 4492b29..e4e2771 100644
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -27,11 +27,11 @@
>  #include "dmar.h"
>  #include "vtd.h"
>  #include "extern.h"
> +#include "../ats.h"

Earlier you said:
>    1. a forward declaration struct pci_ats_dev*, instead of
>       including ats.h.

But above you still have ats.h included.

> 
>  #define VTD_QI_TIMEOUT       1
> 
> -static int __must_check invalidate_sync(struct iommu *iommu,
> -                                        bool_t flush_dev_iotlb);
> +static int __must_check invalidate_sync(struct iommu *iommu);

I don't understand the rationale behind. In earlier patch you introduce
a new parameter which is however just removed later here....

Thanks
Kevin


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel