On Tue, Feb 16, 2010 at 10:14:56AM +0100, Jan Kiszka wrote: > Gleb Natapov wrote: > > On Mon, Feb 15, 2010 at 07:17:18PM +0100, Jan Kiszka wrote: > >> As there is no interception on AMD on the end of an NMI handler but only > >> on its iret, we are forced to step out by setting TF in rflags. This can > >> collide with host or guest using single-step mode, and it may leak the > >> flags onto the guest stack if IRET triggers some exception. > > The code is trying to handle the case where debugger used TF flags and we > > want to single step from NMI handler simultaneously. Do you see problem with > > that code? Uf yes may be it sill be much simpler to fix it? TF leakage is real, > > but what problem it may cause? Note that your patch does not solve this problem > > too. See the comment that you've deleted: > > /* Something prevents NMI from been injected. Single step over > > possible problem (IRET or exception injection or interrupt > > shadow) */ > > So the reason for single step is not necessary IRET, _any_ exception > > is possible at this point. > > That is exactly what my code tries to avoid: Exceptions are all (famous > last word) caught, and single-stepping is disabled until that is > resolved. So no more leakage, and only IRET remains as reason here (thus > my deletion). > I don't understand why only IRET remains as a reason here? Code will get there if interrupt shadow is in effect too and then next instruction may generate any exception not only those that IRET generates. Also you haven't answered what is the problem with current code (except TF leakage) and why TF leakage is so important. BTW are you sure that TF leakage actually happens? I see in Intel SDM: The processor clears the TF flag before calling the exception handler. > > > >> This patch addresses the TF leakage by trapping all exceptions that may > >> show up on IRET execution and cleaning up the state again before > >> reinjecting the exception. Collisions with concurrent users are avoided > >> by carefully checking for their presence and forwarding #DB exceptions > >> whenever required. > > This patch is scary to apply without test cases. > > Do you have one for the exception case? The others should have been I have one that check exception during nmi delivery and one that check that nested nmi is delivered after iret. I don't have one that fault during iret. > covered manually here, but I agree we should write a unit test, also for > the sake of VMX. > -- Gleb. -- 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
