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

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

On 08/12/2014 08:45 AM, Jan Beulich wrote:
>>>> On 08.08.14 at 18:55, <boris.ostrovsky@oracle.com> 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.

The return value of 1 of vpmu_do_msr() will now indicate that the 
routine encountered a fault as opposed to indicating whether the MSR 
access was to a VPMU register.

I will make a comment to that effect.

>
>> @@ -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).

Right. Because we don't want to change VPMU state if a fault is 
encountered (which is possible with current code).

Come think of it, this is still not enough --- we may leave registers 
updated when fault happens. I should fix that too.

-boris

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