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>

["Jan Beulich" <JBeulich@xxxxxxxx>]

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