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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan

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