Re: [PATCH v23 11/15] VPMU/AMD: Check MSR values before writing to hardware

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

On 06/05/2015 12:03 PM, Jan Beulich wrote:
>>>> On 29.05.15 at 20:42, <boris.ostrovsky@oracle.com> wrote:
>> @@ -289,19 +302,24 @@ static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
>>   {
>>       struct vcpu *v = current;
>>       struct vpmu_struct *vpmu = vcpu_vpmu(v);
>> +    unsigned int idx = 0;
>> +    int type = get_pmu_reg_type(msr, &idx);
>>   
>>       ASSERT(!supported);
>>   
>>       /* For all counters, enable guest only mode for HVM guest */
>> -    if ( has_hvm_container_vcpu(v) &&
>> -         (get_pmu_reg_type(msr) == MSR_TYPE_CTRL) &&
>> +    if ( has_hvm_container_vcpu(v) && (type == MSR_TYPE_CTRL) &&
>>            !is_guest_mode(msr_content) )
>>       {
>>           set_guest_mode(msr_content);
>>       }
>>   
>> +    if ( (type == MSR_TYPE_CTRL ) &&
>> +         ((msr_content & CTRL_RSVD_MASK) != ctrl_rsvd[idx]) )
>> +        return 1;
> I think the check should go before the use of the value for anything,
> i.e. including the set_guest_mode() above.
>
> Also I'm pretty sure I asked about the meaning of 1 as a return
> value of a function returning int: If this is a boolean, the function
> should return bool_t (and probably use 1 as success indicator,
> while the case at hand appears to be a failure one). If this is an
> error indicator, -E... values should be used. Of course, if for some
> reason this is meant to represent success, considering the function
> being that way even before your change, I'd not insist on you
> re-working the return value aspects of it.


One means error (which is the reverse of what it is now), this is 
described in patch 8.  We did talk about this a while ago (not during 
latest round) when IIRC you requested me to clarify this in the commit 
message.

I can replace these 1s with -EINVAL (or -EFAULT).

-boris