On Fri, 2021-01-15 at 14:48 +0100, Paolo Bonzini wrote: > On 15/01/21 01:14, Sean Christopherson wrote: > > > + trace_kvm_nested_vmlaunch_resume(kvm_rip_read(vcpu), > > Hmm, won't this RIP be wrong for the migration case? I.e. it'll be L2, not L1 > > as is the case for the "true" nested VM-Enter path. Actually in this case, the initial RIP of 0x000000000000fff0 will be printed which isn't that bad. A tracepoint in nested state load function would be very nice to add to mark this explicitly. I'll do this later. > > It will be the previous RIP---might as well be 0xfffffff0 depending on > what userspace does. I don't think you can do much better than that, > using vmcs12->host_rip would be confusing in the SMM case. > > > > + vmx->nested.current_vmptr, > > > + vmcs12->guest_rip, > > > + vmcs12->vm_entry_intr_info_field); > > The placement is a bit funky. I assume you put it here so that calls from > > vmx_set_nested_state() also get traced. But, that also means > > vmx_pre_leave_smm() will get traced, and it also creates some weirdness where > > some nested VM-Enters that VM-Fail will get traced, but others will not. > > > > Tracing vmx_pre_leave_smm() isn't necessarily bad, but it could be confusing, > > especially if the debugger looks up the RIP and sees RSM. Ditto for the > > migration case. > > Actually tracing vmx_pre_leave_smm() is good, and pointing to RSM makes > sense so I'm not worried about that. > > Paolo > I agree with that and indeed this was my intention. In fact I will change the svm's tracepoint to behave the same way in the next patch series (I'll move it to enter_svm_guest_mode). (When I wrote this patch I somehow thought that this is what SVM already does). Best regards, Maxim Levitsky
