From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aravind Gopalakrishnan Subject: Re: [PATCH v25 11/15] VPMU/AMD: Check MSR values before writing to hardware Date: Wed, 8 Jul 2015 10:35:59 -0500 Message-ID: <559D435F.1070708@amd.com> References: <1434739486-1611-1-git-send-email-boris.ostrovsky@oracle.com> <1434739486-1611-12-git-send-email-boris.ostrovsky@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1434739486-1611-12-git-send-email-boris.ostrovsky@oracle.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: Boris Ostrovsky , JBeulich@suse.com, kevin.tian@intel.com, suravee.suthikulpanit@amd.com, dietmar.hahn@ts.fujitsu.com, dgdegra@tycho.nsa.gov, andrew.cooper3@citrix.com Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 6/19/2015 1:44 PM, Boris Ostrovsky wrote: > A number of fields of PMU control MSRs are defined as Reserved. AMD > documentation requires that such fields are preserved when the register > is written by software. > > Add checks to amd_vpmu_do_wrmsr() to make sure that guests don't attempt > to modify those bits. > > Signed-off-by: Boris Ostrovsky Reviewed-by: Aravind Gopalakrishnan > --- > xen/arch/x86/hvm/svm/vpmu.c | 49 +++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 43 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/x86/hvm/svm/vpmu.c b/xen/arch/x86/hvm/svm/vpmu.c > index 74d03a5..934f1b7 100644 > --- a/xen/arch/x86/hvm/svm/vpmu.c > +++ b/xen/arch/x86/hvm/svm/vpmu.c > @@ -48,6 +48,7 @@ static bool_t __read_mostly k7_counters_mirrored; > > #define F10H_NUM_COUNTERS 4 > #define F15H_NUM_COUNTERS 6 > +#define MAX_NUM_COUNTERS F15H_NUM_COUNTERS > > /* PMU Counter MSRs. */ > static const u32 AMD_F10H_COUNTERS[] = { > @@ -83,6 +84,11 @@ static const u32 AMD_F15H_CTRLS[] = { > MSR_AMD_FAM15H_EVNTSEL5 > }; > > +/* Bits [63:42], [39:36], 21 and 19 are reserved */ > +#define CTRL_RSVD_MASK ((-1ULL & (~((1ULL << 42) - 1))) | \ > + (0xfULL << 36) | (1ULL << 21) | (1ULL << 19)) > +static uint64_t __read_mostly ctrl_rsvd[MAX_NUM_COUNTERS]; > + > /* Use private context as a flag for MSR bitmap */ > #define msr_bitmap_on(vpmu) do { \ > (vpmu)->priv_context = (void *)-1L; \ > @@ -92,17 +98,24 @@ static const u32 AMD_F15H_CTRLS[] = { > } while (0) > #define is_msr_bitmap_on(vpmu) ((vpmu)->priv_context != NULL) > > -static inline int get_pmu_reg_type(u32 addr) > +static inline int get_pmu_reg_type(u32 addr, unsigned int *idx) > { > if ( (addr >= MSR_K7_EVNTSEL0) && (addr <= MSR_K7_EVNTSEL3) ) > + { > + *idx = addr - MSR_K7_EVNTSEL0; > return MSR_TYPE_CTRL; > + } > > if ( (addr >= MSR_K7_PERFCTR0) && (addr <= MSR_K7_PERFCTR3) ) > + { > + *idx = addr - MSR_K7_PERFCTR0; > return MSR_TYPE_COUNTER; > + } > > if ( (addr >= MSR_AMD_FAM15H_EVNTSEL0) && > (addr <= MSR_AMD_FAM15H_PERFCTR5 ) ) > { > + *idx = (addr - MSR_AMD_FAM15H_EVNTSEL0) >> 1; > if (addr & 1) > return MSR_TYPE_COUNTER; > else > @@ -140,6 +153,16 @@ static inline u32 get_fam15h_addr(u32 addr) > return addr; > } > > +static void amd_vpmu_init_regs(struct xen_pmu_amd_ctxt *ctxt) > +{ > + unsigned i; > + uint64_t *ctrl_regs = vpmu_reg_pointer(ctxt, ctrls); > + > + memset(&ctxt->regs[0], 0, 2 * sizeof(uint64_t) * num_counters); > + for ( i = 0; i < num_counters; i++ ) > + ctrl_regs[i] = ctrl_rsvd[i]; > +} > + > static void amd_vpmu_set_msr_bitmap(struct vcpu *v) > { > unsigned int i; > @@ -289,19 +312,24 @@ static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, > { > struct vcpu *v = current; > struct vpmu_struct *vpmu = vcpu_vpmu(v); > + unsigned int idx = 0; > + int type = get_pmu_reg_type(msr, &idx); > > ASSERT(!supported); > > + if ( (type == MSR_TYPE_CTRL ) && > + ((msr_content & CTRL_RSVD_MASK) != ctrl_rsvd[idx]) ) > + return -EINVAL; > + > /* For all counters, enable guest only mode for HVM guest */ > - if ( has_hvm_container_vcpu(v) && > - (get_pmu_reg_type(msr) == MSR_TYPE_CTRL) && > + if ( has_hvm_container_vcpu(v) && (type == MSR_TYPE_CTRL) && > !is_guest_mode(msr_content) ) > { > set_guest_mode(msr_content); > } > > /* check if the first counter is enabled */ > - if ( (get_pmu_reg_type(msr) == MSR_TYPE_CTRL) && > + if ( (type == MSR_TYPE_CTRL) && > is_pmu_enabled(msr_content) && !vpmu_is_set(vpmu, VPMU_RUNNING) ) > { > if ( !acquire_pmu_ownership(PMU_OWNER_HVM) ) > @@ -313,7 +341,7 @@ static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, > } > > /* stop saving & restore if guest stops first counter */ > - if ( (get_pmu_reg_type(msr) == MSR_TYPE_CTRL) && > + if ( (type == MSR_TYPE_CTRL) && > (is_pmu_enabled(msr_content) == 0) && vpmu_is_set(vpmu, VPMU_RUNNING) ) > { > vpmu_reset(vpmu, VPMU_RUNNING); > @@ -433,7 +461,7 @@ int svm_vpmu_initialise(struct vcpu *v) > if ( !counters ) > return -EINVAL; > > - ctxt = xzalloc_bytes(sizeof(*ctxt) + > + ctxt = xmalloc_bytes(sizeof(*ctxt) + > 2 * sizeof(uint64_t) * num_counters); > if ( !ctxt ) > { > @@ -445,6 +473,7 @@ int svm_vpmu_initialise(struct vcpu *v) > > ctxt->counters = sizeof(*ctxt); > ctxt->ctrls = ctxt->counters + sizeof(uint64_t) * num_counters; > + amd_vpmu_init_regs(ctxt); > > vpmu->context = ctxt; > vpmu->priv_context = NULL; > @@ -457,6 +486,8 @@ int svm_vpmu_initialise(struct vcpu *v) > > int __init amd_vpmu_init(void) > { > + unsigned int i; > + > switch ( current_cpu_data.x86 ) > { > case 0x15: > @@ -490,6 +521,12 @@ int __init amd_vpmu_init(void) > return -ENOSPC; > } > > + for ( i = 0; i < num_counters; i++ ) > + { > + rdmsrl(ctrls[i], ctrl_rsvd[i]); > + ctrl_rsvd[i] &= CTRL_RSVD_MASK; > + } > + > return 0; > } >