Re: [Xen-devel] [Patch] x86/HVM: Fix RTC interrupt modelling

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.02.14 at 13:11, Tim Deegan <tim@xxxxxxx> wrote:
> At 09:15 +0000 on 11 Feb (1392106520), Jan Beulich wrote:
>> >>> On 10.02.14 at 18:21, Tim Deegan <tim@xxxxxxx> wrote:
>> > My understanding was that the problem is explicitly in the change to
>> > how RTC code is called from vpt code.
>> > 
>> > Originally, the RTC callback was called from pt_intr_post, like other
>> > vpt sources.  Your rework changed it to be called much earlier, when
>> > the vpt was considering which time source to choose.  AIUI that was to
>> > let the RTC code tell the VPT not to inject, if the guest hasn't acked
>> > the last interrupt, right?
>> 
>> Not only, it was also a necessary pre-adjustment for the !PIE case
>> to work (see below).
>> 
>> > Since that was changed later to allow a certain number of dead ticks
>> > before deciding to stop the timer chain, the decision no longer has to
>> > be made so early -- we can allow one more IRQ to go in and then
>> > disable it. 
>> > 
>> > That is the main change of this cset:  we go back to driving
>> > the interrupt from the vpt code and fixing up the RTC state after vpt
>> > tells us it's injected an interrupt.
>> 
>> And that's what is wrong imo, as it doesn't allow driving PF correctly
>> when !PIE.
> 
> Oh, I see -- the current code doesn't turn the vpt off when !PIE.  Can
> you remember why not?  Have I forgotten some wrinkle or race here?

Because an OS could inspect PF without setting PIE.

>> > Yeah, this has nothing to do with the bug being fixed here.  The old
>> > REG_C read was operating correctly, but on the return-to-guest path:
>> >  - vpt sees another RTC interrupt is due and calls RTC code
>> >  - RTC code sees REG_C clear, sets PF|IRQF and asserts the line
>> >  - vlapic code sees the last interrupt is still in the ISR and does
>> >    nothing;
>> >  - we return to the guest having set IRQF but not consumed a timer
>> >    event, so vpt stste is the same
>> >  - the guest sees the old REG_C, with PF|IRQF set, and re-reads, 
>> >    waiting for a read of 0.
>> >  - repeat forever.
>> 
>> Which would call for a flag suppressing the setting of PF|IRQF
>> until the timer event got consumed. Possibly with some safety
>> belt for this to not get deferred indefinitely (albeit if the interrupt
>> doesn't get injected for extended periods of time, the guest
>> would presumably have more severe problems than these flags
>> not getting updated as expected).
> 
> That's pretty much what we're doing here -- the pt_intr_post callback
> sets PF|IRQF when the interrupt is injected.

Right, except you do this be reverting other stuff rather than
adding the missing functionality on top.

Jan


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