[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 1/3] x86/vpmu: Add get/put_vpmu() and VPMU_AVAILABLE
> From: Boris Ostrovsky [mailto:boris.ostrovsky@xxxxxxxxxx] > Sent: Tuesday, February 21, 2017 10:10 PM > > On 02/21/2017 03:09 AM, Tian, Kevin wrote: > >> From: Boris Ostrovsky [mailto:boris.ostrovsky@xxxxxxxxxx] > >> Sent: Saturday, February 18, 2017 1:40 AM > >> > >> vpmu_enabled() (used by hvm/pv_cpuid() to properly report 0xa leaf > >> for Intel processors) is based on the value of VPMU_CONTEXT_ALLOCATED > >> bit. This is problematic: > >> * For HVM guests VPMU context is allocated lazily, during the first > >> access to VPMU MSRs. Since the leaf is typically queried before guest > >> attempts to read or write the MSRs it is likely that CPUID will report > >> no PMU support > >> * For PV guests the context is allocated eagerly but only in responce to > >> guest's XENPMU_init hypercall. There is a chance that the guest will > >> try to read CPUID before making this hypercall. > >> > >> This patch introduces VPMU_AVAILABLE flag which is set (subject to > >> vpmu_mode > >> constraints) during VCPU initialization for both PV and HVM guests. Since > >> this flag is expected to be managed together with vpmu_count, > >> get/put_vpmu() > >> are added to simplify code. > >> > >> vpmu_enabled() (renamed to vpmu_available()) can now use this new flag. > >> > >> (As a side affect this patch also fixes a race in pvpmu_init() where we > >> increment vcpu_count in vpmu_initialise() after checking vpmu_mode) > >> > >> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> > >> --- > >> v2: > >> * Rename VPMU_ENABLED to VPMU_AVAILABLE (and vpmu_enabled() to > >> vpmu_available() > >> * Check VPMU_AVAILABLE flag in put_vpmu() under lock > >> > >> xen/arch/x86/cpu/vpmu.c | 111 > >> ++++++++++++++++++++++++++++++--------------- > >> xen/arch/x86/cpuid.c | 8 ++-- > >> xen/arch/x86/domain.c | 5 ++ > >> xen/arch/x86/hvm/svm/svm.c | 5 -- > >> xen/arch/x86/hvm/vmx/vmx.c | 5 -- > >> xen/include/asm-x86/vpmu.h | 6 ++- > >> 6 files changed, 88 insertions(+), 52 deletions(-) > >> > >> diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c > >> index 35a9403..b30769d 100644 > >> --- a/xen/arch/x86/cpu/vpmu.c > >> +++ b/xen/arch/x86/cpu/vpmu.c > >> @@ -456,32 +456,21 @@ int vpmu_load(struct vcpu *v, bool_t from_guest) > >> return 0; > >> } > >> > >> -void vpmu_initialise(struct vcpu *v) > >> +static int vpmu_arch_initialise(struct vcpu *v) > >> { > >> struct vpmu_struct *vpmu = vcpu_vpmu(v); > >> uint8_t vendor = current_cpu_data.x86_vendor; > >> int ret; > >> - bool_t is_priv_vpmu = is_hardware_domain(v->domain); > >> > >> BUILD_BUG_ON(sizeof(struct xen_pmu_intel_ctxt) > XENPMU_CTXT_PAD_SZ); > >> BUILD_BUG_ON(sizeof(struct xen_pmu_amd_ctxt) > XENPMU_CTXT_PAD_SZ); > >> BUILD_BUG_ON(sizeof(struct xen_pmu_regs) > XENPMU_REGS_PAD_SZ); > >> BUILD_BUG_ON(sizeof(struct compat_pmu_regs) > XENPMU_REGS_PAD_SZ); > >> > >> - ASSERT(!vpmu->flags && !vpmu->context); > >> + ASSERT(!(vpmu->flags & ~VPMU_AVAILABLE) && !vpmu->context); > >> > >> - if ( !is_priv_vpmu ) > >> - { > >> - /* > >> - * Count active VPMUs so that we won't try to change vpmu_mode > >> while > >> - * they are in use. > >> - * vpmu_mode can be safely updated while dom0's VPMUs are active > >> and > >> - * so we don't need to include it in the count. > >> - */ > >> - spin_lock(&vpmu_lock); > >> - vpmu_count++; > >> - spin_unlock(&vpmu_lock); > >> - } > >> + if ( !vpmu_available(v) ) > >> + return 0; > >> > >> switch ( vendor ) > >> { > >> @@ -501,7 +490,7 @@ void vpmu_initialise(struct vcpu *v) > >> opt_vpmu_enabled = 0; > >> vpmu_mode = XENPMU_MODE_OFF; > >> } > >> - return; /* Don't bother restoring vpmu_count, VPMU is off forever > >> */ > >> + return -EINVAL; > >> } > >> > >> vpmu->hw_lapic_lvtpc = PMU_APIC_VECTOR | APIC_LVT_MASKED; > >> @@ -509,15 +498,63 @@ void vpmu_initialise(struct vcpu *v) > >> if ( ret ) > >> printk(XENLOG_G_WARNING "VPMU: Initialization failed for %pv\n", > >> v); > >> > >> - /* Intel needs to initialize VPMU ops even if VPMU is not in use */ > >> - if ( !is_priv_vpmu && > >> - (ret || (vpmu_mode == XENPMU_MODE_OFF) || > >> - (vpmu_mode == XENPMU_MODE_ALL)) ) > >> + return ret; > >> +} > >> + > >> +static void get_vpmu(struct vcpu *v) > >> +{ > >> + spin_lock(&vpmu_lock); > >> + > >> + /* > >> + * Count active VPMUs so that we won't try to change vpmu_mode while > >> + * they are in use. > >> + * vpmu_mode can be safely updated while dom0's VPMUs are active and > >> + * so we don't need to include it in the count. > >> + */ > >> + if ( !is_hardware_domain(v->domain) && > >> + (vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV)) ) > >> + { > >> + vpmu_count++; > >> + vpmu_set(vcpu_vpmu(v), VPMU_AVAILABLE); > >> + } > >> + else if ( is_hardware_domain(v->domain) && > >> + (vpmu_mode != XENPMU_MODE_OFF) ) > >> + vpmu_set(vcpu_vpmu(v), VPMU_AVAILABLE); > >> + > >> + spin_unlock(&vpmu_lock); > >> +} > >> + > >> +static void put_vpmu(struct vcpu *v) > >> +{ > >> + spin_lock(&vpmu_lock); > >> + > >> + if ( !vpmu_is_set(vcpu_vpmu(v), VPMU_AVAILABLE) ) > >> + goto out; > > just use !vpmu_available(v) > > Yes. > > > > >> + > >> + if ( !is_hardware_domain(v->domain) && > >> + (vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV)) ) > >> { > >> - spin_lock(&vpmu_lock); > >> vpmu_count--; > >> - spin_unlock(&vpmu_lock); > >> + vpmu_reset(vcpu_vpmu(v), VPMU_AVAILABLE); > >> } > >> + else if ( is_hardware_domain(v->domain) && > >> + (vpmu_mode != XENPMU_MODE_OFF) ) > >> + vpmu_reset(vcpu_vpmu(v), VPMU_AVAILABLE); > >> + > >> + out: > >> + spin_unlock(&vpmu_lock); > >> +} > >> + > >> +void vpmu_initialise(struct vcpu *v) > >> +{ > >> + get_vpmu(v); > >> + > >> + /* > >> + * Guests without LAPIC (i.e. PV) call vpmu_arch_initialise() > >> + * from pvpmu_init(). > >> + */ > > implication is that PV VPMU is not counted then? > > No. get_vpmu() is what does the counting now. Since we do > vcpu_initialise() -> vpmu_initialise() for all type of VCPUs both PV and > HVM VPMUs are counted here. But we defer arch-specific intialization > (which doesn't do the counting) for PV until the hypercall. > earlier comment said vpmu_count is to count active VPMUs. what's the definition of 'active' here? An uninitialized pv VPMU is also considered active? Thanks Kevin _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |