From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86/APIC: don't make wrong implications on constants Date: Wed, 1 Oct 2014 09:15:45 +0100 Message-ID: <542BB831.1050003@citrix.com> References: <542BC9AE020000780003B53D@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4674114473061255298==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XZF4i-0008J1-Ri for xen-devel@lists.xenproject.org; Wed, 01 Oct 2014 08:15:53 +0000 In-Reply-To: <542BC9AE020000780003B53D@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 , xen-devel Cc: Boris Ostrovsky , Keir Fraser List-Id: xen-devel@lists.xenproject.org --===============4674114473061255298== Content-Type: multipart/alternative; boundary="------------040601070604040102080600" --------------040601070604040102080600 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 01/10/14 08:30, Jan Beulich wrote: > For the physical APIC oprofile code was abusing APIC_DM_NMI as a mask. I think you need a comma after APIC for this sentence to parse correctly. > > For the virtual APIC a wrong assumption was made that LVTPC could be > programmed to only fixed or NMI delivery modes. While other modes are > invalid here, we still shouldn't inject an NMI into the guest in such > a case. Instead just do nothing. > > In the course of adjusting this it became obvious that what value > vpmu_do_interrupt() returns on its various return paths was pretty > arbitrary. With its only caller ignoring the return value, simply make > the function's return type "void". > > Signed-off-by: Jan Beulich Content wise, Reviewed-by: Andrew Cooper > > --- a/xen/arch/x86/hvm/vpmu.c > +++ b/xen/arch/x86/hvm/vpmu.c > @@ -82,7 +82,7 @@ int vpmu_do_rdmsr(unsigned int msr, uint > return 0; > } > > -int vpmu_do_interrupt(struct cpu_user_regs *regs) > +void vpmu_do_interrupt(struct cpu_user_regs *regs) > { > struct vcpu *v = current; > struct vpmu_struct *vpmu = vcpu_vpmu(v); > @@ -91,25 +91,23 @@ int vpmu_do_interrupt(struct cpu_user_re > { > struct vlapic *vlapic = vcpu_vlapic(v); > u32 vlapic_lvtpc; > - unsigned char int_vec; > > - if ( !vpmu->arch_vpmu_ops->do_interrupt(regs) ) > - return 0; > - > - if ( !is_vlapic_lvtpc_enabled(vlapic) ) > - return 1; > + if ( !vpmu->arch_vpmu_ops->do_interrupt(regs) || > + !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 > + switch ( GET_APIC_DELIVERY_MODE(vlapic_lvtpc) ) > + { > + case APIC_MODE_FIXED: > + vlapic_set_irq(vlapic, vlapic_lvtpc & APIC_VECTOR_MASK, 0); > + break; > + case APIC_MODE_NMI: > v->nmi_pending = 1; > - return 1; > + break; > + } > } > - > - return 0; > } > > void vpmu_do_cpuid(unsigned int input, > --- a/xen/arch/x86/oprofile/nmi_int.c > +++ b/xen/arch/x86/oprofile/nmi_int.c > @@ -274,7 +274,7 @@ static void nmi_cpu_stop(void * dummy) > * power on apic lvt contain a zero vector nr which are legal only for > * NMI delivery mode. So inhibit apic err before restoring lvtpc > */ > - if ( !(apic_read(APIC_LVTPC) & APIC_DM_NMI) > + if ( (apic_read(APIC_LVTPC) & APIC_MODE_MASK) != APIC_DM_NMI > || (apic_read(APIC_LVTPC) & APIC_LVT_MASKED) ) > { > printk("nmi_stop: APIC not good %ul\n", apic_read(APIC_LVTPC)); > --- a/xen/include/asm-x86/hvm/vpmu.h > +++ b/xen/include/asm-x86/hvm/vpmu.h > @@ -89,7 +89,7 @@ struct vpmu_struct { > > int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, uint64_t supported); > int vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content); > -int vpmu_do_interrupt(struct cpu_user_regs *regs); > +void vpmu_do_interrupt(struct cpu_user_regs *regs); > void vpmu_do_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, > unsigned int *ecx, unsigned int *edx); > void vpmu_initialise(struct vcpu *v); > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------040601070604040102080600 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 01/10/14 08:30, Jan Beulich wrote:
For the physical APIC oprofile code was abusing APIC_DM_NMI as a mask.

