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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.06.15 at 18:32, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 06/05/2015 12:03 PM, Jan Beulich wrote:
>>>>> On 29.05.15 at 20:42, <boris.ostrovsky@xxxxxxxxxx> 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).

Yes please - using 1 to indicate an error is pretty odd looking at
most other hypervisor code.

Jan


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