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

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3583 bytes --]

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


[-- Attachment #1.2: Type: text/html, Size: 4544 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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