Re: [Xen-devel] [PATCH] x86/APIC: don't make wrong implications on constants

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>

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

--- 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

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