From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Xu, Quan" Subject: Re: [PATCH v2 2/2] VT-d: Fix vt-d flush timeout issue. Date: Fri, 11 Dec 2015 08:01:14 +0000 Message-ID: <945CA011AD5F084CBEA3E851C0AB28894AE59310@SHSMSX103.ccr.corp.intel.com> References: <1449739990-66155-1-git-send-email-quan.xu@intel.com> <1449739990-66155-3-git-send-email-quan.xu@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Tian, Kevin" , "jbeulich@suse.com" Cc: "Wu, Feng" , "Dong, Eddie" , "george.dunlap@eu.citrix.com" , "andrew.cooper3@citrix.com" , "tim@xen.org" , "xen-devel@lists.xen.org" , "Nakajima, Jun" , "keir@xen.org" List-Id: xen-devel@lists.xenproject.org On 11.12.2015 at 3:28pm, 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 > > --- > [...] > > 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