Re: [PATCH v24 08/15] x86/VPMU: When handling MSR accesses, leave fault injection to callers

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 06/15/2015 11:06 AM, Jan Beulich wrote:
>>>> On 10.06.15 at 17:04, <boris.ostrovsky@oracle.com> wrote:
>> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
>> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
>> @@ -454,36 +454,41 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
>>                                IA32_DEBUGCTLMSR_BTS_OFF_USR;
>>               if ( !(msr_content & ~supported) &&
>>                    vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
>> -                return 1;
>> +                return 0;
>>               if ( (msr_content & supported) &&
>>                    !vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
>>                   printk(XENLOG_G_WARNING
>>                          "%pv: Debug Store unsupported on this CPU\n",
>>                          current);
>>           }
>> -        return 0;
>> +        return -EINVAL;
>>       }
>>   
>>       ASSERT(!supported);
>>   
>> +    if ( type == MSR_TYPE_COUNTER &&
>> +         (msr_content &
>> +          ~((1ull << core2_get_bitwidth_fix_count()) - 1)) )
>> +        /* Writing unsupported bits to a fixed counter */
>> +        return -EINVAL;
>> +
>>       core2_vpmu_cxt = vpmu->context;
>>       enabled_cntrs = vpmu->priv_context;
>>       switch ( msr )
>>       {
>>       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;
>> +        return -EINVAL;
> Is it intentional that you convert a success to a failure here? If so,
> this should be mentioned (with reason) in the commit message. If
> not, this should be adjusted to there's no behavioral change here.

Yes, this is intentional. Until now return value indicated whether 
access was to a PMU register. This worked for HVM guests since they can 
do hvm_inject_trap() at any time. For PV guests we are called from 
emulate_privileged_op() and we need to know whether access was 
successful or not. This way emulate_privileged_op() will take care of 
fault injection by returning 0.

-boris