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>

["Sapello, Angelo" <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