Re: [PATCH v8 12/19] x86/VPMU: When handling MSR accesses, leave fault injection to callers

["Jan Beulich" <JBeulich@suse.com>]

>>> On 01.07.14 at 16:37, <boris.ostrovsky@oracle.com> wrote:
> @@ -2079,11 +2079,18 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>                         MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL;
>          /* Perhaps vpmu will change some bits. */
>          if ( vpmu_do_rdmsr(msr, msr_content) )
> -            goto done;
> +            goto gp_fault;
>          break;
> -    default:
> +    case MSR_P6_PERFCTR0...MSR_P6_PERFCTR1:
> +    case MSR_P6_EVNTSEL0...MSR_P6_EVNTSEL1:
> +    case MSR_CORE_PERF_FIXED_CTR0...MSR_CORE_PERF_FIXED_CTR2:
> +    case MSR_CORE_PERF_FIXED_CTR_CTRL...MSR_CORE_PERF_GLOBAL_OVF_CTRL:
> +    case MSR_IA32_PEBS_ENABLE:
> +    case MSR_IA32_DS_AREA:
>          if ( vpmu_do_rdmsr(msr, msr_content) )
> -            break;
> +            goto gp_fault;
> +        break;
> +    default:

If you really want to switch to a white listing approach, I think you'd
be better off having the previous case fall through into the PMU ones,
thus reducing code duplication.

> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> @@ -461,13 +461,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_DEBUG,
> +                         "Debug Store is not supported on this cpu\n");
> +                return 1;

I think these warning level adjustments should be in their own patch,
with (unless there is proof of the contrary) the wording in the
commit message adjusted so that one doesn't get the false impression
of you wanting to slip in a security fix this way: Messages at guest
level are (by default) intentionally always rate limited.

Jan