[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] Add support for disabling LBR recording after it has been enabled in HVMs using VMX. Signed-off-by: Angelo Sapello <asapello@xxxxxxxxxxxxx>
My apologies for the format, git send-email refused to connect to our server so I had to construct the email by hand. Also, sorry about the coding style. Okay, as far as actual content: 1) The goal here is to allow an HVM using VMX to first enable last branch recording, then suspend last branch recording, then read the frozen LBR stack. Consider if you want to print a back trace of your code using the LBRs, you certainly don't want to continue recording the jumps into the debug printing code. 2) The changes here, do have an effect. (I've tested it, and it works.) The issue with the origin code was that after enable LBRs, the DEBUGCTL msr is 1. To disable LBRs you have to set it back to 0. However, the first check is whether or not the the requested value is zero, in which case it aborts. My revision checks to see if the set of changes (the current value in the MSR xored against the requested new value) is empty, in which case the request can be ignored. 3) The second "if" statement is more about consistency, but didn't really need to be changed. If more functionality was added when enabling LBRs, it would be good to skip this if LBRs were enabled previously. 4) The final comment is pointing out the issue in 2) above. Namely, in the origin code, you couldn't reach that line with a msr_content value of 0 (turn off all debug features). In addition, someone might be tempted to remove access to the LBR stack when LBRs are disable, but this would break the use case I stated in 1). Thanks, Angelo Sapello ________________________________________ From: Jan Beulich [JBeulich@xxxxxxxx] Sent: Friday, September 06, 2013 11:15 AM To: Sapello, Angelo Cc: xen-devel Subject: Re: [Xen-devel] [PATCH] Add support for disabling LBR recording after it has been enabled in HVMs using VMX. Signed-off-by: Angelo Sapello <asapello@xxxxxxxxxxxxx> >>> On 06.09.13 at 16:28, "Sapello, Angelo" <asapello@xxxxxxxxxxxxx> wrote: First and foremost: Please send patches in the form matching general expectations. E.g. only the title belongs in the subject line, description and tags go in the body, preceding the actual patch. > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -2054,16 +2054,25 @@ static int vmx_msr_write_intercept(unsigned int msr, > ui$ > case MSR_IA32_DEBUGCTLMSR: { > int i, rc = 0; > uint64_t supported = IA32_DEBUGCTLMSR_LBR | IA32_DEBUGCTLMSR_BTF; > + uint64_t old_msr_content, change_set; > > - if ( !msr_content ) > +// Don't change everything, but just consider what features are being changed > +// May be a little slow with the extra read, but changes to DEBUGCTLMSR > should not be frequent > +// ~ Angelo Sapello And then you should read ./CODING_STYLE. Comments like this are a no-go. We also don't add name tags to comments - who added a comment is visible from the commit metadata. > + old_msr_content = __vmread(GUEST_IA32_DEBUGCTL); > + change_set = (old_msr_content ^ msr_content); > + > +// Setting DEBUGCTLMSR to zero is valid when disabling debug features > +// only consider changes ~ AS > + if ( !change_set ) > break; > - if ( msr_content & ~supported ) > + if ( change_set & ~supported ) // Only consider bits that changed ~ > AS I don't think this change has any actual effect. > { > /* Perhaps some other bits are supported in vpmu. */ > if ( !vpmu_do_wrmsr(msr, msr_content) ) > break; > } > - if ( msr_content & IA32_DEBUGCTLMSR_LBR ) > + if ( change_set & msr_content & IA32_DEBUGCTLMSR_LBR ) What's the goal here? Performance can't be it, according to your comment above. > { > const struct lbr_info *lbr = last_branch_msr_get(); > if ( lbr == NULL ) > @@ -2074,6 +2083,10 @@ static int vmx_msr_write_intercept(unsigned int msr, > uint64_t msr_content) > if ( (rc = vmx_add_guest_msr(lbr->base + i)) == 0 ) > vmx_disable_intercept_for_msr(v, lbr->base + i, > MSR_TYPE_R | MSR_TYPE_W); > } > +// NB that we can now reach here to turn off LBR recording > +// Also, never turn actual LBRs (from IPs, to IPs) back off, since > +// HVM may wish to read them in their frozen state. > +// ~AS This comment, at least to me, is confusing rather than clarifying. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |