| Message ID | 1458198767-61293-2-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> From: Xu, Quan > Sent: Thursday, March 17, 2016 3:13 PM > diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c > index b81b0bd..37a15fb 100644 > --- a/xen/drivers/passthrough/vtd/qinval.c > +++ b/xen/drivers/passthrough/vtd/qinval.c > @@ -28,6 +28,11 @@ > #include "vtd.h" > #include "extern.h" > > +static unsigned int __read_mostly vtd_qi_timeout = 1; > +integer_param("vtd_qi_timeout", vtd_qi_timeout); > + > +#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) > + > static void print_qi_regs(struct iommu *iommu) > { > u64 val; > @@ -130,10 +135,14 @@ static void queue_invalidate_iotlb(struct iommu *iommu, > spin_unlock_irqrestore(&iommu->register_lock, flags); > } > > +/* > + * NB. We must check all kinds of error and all the way up the > + * call trees. > + */ This comment is meaningless put in the code. It just reflects what you need do to push the whole patch series, but it's obvious a straightforward requirement. > static int queue_invalidate_wait(struct iommu *iommu, > u8 iflag, u8 sw, u8 fn) > { Thanks Kevin
On March 17, 2016 3:45pm, Tian, Kevin <kevin.tian@intel.com> wrote: > > From: Xu, Quan > > Sent: Thursday, March 17, 2016 3:13 PM diff --git > > a/xen/drivers/passthrough/vtd/qinval.c > > b/xen/drivers/passthrough/vtd/qinval.c > > index b81b0bd..37a15fb 100644 > > --- a/xen/drivers/passthrough/vtd/qinval.c > > +++ b/xen/drivers/passthrough/vtd/qinval.c > > @@ -28,6 +28,11 @@ > > #include "vtd.h" > > #include "extern.h" > > > > +static unsigned int __read_mostly vtd_qi_timeout = 1; > > +integer_param("vtd_qi_timeout", vtd_qi_timeout); > > + > > +#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) > > + > > static void print_qi_regs(struct iommu *iommu) { > > u64 val; > > @@ -130,10 +135,14 @@ static void queue_invalidate_iotlb(struct iommu > *iommu, > > spin_unlock_irqrestore(&iommu->register_lock, flags); } > > > > +/* > > + * NB. We must check all kinds of error and all the way up the > > + * call trees. > > + */ > > This comment is meaningless put in the code. It just reflects what you need do > to push the whole patch series, but it's obvious a straightforward requirement. > Kevin, This is for Jan requirement: http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg02388.html Jan said: """ Without a __must_check annotation on the function I cannot see how I should reasonably convince myself that all call sites now handle such an error in one way or another.""" Now I think I misunderstood what Jan said. It may be: + static int __must_check queue_invalidate_wait(struct iommu *iommu, u8 iflag, u8 sw, u8 fn) I found that there is a '__must_check' in xen code. Quan > > static int queue_invalidate_wait(struct iommu *iommu, > > u8 iflag, u8 sw, u8 fn) > > {
>>> On 17.03.16 at 09:11, <quan.xu@intel.com> wrote: > On March 17, 2016 3:45pm, Tian, Kevin <kevin.tian@intel.com> wrote: >> > From: Xu, Quan >> > Sent: Thursday, March 17, 2016 3:13 PM diff --git >> > a/xen/drivers/passthrough/vtd/qinval.c >> > b/xen/drivers/passthrough/vtd/qinval.c >> > index b81b0bd..37a15fb 100644 >> > --- a/xen/drivers/passthrough/vtd/qinval.c >> > +++ b/xen/drivers/passthrough/vtd/qinval.c >> > @@ -28,6 +28,11 @@ >> > #include "vtd.h" >> > #include "extern.h" >> > >> > +static unsigned int __read_mostly vtd_qi_timeout = 1; >> > +integer_param("vtd_qi_timeout", vtd_qi_timeout); >> > + >> > +#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) >> > + >> > static void print_qi_regs(struct iommu *iommu) { >> > u64 val; >> > @@ -130,10 +135,14 @@ static void queue_invalidate_iotlb(struct iommu >> *iommu, >> > spin_unlock_irqrestore(&iommu->register_lock, flags); } >> > >> > +/* >> > + * NB. We must check all kinds of error and all the way up the >> > + * call trees. >> > + */ >> >> This comment is meaningless put in the code. It just reflects what you need > do >> to push the whole patch series, but it's obvious a straightforward > requirement. >> > Kevin, > This is for Jan requirement: > > http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg02388.html > Jan said: > """ Without a __must_check annotation on the function I cannot see > how I should reasonably convince myself that all call sites now > handle such an error in one way or another.""" > > Now I think I misunderstood what Jan said. It may be: > > + static int __must_check queue_invalidate_wait(struct iommu *iommu, u8 > iflag, u8 sw, u8 fn) > > I found that there is a '__must_check' in xen code. Indeed that's what I meant. Jan
On March 17, 2016 4:14pm, <JBeulich@suse.com> wrote: > >>> On 17.03.16 at 09:11, <quan.xu@intel.com> wrote: > > On March 17, 2016 3:45pm, Tian, Kevin <kevin.tian@intel.com> wrote: > >> > From: Xu, Quan > >> > Sent: Thursday, March 17, 2016 3:13 PM diff --git > >> > a/xen/drivers/passthrough/vtd/qinval.c > >> > b/xen/drivers/passthrough/vtd/qinval.c > >> > index b81b0bd..37a15fb 100644 > >> > --- a/xen/drivers/passthrough/vtd/qinval.c > >> > +++ b/xen/drivers/passthrough/vtd/qinval.c > >> > @@ -28,6 +28,11 @@ > >> > #include "vtd.h" > >> > #include "extern.h" > >> > > >> > +static unsigned int __read_mostly vtd_qi_timeout = 1; > >> > +integer_param("vtd_qi_timeout", vtd_qi_timeout); > >> > + > >> > +#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) > >> > + > >> > static void print_qi_regs(struct iommu *iommu) { > >> > u64 val; > >> > @@ -130,10 +135,14 @@ static void queue_invalidate_iotlb(struct > >> > iommu > >> *iommu, > >> > spin_unlock_irqrestore(&iommu->register_lock, flags); } > >> > > >> > +/* > >> > + * NB. We must check all kinds of error and all the way up the > >> > + * call trees. > >> > + */ > >> > >> This comment is meaningless put in the code. It just reflects what > >> you need > > do > >> to push the whole patch series, but it's obvious a straightforward > > requirement. > >> > > Kevin, > > This is for Jan requirement: > > > > http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg02388.h > > tml > > Jan said: > > """ Without a __must_check annotation on the function I cannot see how > > I should reasonably convince myself that all call sites now handle > > such an error in one way or another.""" > > > > Now I think I misunderstood what Jan said. It may be: > > > > + static int __must_check queue_invalidate_wait(struct iommu *iommu, > > + u8 > > iflag, u8 sw, u8 fn) > > > > I found that there is a '__must_check' in xen code. > > Indeed that's what I meant. > Sorry:(.. I will fix it next v8. Quan
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown index ca77e3b..384dde7 100644 --- a/docs/misc/xen-command-line.markdown +++ b/docs/misc/xen-command-line.markdown @@ -1532,6 +1532,13 @@ Note that if **watchdog** option is also specified vpmu will be turned off. As the virtualisation is not 100% safe, don't use the vpmu flag on production systems (see http://xenbits.xen.org/xsa/advisory-163.html)! +### vtd\_qi\_timeout (VT-d) +> `= <integer>` + +> Default: `1` + +Specify the timeout of the VT-d Queued Invalidation in milliseconds. + ### watchdog > `= force | <boolean>` diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c index b81b0bd..37a15fb 100644 --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -28,6 +28,11 @@ #include "vtd.h" #include "extern.h" +static unsigned int __read_mostly vtd_qi_timeout = 1; +integer_param("vtd_qi_timeout", vtd_qi_timeout); + +#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) + static void print_qi_regs(struct iommu *iommu) { u64 val; @@ -130,10 +135,14 @@ static void queue_invalidate_iotlb(struct iommu *iommu, spin_unlock_irqrestore(&iommu->register_lock, flags); } +/* + * NB. We must check all kinds of error and all the way up the + * call trees. + */ static int queue_invalidate_wait(struct iommu *iommu, u8 iflag, u8 sw, u8 fn) { - s_time_t start_time; + s_time_t timeout; volatile u32 poll_slot = QINVAL_STAT_INIT; unsigned int index; unsigned long flags; @@ -164,13 +173,15 @@ static int queue_invalidate_wait(struct iommu *iommu, if ( sw ) { /* In case all wait descriptor writes to same addr with same data */ - start_time = NOW(); + timeout = NOW() + IOMMU_QI_TIMEOUT; while ( poll_slot != QINVAL_STAT_DONE ) { - if ( NOW() > (start_time + DMAR_OPERATION_TIMEOUT) ) + if ( NOW() > timeout ) { print_qi_regs(iommu); - panic("queue invalidate wait descriptor was not executed"); + printk(XENLOG_WARNING VTDPREFIX + "Queue invalidate wait descriptor timed out.\n"); + return -ETIMEDOUT; } cpu_relax(); }
Signed-off-by: Quan Xu <quan.xu@intel.com> --- docs/misc/xen-command-line.markdown | 7 +++++++ xen/drivers/passthrough/vtd/qinval.c | 19 +++++++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-)