Gleb Natapov wrote: > 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. OK, so the faults raised by the instruction under the interrupt shadow can still cause troubles. Guess we have to live with it unless we what to trap all exceptions that instructions can raise. Will adjust the comment. > > 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. Does it clear it _for_ the exception handler or also in rflags pushed on the stack? Besides this, proper #DB forwarding to the guest was missing. > >>>> 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. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux -- 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
