>>> On 08.05.15 at 23:06, <boris.ostrov...@oracle.com> wrote: > --- a/xen/arch/x86/hvm/svm/vpmu.c > +++ b/xen/arch/x86/hvm/svm/vpmu.c > @@ -45,6 +45,7 @@ static unsigned int __read_mostly num_counters; > static const u32 __read_mostly *counters; > static const u32 __read_mostly *ctrls; > static bool_t __read_mostly k7_counters_mirrored; > +static unsigned long __read_mostly ctxt_sz;
Can this reasonably require a long instead of just an int? > +static int core2_vpmu_verify(struct vcpu *v) > +{ > + unsigned int i; > + struct vpmu_struct *vpmu = vcpu_vpmu(v); > + struct xen_pmu_intel_ctxt *core2_vpmu_cxt = > + &v->arch.vpmu.xenpmu_data->pmu.c.intel; > + uint64_t *fixed_counters = vpmu_reg_pointer(core2_vpmu_cxt, > fixed_counters); > + struct xen_pmu_cntr_pair *xen_pmu_cntr_pair = > + vpmu_reg_pointer(core2_vpmu_cxt, arch_counters); > + uint64_t fixed_ctrl; > + uint64_t enabled_cntrs = 0; > + > + if ( core2_vpmu_cxt->global_ovf_ctrl & global_ovf_ctrl_mask ) > + return 1; The function returns only 0 and 1, with 1 apparently meaning failure. Either make the function return (negative) error values, or make it return bool_t, flipping the individual return values. Also - indentation. > + > + fixed_ctrl = core2_vpmu_cxt->fixed_ctrl; > + if ( fixed_ctrl & fixed_ctrl_mask ) > + return 1; > + > + for ( i = 0; i < fixed_pmc_cnt; i++ ) > + { > + if ( fixed_counters[i] & fixed_counters_mask ) > + return 1; > + if ( (fixed_ctrl >> (i * FIXED_CTR_CTRL_BITS)) & 3 ) > + enabled_cntrs |= (1ULL << i); > + } > + enabled_cntrs <<= 32; > + > + for ( i = 0; i < arch_pmc_cnt; i++ ) > + { > + uint64_t control = xen_pmu_cntr_pair[i].control; > + > + if ( control & ARCH_CTRL_MASK ) > + return 1; > + if ( control & (1ULL << 22) ) > + enabled_cntrs |= (1ULL << i); > + } > + > + if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) && > + !is_canonical_address(core2_vpmu_cxt->ds_area) ) > + return 1; > + > + if ( (core2_vpmu_cxt->global_ctrl & enabled_cntrs) || > + (core2_vpmu_cxt->ds_area != 0) ) > + vpmu_set(vpmu, VPMU_RUNNING); > + else > + vpmu_reset(vpmu, VPMU_RUNNING); > + > + *(uint64_t *)vpmu->priv_context = enabled_cntrs; Very ugly and non-obvious. Preferably replace the explicit cast (container_of or some such would be fine); if that's impossible, add a brief comment explaining what is going on here. > +static int core2_vpmu_load(struct vcpu *v, bool_t from_guest) > { > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) ) > - return; > + return 0; > + > + if ( from_guest ) > + { > + ASSERT(!is_hvm_vcpu(v)); > + if ( core2_vpmu_verify(v) ) > + return 1; Same issue with the return type/values here. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel