On 20/06/16 14:11, Marcelo Tosatti wrote:
On Sat, Jun 18, 2016 at 02:43:51PM +0100, Alan Jenkins wrote:
On 18/06/2016, Marcelo Tosatti <mtosatti@xxxxxxxxxx> wrote:
I'm happy to check my own test if this is considered a solution.
(And apologies for how my own efforts go... flailing around and not
always communicating effectively...)
Your report had many details.
Good, let's hope I will not report too many irrelevant details ("bury
the lede") :).
1) I think the commit message could be improved. Specifically it
says the problem is due to TSC write, then that we need to fix LAPIC
reset. It doesn't say what the connection is between the two.
Right.
(I.e. you would have to read the bug report, & then realize that it
_happens_ to involve a LAPIC state restore as well, because it's
triggered by reloading the entire VM state from a snapshot)
Doing so might make the intention of this commit clearer. I've been
trying to work out a high quality fix, which prevents the lockups
revealed by this case. I think this commit would fix my bug nicely,
but it doesn't help e.g. if the LAPIC restore was omitted from the
sequence.
(Both TSC and the value of TSC deadline are set by MSRs, not by
writing to the LAPIC. So I don't think the LAPIC state restore is
necessary to trigger the issue).
2) If this approach is considered expedient, I have two comments -
i) isn't it better to set lapic_timer.expired_tscdeadline in
lapic.c, i.e. in apic_post_state_restore() ?
ii) then, wouldn't it be more correct to set it between
hrtimer_cancel() and start_apic_timer()? It doesn't sound right to
clear expired_tscdeadline, in case it was set immediately by
start_apic_timer()
Alan
Advancing the timer expiration should only be necessary on
guest initiated writes. Lets improve the commit message.
The timer expiration is still advanced, this patch just avoids the
busywait. I think you mean it's ok if the guest would be interrupted
early after a host-initiated write.
I think it'd be fine in practice; even interrupting before observed TSC
reaches the TSC deadline isn't a problem in itself. When restoring CPU
state, qemu will already have burnt the ~10ns advance in terms of *real*
time. And if there actually were external observers, they would already
be confused by the guest being rewound. I just can't code something
like that without leaving a comment.
Can you confirm the patch fixes your problem?
That did it, yes.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
