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>

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/09/13 17:05, Sapello, Angelo wrote:
> My apologies for the format, git send-email refused to connect to our server 
> so I had to construct the email by hand.

In which case, could you also attach the patch file to the email, so
there is at least one canonical version available.

Although fixing git send-email would be the preferred solution.

~Andrew

>   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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel