Re: [Xen-devel] [PATCH v21 02/14] x86/VPMU: Add public xenpmu.h

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 05/18/2015 11:15 AM, Jan Beulich wrote:
> > > > On 08.05.15 at 23:06, <boris.ostrovsky@xxxxxxxxxx> wrote:
> > --- /dev/null
> > +++ b/xen/include/public/arch-x86/pmu.h
> > @@ -0,0 +1,122 @@
> > +#ifndef __XEN_PUBLIC_ARCH_X86_PMU_H__
> > +#define __XEN_PUBLIC_ARCH_X86_PMU_H__
> > +
> > +/* x86-specific PMU definitions */
> > +
> > +/* AMD PMU registers and structures */
> > +struct xen_pmu_amd_ctxt {
> > +    /* Offsets to counter and control MSRs (relative to xen_pmu_arch.c.amd)
> > */
> > +    uint32_t counters;
> > +    uint32_t ctrls;
> > +};
> > +typedef struct xen_pmu_amd_ctxt xen_pmu_amd_ctxt_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_pmu_amd_ctxt_t);
> > +
> > +/* Intel PMU registers and structures */
> > +struct xen_pmu_cntr_pair {
> > +    uint64_t counter;
> > +    uint64_t control;
> > +};
> > +typedef struct xen_pmu_cntr_pair xen_pmu_cntr_pair_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_pmu_cntr_pair_t);
> > +
> > +struct xen_pmu_intel_ctxt {
> > +    uint64_t global_ctrl;
> > +    uint64_t global_ovf_ctrl;
> > +    uint64_t global_status;
> > +    uint64_t fixed_ctrl;
> > +    uint64_t ds_area;
> > +    uint64_t pebs_enable;
> > +    uint64_t debugctl;
> > +    /*
> > +     * Offsets to fixed and architectural counter MSRs (relative to
> > +     * xen_pmu_arch.c.intel)
> > +     */
> > +    uint32_t fixed_counters;
> > +    uint32_t arch_counters;
> > +};
> > +typedef struct xen_pmu_intel_ctxt xen_pmu_intel_ctxt_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_pmu_intel_ctxt_t);
> > +
> > +/* Sampled domain's registers */
> > +struct xen_pmu_regs {
> > +    uint64_t ip;
> > +    uint64_t sp;
> > +    uint64_t flags;
> > +    uint16_t cs;
> > +    uint16_t ss;
> > +    uint8_t cpl;
> > +    uint8_t pad[3];
> > +};
> > +typedef struct xen_pmu_regs xen_pmu_regs_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_pmu_regs_t);
> > +
> > +/* PMU flags */
> > +#define PMU_CACHED         (1<<0) /* PMU MSRs are cached in the context */
> > +
> > +/*
> > + * Architecture-specific information describing state of the processor at
> > + * the time of PMU interrupt.
> > + * Fields of this structure marked as RW for guest can only be written by the
> > + * guest when PMU_CACHED bit in pmu_flags is set (which is done by the
> > + * hypervisor during PMU interrupt). Hypervisor will read updated data in
> > + * XENPMU_flush hypercall and clear PMU_CACHED bit.
> > + */
> > +struct xen_pmu_arch {
> > +    union {
> > +        /*
> > +         * Processor's registers at the time of interrupt.
> > +         * RW for hypervisor, RO for guests.
> > +         */
> > +        struct xen_pmu_regs regs;
> > +        /* Padding for adding new registers to xen_pmu_regs in the future */
> > +#define XENPMU_REGS_PAD_SZ  64
> > +        uint8_t pad[XENPMU_REGS_PAD_SZ];
> > +    } r;
> > +
> > +    /* RW for hypervisor, RO for guest */
> > +    uint64_t pmu_flags;
> > +
> > +    /*
> > +     * APIC LVTPC register.
> > +     * RW for both hypervisor and guest.
> > +     * Only APIC_LVT_MASKED bit is loaded by the hypervisor into hardware
> > +     * during XENPMU_flush.
> > +     */
> > +    union {
> > +        uint32_t lapic_lvtpc;
> > +        uint64_t pad;
> > +    } l;
> > +
> > +    /*
> > +     * Vendor-specific PMU registers.
> > +     * RW for both hypervisor and guest.
> > +     * Guest's updates to this field are verified and then loaded by the
> > +     * hypervisor into hardware during XENPMU_flush
> > +     */
> > +    union {
> > +        struct xen_pmu_amd_ctxt amd;
> > +        struct xen_pmu_intel_ctxt intel;
> > +
> > +        /*
> > +         * Padding for contexts (fixed parts only, does not include MSR banks
> > +         * that are specified by offsets)
> > +         */
> > +#define XENPMU_CTXT_PAD_SZ  128
> > +        uint8_t pad[XENPMU_CTXT_PAD_SZ];
> > +    } c;
> > +};
> Marking all the fields RW for the hypervisor is certainly correct from
> a permissions pov, but requires close auditing that the hypervisor
> doesn't ever read a field twice, potentially getting different results
> and hence inconsistent internal state. Therefore - do all of the fields
> _need_ to be RW for the hypervisor? If not, marking the ones
> where this isn't needed as WO would be much preferred, to limit
> the scope of whats needs to be audited.
Right, all arch-independent bits are WO for hypervisor as are 
xen_pmu_regs above. I in fact meant to label them as such but for 
reasons that I can't remember now decided to mark them as RW.
-boris

> Same of course applies to
> all the arch-independent bits further down (which didn't get
> annotated so far).


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