I think you need a comma after APIC for this sentence to parse correctly.


For the virtual APIC a wrong assumption was made that LVTPC could be
programmed to only fixed or NMI delivery modes. While other modes are
invalid here, we still shouldn't inject an NMI into the guest in such
a case. Instead just do nothing.

In the course of adjusting this it became obvious that what value
vpmu_do_interrupt() returns on its various return paths was pretty
arbitrary. With its only caller ignoring the return value, simply make
the function's return type "void".

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Content wise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>


--- a/xen/arch/x86/hvm/vpmu.c
+++ b/xen/arch/x86/hvm/vpmu.c
@@ -82,7 +82,7 @@ int vpmu_do_rdmsr(unsigned int msr, uint
     return 0;
 }
 
-int vpmu_do_interrupt(struct cpu_user_regs *regs)
+void vpmu_do_interrupt(struct cpu_user_regs *regs)
 {
     struct vcpu *v = current;
     struct vpmu_struct *vpmu = vcpu_vpmu(v);
@@ -91,25 +91,23 @@ int vpmu_do_interrupt(struct cpu_user_re
     {
         struct vlapic *vlapic = vcpu_vlapic(v);
         u32 vlapic_lvtpc;
-        unsigned char int_vec;
 
-        if ( !vpmu->arch_vpmu_ops->do_interrupt(regs) )
-            return 0;
-
-        if ( !is_vlapic_lvtpc_enabled(vlapic) )
-            return 1;
+        if ( !vpmu->arch_vpmu_ops->do_interrupt(regs) ||
+             !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
+        switch ( GET_APIC_DELIVERY_MODE(vlapic_lvtpc) )
+        {
+        case APIC_MODE_FIXED:
+            vlapic_set_irq(vlapic, vlapic_lvtpc & APIC_VECTOR_MASK, 0);
+            break;
+        case APIC_MODE_NMI:
             v->nmi_pending = 1;
-        return 1;
+            break;
+        }
     }
-
-    return 0;
 }
 
 void vpmu_do_cpuid(unsigned int input,
--- a/xen/arch/x86/oprofile/nmi_int.c
+++ b/xen/arch/x86/oprofile/nmi_int.c
@@ -274,7 +274,7 @@ static void nmi_cpu_stop(void * dummy)
 	 * power on apic lvt contain a zero vector nr which are legal only for
 	 * NMI delivery mode. So inhibit apic err before restoring lvtpc
 	 */
-	if ( !(apic_read(APIC_LVTPC) & APIC_DM_NMI)
+	if ( (apic_read(APIC_LVTPC) & APIC_MODE_MASK) != APIC_DM_NMI
 	     || (apic_read(APIC_LVTPC) & APIC_LVT_MASKED) )
 	{
 		printk("nmi_stop: APIC not good %ul\n", apic_read(APIC_LVTPC));
--- a/xen/include/asm-x86/hvm/vpmu.h
+++ b/xen/include/asm-x86/hvm/vpmu.h
@@ -89,7 +89,7 @@ struct vpmu_struct {
 
 int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, uint64_t supported);
 int vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content);
-int vpmu_do_interrupt(struct cpu_user_regs *regs);
+void vpmu_do_interrupt(struct cpu_user_regs *regs);
 void vpmu_do_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
                                        unsigned int *ecx, unsigned int *edx);
 void vpmu_initialise(struct vcpu *v);






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

--------------040601070604040102080600-- --===============4674114473061255298== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============4674114473061255298==--