Re: [Xen-devel] [PATCH v4 09/17] x86/VPMU: Interface for setting PMU mode and flags

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 01/24/2014 10:10 AM, Jan Beulich wrote:
> > > > 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.
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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel