Re: [Xen-devel] [xen-unstable test] 104131: regressions - FAIL

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Friday, January 20, 2017 7:49 PM
> 
> >>> On 18.01.17 at 11:23, <kevin.tian@xxxxxxxxx> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Wednesday, January 18, 2017 5:38 PM
> >>
> >> >>> On 18.01.17 at 05:57, <kevin.tian@xxxxxxxxx> wrote:
> >> > Attached was my earlier comment:
> >> >
> >> > --
> >> >> >>> On 20.12.16 at 06:37, <kevin.tian@xxxxxxxxx> wrote:
> >> >> >>  From: Xuquan (Quan Xu) [mailto:xuquan8@xxxxxxxxxx]
> >> >> >> Sent: Friday, December 16, 2016 5:40 PM
> >> >> >> -        if (pt_vector != -1)
> >> >> >> -            vmx_set_eoi_exit_bitmap(v, pt_vector);
> >> >> >> +        if ( pt_vector != -1 ) {
> >> >> >> +            if ( intack.vector > pt_vector )
> >> >> >> +                vmx_set_eoi_exit_bitmap(v, intack.vector);
> >> >> >> +            else
> >> >> >> +                vmx_set_eoi_exit_bitmap(v, pt_vector);
> >> >> >> +        }
> >> >> >
> >> >> > Above can be simplified as one line change:
> >> >> >       if ( pt_vector != -1 )
> >> >> >               vmx_set_eoi_exit_bitmap(v, intack.vector);
> >> >>
> >> >> Hmm, I don't understand. Did you mean to use max() here? Or
> >> >> else how is this an equivalent of the originally proposed code?
> >> >>
> >> >
> >> > Original code is not 100% correct. The purpose is to set EOI exit
> >> > bitmap for any vector which may block injection of pt_vector -
> >> > give chance to recognize pt_vector in future intack and then do pt
> >> > intr post. The simplified code achieves this effect same as original
> >> > code if intack.vector >= vector. I cannot come up a case why
> >> > intack.vector might be smaller than vector. If this case happens,
> >> > we still need enable exit bitmap for intack.vector instead of
> >> > pt_vector for said purpose while original code did it wrong.
> >> >
> >> > Thanks
> >> > Kevin
> >> > --
> >> >
> >> > Using intack.vector is always expected here regardless of the
> >> > comparison result between intack.vector and pt_vector. The
> >> > reason why I was OK adding an ASSERT was simply to test
> >> > whether intack.vecor<pt_vector does happen which is
> >> > orthogonal to the fix itself.
> >>
> >> Well, a vector lower than pt_vector can't block delivery. Or wait:
> >
> > There are two points here:
> >
> > a) We need enable EOI exit bitmap when pt_vector is blocked.
> >
> > b) As you said, ideally a vector lower than pt_vecotr cannot block
> >
> > The patch fixed a) and then added an ASSERT to verify b). Strictly
> > speaking they are separate issues.
> 
> Okay, I think I finally understand your argumentation here.
> 
> >> Don't we need to consider vector classes here, i.e.
> >>
> >>             ASSERT((intack.vector >> 4) >= (pt_vector >> 4));
> >>
> >> ?
> >>
> >
> > However it still doesn't explain why original ASSERT is triggered.
> > vlapic_find_highest_vector actually finds the highest vector, instead
> > of highest class...
> >
> > static int vlapic_find_highest_vector(const void *bitmap)
> > {
> >     const uint32_t *word = bitmap;
> >     unsigned int word_offset = NR_VECTORS / 32;
> >
> >     /* Work backwards through the bitmap (first 32-bit word in every four). 
> > */
> >     while ( (word_offset != 0) && (word[(--word_offset)*4] == 0) )
> >         continue;
> >
> >     return (fls(word[word_offset*4]) - 1) + (word_offset * 32);
> > }
> 
> Well, perhaps a PIR -> IRR syncing issue then (I in particular note
> the early bailing from vmx_sync_pir_to_irr())? I guess we'd want
> to see the entire IRR array (and perhaps also PI state) if the check
> in the assertion fails.
> 

Yes, I asked Chao to add some debug info in that case. The problem now
is when we will reproduce the bug to see meaningful hint...

Thanks
Kevin

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