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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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