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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Mar 28, 2016 at 06:27:21AM +0000, Xu, Quan wrote:
> 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:".

But that goodness never gets documented in the code..

> IMO, it is not a good idea to explain in this commit description, as I don't touch it.  
> Right?

Why?

Put yourself in the shoes of a an engineer who wants to fix or add some code.

He or she will look at the code and say: Ok, they added __mustcheck so they
MUST have thought it is important to check the return code. But then tracking
through the different users and then it is ignored two or three levels up.

Huh? Why didn't the account for that?

If you include that information in the description it will save them further
work from having to guess or email you.

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

In which case I would just say that:

"The followup patch titled XYZ addresses the __mustcheck. That is the other callers
of this routine (two or three levels up) ignore the return code. This patch does
not address this but the other does."

And that 
> 
> > 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.

<grins>

I would recommend you send out an email to xen-devel with [BRAINSTORM] or such to explain
the problem or ways to make it work. That way we can all benefit from your hard digging
in the code and untangling it.

Or if you can - be at the Xen Hackathon in April to brainstorm this in person?

> 
> 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