[PATCH] Add support for disabling LBR recording after it has been enabled in HVMs using VMX. Signed-off-by: Angelo Sapello <asapello@appcomsci.com>

[PATCH] Add support for disabling LBR recording after it has been enabled in HVMs using VMX. Signed-off-by: Angelo Sapello <asapello@appcomsci.com>

[Sapello, Angelo]

---
 xen/arch/x86/hvm/vmx/vmx.c |   19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 011a817..149f28e 100644
--- 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
+        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
         {
             /* 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 )
         {
             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
 
         if ( (rc < 0) ||
              (vmx_add_host_load_msr(msr) < 0) )
-- 
1.7.10.4

Re: [PATCH] Add support for disabling LBR recording after it has been enabled in HVMs using VMX. Signed-off-by: Angelo Sapello <asapello@appcomsci.com>

[Jan Beulich]

>>> On 06.09.13 at 16:28, "Sapello, Angelo" <asapello@appcomsci.com> 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

Re: [PATCH] Add support for disabling LBR recording after it has been enabled in HVMs using VMX. Signed-off-by: Angelo Sapello <asapello@appcomsci.com>

[Sapello, Angelo]

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@suse.com]
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@appcomsci.com>

>>> On 06.09.13 at 16:28, "Sapello, Angelo" <asapello@appcomsci.com> 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

Re: [PATCH] Add support for disabling LBR recording after it has been enabled in HVMs using VMX. Signed-off-by: Angelo Sapello <asapello@appcomsci.com>

[Andrew Cooper]

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@suse.com]
> 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@appcomsci.com>
>
>>>> On 06.09.13 at 16:28, "Sapello, Angelo" <asapello@appcomsci.com> 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@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] Add support for disabling LBR recording after it has been enabled in HVMs using VMX. Signed-off-by: Angelo Sapello <asapello@appcomsci.com>

[Jan Beulich]

>>> On 06.09.13 at 18:05, "Sapello, Angelo" <asapello@appcomsci.com> wrote:
> 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.

And one more formal thing: Please don't top-post.

> 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.

Yes, this is all fine and understood. We just need a well formed patch.

> 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).

And this doesn't need a code comment at all; it just needs to be clear
from the patch description.

Jan