From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v4 09/17] x86/VPMU: Interface for setting PMU mode and flags Date: Fri, 24 Jan 2014 12:13:11 -0500 Message-ID: <52E29F27.50403@oracle.com> References: <1390331342-3967-1-git-send-email-boris.ostrovsky@oracle.com> <1390331342-3967-10-git-send-email-boris.ostrovsky@oracle.com> <52E2905D0200007800116BD2@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <52E2905D0200007800116BD2@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: keir@xen.org, suravee.suthikulpanit@amd.com, andrew.cooper3@citrix.com, eddie.dong@intel.com, dietmar.hahn@ts.fujitsu.com, xen-devel@lists.xen.org, jun.nakajima@intel.com List-Id: xen-devel@lists.xenproject.org On 01/24/2014 10:10 AM, Jan Beulich wrote: >>>> On 21.01.14 at 20:08, Boris Ostrovsky 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. I think I can leave this as is for now --- I am storing VPMU mode and VPMU features in the Xen-private vpmu_mode, which is a 64-bit value. What I probably should do is remove XENPMU_MODE_MASK (and XENPMU_FEATURE_SHIFT and XENPMU_FEATURE_MASK) from the public header since Linux passes down 64-bit pmu_params.d.val without any format assumptions anyway. > >> --- 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. An architecture doesn't necessarily need to have HW performance monitoring support. In principle this interface can be used for passing any performance-related data (e.g. collected by the hypervisor) to the guest. >> +/* 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. Currently Xen doesn't care about version but in the future a guest may specify what version of PMU it wants to use (I hope this day will never come though...) -boris