[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 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |