[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.