Re: [Xen-devel] [PATCH v2 2/2] VT-d: Fix vt-d flush timeout issue.

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

On 11.12.2015 at 3:28pm, <Tian, Kevin> wrote:
> > From: Xu, Quan
> > Sent: Thursday, December 10, 2015 5:33 PM
> >
> > If IOTLB/Context/IETC flush is timeout, we should think all devices
> > under this IOMMU cannot function correctly.
> > So for each device under this IOMMU we'll mark it as unassignable and
> > kill the domain owning the device.
> >
> > If Device-TLB flush is timeout, we'll mark the target ATS device as
> > unassignable and kill the domain owning this device.
> >
> > If impacted domain is hardware domain, just throw out a warning. It's
> > an open here whether we want to kill hardware domain (or directly
> > panic hypervisor). Comments are welcomed.
> >
> > Device marked as unassignable will be disallowed to be further
> > assigned to any domain.
> >
> > Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> > ---
> [...]
> > diff --git a/xen/drivers/passthrough/vtd/iommu.h
> > b/xen/drivers/passthrough/vtd/iommu.h
> > index ac71ed1..c3beaa6 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.h
> > +++ b/xen/drivers/passthrough/vtd/iommu.h
> > @@ -452,6 +452,11 @@ struct qinval_entry {
> >
> >  #define RESERVED_VAL        0
> >
> > +#define INVALID_DID    ((u16)~0)
> > +#define INVALID_SEG    ((u16)~0)
> > +#define INVALID_BUS    ((u8)~0)
> > +#define INVALID_DEVFN  ((u8)~0)
> > +
> 
> Are those invalid values defined by specification? 
 This is not defined by specification.

>Or if they are software
> defined, does related mgmt. code guarantee that they won't be allocated?
> 

As similar as the other Xen code, it defined invalid value with "~0". Such as:
          $#define INVALID_MFN (~0UL)
          $#define INVALID_GFN (~0UL)
          .etc

Code can't not guarantee that won't be allocated, but it can guarantee it will 
not be used when it is INVALID_*.
Any idea, how to indicate that the value is invalid?


> >  #define TYPE_INVAL_CONTEXT      0x1
> >  #define TYPE_INVAL_IOTLB        0x2
> >  #define TYPE_INVAL_DEVICE_IOTLB 0x3
> > diff --git a/xen/drivers/passthrough/vtd/qinval.c
> > b/xen/drivers/passthrough/vtd/qinval.c
> > index 990baf2..bf7f5b0 100644
> > --- a/xen/drivers/passthrough/vtd/qinval.c
> > +++ b/xen/drivers/passthrough/vtd/qinval.c
> > @@ -27,12 +27,62 @@
> >  #include "dmar.h"
> >  #include "vtd.h"
> >  #include "extern.h"
> > +#include "../ats.h"
> >
> >  static int __read_mostly iommu_qi_timeout_ms = 1;
> > integer_param("iommu_qi_timeout_ms", iommu_qi_timeout_ms);
> >
> >  #define IOMMU_QI_TIMEOUT (iommu_qi_timeout_ms * MILLISECS(1))
> >
> > +void invalidate_timeout(struct iommu *iommu, int type, u16 did,
> > +                        u16 seg, u8 bus, u8 devfn) {
[...]
> > +    case TYPE_INVAL_DEVICE_IOTLB:
> > +        d = rcu_lock_domain_by_id(iommu->domid_map[did]);
> > +        ASSERT(d);
> > +        for_each_pdev(d, pdev)
> > +            if ( (pdev->seg == seg) &&
> > +                 (pdev->bus == bus) &&
> > +                 (pdev->devfn == devfn) )
> > +                mark_pdev_unassignable(pdev);
> 
> Once found you can break the for loop immediately since BDF is unique.
> 


Good advice. Agreed.

Quan

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