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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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

-boris


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