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

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

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

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

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

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