[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 09/17] x86/VPMU: Interface for setting PMU mode and flags
>>> On 21.01.14 at 20:08, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote: > +long do_xenpmu_op(int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg) > +{ > + int ret = -EINVAL; > + xen_pmu_params_t pmu_params; > + uint32_t mode; > + > + switch ( op ) > + { > + case XENPMU_mode_set: > + if ( !is_control_domain(current->domain) ) > + return -EPERM; > + > + if ( copy_from_guest(&pmu_params, arg, 1) ) > + return -EFAULT; > + > + mode = (uint32_t)pmu_params.d.val & XENPMU_MODE_MASK; > + if ( mode & ~XENPMU_MODE_ON ) > + return -EINVAL; Please, if you add a new interface, think carefully about future extension room: Here you ignore the upper 32 bits of .val instead of making sure they're zero, thus making it impossible to assign them some meaning later on. > + > + vpmu_mode &= ~XENPMU_MODE_MASK; > + vpmu_mode |= mode; > + > + ret = 0; > + break; > + > + case XENPMU_mode_get: > + pmu_params.d.val = vpmu_mode & XENPMU_MODE_MASK; > + pmu_params.v.version.maj = XENPMU_VER_MAJ; > + pmu_params.v.version.min = XENPMU_VER_MIN; > + if ( copy_to_guest(arg, &pmu_params, 1) ) __copy_to_guest(). > + return -EFAULT; > + ret = 0; > + break; > + > + case XENPMU_feature_set: > + if ( !is_control_domain(current->domain) ) > + return -EPERM; > + > + if ( copy_from_guest(&pmu_params, arg, 1) ) > + return -EFAULT; > + > + if ( (uint32_t)pmu_params.d.val & ~XENPMU_FEATURE_INTEL_BTS ) > + return -EINVAL; See above. > + > + vpmu_mode &= ~XENPMU_FEATURE_MASK; > + vpmu_mode |= (uint32_t)pmu_params.d.val << XENPMU_FEATURE_SHIFT; > + > + ret = 0; > + break; > + > + case XENPMU_feature_get: > + pmu_params.d.val = vpmu_mode & XENPMU_FEATURE_MASK; > + if ( copy_to_guest(arg, &pmu_params, 1) ) See above. > --- a/xen/include/public/xen.h > +++ b/xen/include/public/xen.h > @@ -101,6 +101,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t); > #define __HYPERVISOR_kexec_op 37 > #define __HYPERVISOR_tmem_op 38 > #define __HYPERVISOR_xc_reserved_op 39 /* reserved for XenClient */ > +#define __HYPERVISOR_xenpmu_op 40 > > /* Architecture-specific hypercall definitions. */ > #define __HYPERVISOR_arch_0 48 Are you certain this wouldn't better be an architecture-specific hypercall? Just like with Machine Check, I don't think all architectures are guaranteed to have (or ever get) performance monitoring capabilities. > +/* Parameters structure for HYPERVISOR_xenpmu_op call */ > +struct xen_pmu_params { > + /* IN/OUT parameters */ > + union { > + struct version { > + uint8_t maj; > + uint8_t min; > + } version; > + uint64_t pad; > + } v; Looking at the implementation above I don't see this ever being an IN parameter. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |