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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 05/18/2015 11:15 AM, Jan Beulich wrote:
>>>> On 08.05.15 at 23:06, <boris.ostrovsky@oracle.com> 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).