From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v6 17/19] x86/VPMU: NMI-based VPMU support Date: Mon, 26 May 2014 22:57:01 -0400 Message-ID: <5383FEFD.6010207@oracle.com> References: <1399996413-1998-1-git-send-email-boris.ostrovsky@oracle.com> <1399996413-1998-18-git-send-email-boris.ostrovsky@oracle.com> <538380260200007800015D47@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: <538380260200007800015D47@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, keir@xen.org, suravee.suthikulpanit@amd.com, andrew.cooper3@citrix.com, donald.d.dugger@intel.com, xen-devel@lists.xen.org, dietmar.hahn@ts.fujitsu.com, jun.nakajima@intel.com List-Id: xen-devel@lists.xenproject.org On 05/26/2014 11:55 AM, Jan Beulich wrote: > >> +static void vpmu_send_nmi(struct vcpu *v) >> +{ >> + struct vlapic *vlapic; >> + u32 vlapic_lvtpc; >> + unsigned char int_vec; >> + >> + ASSERT( is_hvm_vcpu(v) ); >> + >> + vlapic = vcpu_vlapic(v); >> + if ( !is_vlapic_lvtpc_enabled(vlapic) ) >> + return; >> + >> + vlapic_lvtpc = vlapic_get_reg(vlapic, APIC_LVTPC); >> + int_vec = vlapic_lvtpc & APIC_VECTOR_MASK; >> + >> + if ( GET_APIC_DELIVERY_MODE(vlapic_lvtpc) == APIC_MODE_FIXED ) >> + vlapic_set_irq(vcpu_vlapic(v), int_vec, 0); >> + else >> + v->nmi_pending = 1; > I realize this is only the result of code movement, but what is this > if/else pair doing? To be honest, I don't know. This code has been in VPMU since day one and I just moved it here. I am not sure whether 'if' clause has ever been tested since perf (and before that, oprofile) use NMIs for PMU interrupts. But I should probably at least rename the routine to something like send_pmu_interrupt (and move int_vec calculation into the 'if' clause). >> + >> + if ( (vpmu_mode & XENPMU_MODE_PRIV) || >> + (sampled->domain->domain_id >= DOMID_FIRST_RESERVED) ) >> + v = hardware_domain->vcpu[smp_processor_id() % >> + hardware_domain->max_vcpus]; >> + else >> + { >> + if ( is_hvm_domain(sampled->domain) ) >> + { >> + vpmu_send_nmi(sampled); >> + return; >> + } >> + v = sampled; >> + } >> + >> + regs = &v->arch.vpmu.xenpmu_data->pmu.r.regs; >> + if ( !is_pv_domain(sampled->domain) ) > Even if it means the same, has_hvm_container_vcpu(sampled) > please: You're guarding an operation here that requires a HVM > container. I am removing PVH-enabling patch (which currently comes after this one) so all is_*_domain() will hopefully make sense at the time they are added. > >> + { >> + struct segment_register cs; >> + >> + hvm_get_segment_register(sampled, x86_seg_cs, &cs); > I hope you understand that what you read here is only implicitly (due > to the softirq getting serviced before the guest gets re-entered) the > current value. Or wait - is it at all? What if a scheduler softirq first > causes the guest to be de-scheduled and another CPU manages to > pick it up before you get here? Based on a similar comment from Kevin I added a check for pending PMU interrupts (i.e. a call to this routine) in vpmu_save() which is called during context switch. We should therefore always pick the current value of guest's CS. > >> @@ -472,6 +574,21 @@ static int pvpmu_init(struct domain *d, xen_pmu_params_t *params) >> return -EINVAL; >> } >> >> + if ( !pvpmu_initted ) >> + { >> + if (reserve_lapic_nmi() == 0) > Coding style. > >> + set_nmi_callback(pmu_nmi_interrupt); >> + else >> + { >> + printk("Failed to reserve PMU NMI\n"); >> + put_page(page); >> + return -EBUSY; >> + } >> + open_softirq(PMU_SOFTIRQ, pmu_softnmi); >> + >> + pvpmu_initted = 1; > Is it excluded that you get two racing pvpmu_init() calls (i.e. are > these exclusively coming from e.g. a domctl)? If not, better > serialization would be needed here. In Linux the first call (it's not a domctl but a dedicated hypercall) is done by boot CPU pre-SMP so I think we should be safe. But then there may be non-Linux users so a lock wouldn't hurt. -boris