Re: [Xen-devel] [PATCH v9 13/20] x86/VPMU: When handling MSR accesses, leave fault injection to callers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.08.14 at 18:55, <boris.ostrovsky@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> @@ -458,13 +458,13 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, 
> uint64_t msr_content)
>              if ( cpu_has(&current_cpu_data, X86_FEATURE_DSCPL) )
>                  supported |= IA32_DEBUGCTLMSR_BTS_OFF_OS |
>                               IA32_DEBUGCTLMSR_BTS_OFF_USR;
> -            if ( msr_content & supported )
> +
> +            if ( !(msr_content & supported ) ||
> +                 !vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
>              {
> -                if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
> -                    return 1;
> -                gdprintk(XENLOG_WARNING, "Debug Store is not supported on 
> this cpu\n");
> -                hvm_inject_hw_exception(TRAP_gp_fault, 0);
> -                return 0;
> +                gdprintk(XENLOG_WARNING,
> +                         "Debug Store is not supported on this cpu\n");
> +                return 1;

The code here (and in particular the #GP injection) was wrong anyway;
see the small series I sent earlier today. Please base your on top of that.

> @@ -476,18 +476,17 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, 
> uint64_t msr_content)
>      {
>      case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
>          core2_vpmu_cxt->global_status &= ~msr_content;
> -        return 1;
> +        return 0;
>      case MSR_CORE_PERF_GLOBAL_STATUS:
>          gdprintk(XENLOG_INFO, "Can not write readonly MSR: "
>                   "MSR_PERF_GLOBAL_STATUS(0x38E)!\n");
> -        hvm_inject_hw_exception(TRAP_gp_fault, 0);
>          return 1;

Suspicious: Most return values get flipped, but this one (and at least
one more below) doesn't. Any such inconsistencies that are being
corrected (assuming this is intentional) as you go should be spelled
out in the description. And then the question of course is whether
it's really necessary to flip the meaning of this and some similar SVM
function's return values anyway.

> @@ -541,45 +539,43 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, 
> uint64_t msr_content)
>          }
>      }
>  
> -    if ( (global_ctrl & *enabled_cntrs) || (core2_vpmu_cxt->ds_area != 0) )
> -        vpmu_set(vpmu, VPMU_RUNNING);
> -    else
> -        vpmu_reset(vpmu, VPMU_RUNNING);

The moving off this code is - again without at least a brief comment -
also not obviously correct; at least to me it looks like this slightly
alters behavior (perhaps towards the better, but anyway).

> @@ -607,19 +603,14 @@ static int core2_vpmu_do_rdmsr(unsigned int msr, 
> uint64_t *msr_content)
>              rdmsrl(msr, *msr_content);
>          }
>      }
> -    else
> +    else if ( msr == MSR_IA32_MISC_ENABLE )
>      {
>          /* Extension for BTS */
> -        if ( msr == MSR_IA32_MISC_ENABLE )
> -        {
> -            if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
> -                *msr_content &= ~MSR_IA32_MISC_ENABLE_BTS_UNAVAIL;
> -        }
> -        else
> -            return 0;

Again a case where the return value stays the same even if in
general it appears to get flipped in this function.

> +        if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
> +            *msr_content &= ~MSR_IA32_MISC_ENABLE_BTS_UNAVAIL;
>      }
>  
> -    return 1;
> +    return 0;
>  }

Jan


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