Re: [PATCH v8 1/3] VT-d: Reduce spin timeout to 1ms, which can be boot-time changed

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

On March 26, 2016 4:07am, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Thu, Mar 24, 2016 at 01:57:58PM +0800, Quan Xu wrote:
> 
> Hey!
> 
> Thanks for the patch!
> 
> I see that you have __must_check..
> 
> But if you check the callchain:
> 
> iommu_flush_iec_[index|global] ->
>     __iommu_flush_iec->invalidate_sync-> queue_invalidate_wait
> 
> you will see that the callers of iommu_flush_iec_[index|global] ignore the
> return value.
> 
> So ... perhaps you could explain in this commit description on how to address
> that?

I mentioned it in 0000-cover-letter.patch -- "Not covered in this series:".
IMO, it is not a good idea to explain in this commit description, as I don't touch it.  
Right?

> Is there an followup patch (if so just put in the name in here) to address
> that?
> 
Yes,

> Or should the top callers: enable_intremap, ioapic_rte_to_remap_entry,
> free_remap_entry, msi_msg_to_remap_entry, and pi_update_irte do
> something?
> 

I'd prefer to discuss about it in another thread. btw, I have spent a lot of time on 
Interrupt research, including the logical topology of interrupt hardware/software, .e.g, lapci/vlapic/ioapic/vioapic/msi/PI.
In last months, I wondered whether I could disable the interrupt remapping dynamically( by 'iommu_intremap = 0') or not.
Now I think it would be insecure.
IMO, it is not a good time to discuss this new topic, otherwise it makes me exhausted.

Quan

> I guess the 'free_remap_entry' reall can't. As you are suppose to always be able
> to free something.
> 
> 
> msi_msg_to_remap_entry, _msg_to_remap_entry, and
> ioapic_rte_to_remap_entry could return an value... Or considering this is v8 -
> was there some epic conversation that went over this quite a lot? In which case
> I would recommend you say why it was not attempted this way so that folks six
> months from now when reading this patch won't ask again.
> 
> 
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > ---
> >  docs/misc/xen-command-line.markdown  |  7 +++++++
> > xen/drivers/passthrough/vtd/qinval.c | 17 ++++++++++++-----
> >  2 files changed, 19 insertions(+), 5 deletions(-)
> >
> > 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..52ba2c2 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,10 @@ static void queue_invalidate_iotlb(struct iommu
> *iommu,
> >      spin_unlock_irqrestore(&iommu->register_lock, flags);  }
> >
> > -static int queue_invalidate_wait(struct iommu *iommu,
> > +static int __must_check 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 +169,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();
> >          }
> > --
> > 1.9.1
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel