From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v21 02/14] x86/VPMU: Add public xenpmu.h Date: Mon, 18 May 2015 12:12:56 -0400 Message-ID: <555A0F88.3010303@oracle.com> References: <1431119174-1947-1-git-send-email-boris.ostrovsky@oracle.com> <1431119174-1947-3-git-send-email-boris.ostrovsky@oracle.com> <555A1E14020000780007B425@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <555A1E14020000780007B425@mail.emea.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: kevin.tian@intel.com, suravee.suthikulpanit@amd.com, andrew.cooper3@citrix.com, tim@xen.org, dietmar.hahn@ts.fujitsu.com, xen-devel@lists.xen.org, Aravind.Gopalakrishnan@amd.com, jun.nakajima@intel.com, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org On 05/18/2015 11:15 AM, Jan Beulich wrote: >>>> On 08.05.15 at 23:06, 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).