* [PATCH] x86/APIC: don't make wrong implications on constants
@ 2014-10-01 7:30 Jan Beulich
2014-10-01 8:15 ` Andrew Cooper
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2014-10-01 7:30 UTC (permalink / raw)
To: xen-devel; +Cc: Boris Ostrovsky, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 3179 bytes --]
For the physical APIC oprofile code was abusing APIC_DM_NMI as a mask.
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>
--- 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);
[-- Attachment #2: x86-APIC-mode-handling.patch --]
[-- Type: text/plain, Size: 3229 bytes --]
x86/APIC: don't make wrong implications on constants
For the physical APIC oprofile code was abusing APIC_DM_NMI as a mask.
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>
--- 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);
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/APIC: don't make wrong implications on constants
2014-10-01 7:30 [PATCH] x86/APIC: don't make wrong implications on constants Jan Beulich
@ 2014-10-01 8:15 ` Andrew Cooper
2014-10-01 8:31 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2014-10-01 8:15 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Boris Ostrovsky, Keir Fraser
[-- 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/APIC: don't make wrong implications on constants
2014-10-01 8:15 ` Andrew Cooper
@ 2014-10-01 8:31 ` Jan Beulich
2014-10-01 8:36 ` Andrew Cooper
2014-10-01 8:41 ` David Vrabel
0 siblings, 2 replies; 5+ messages in thread
From: Jan Beulich @ 2014-10-01 8:31 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Boris Ostrovsky, Keir Fraser
>>> On 01.10.14 at 10:15, <andrew.cooper3@citrix.com> wrote:
> 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.
I had one there and the dropped it. I'm never really sure about the
punctuation differences between German and English.
>> For the virtual APIC a wrong assumption was made that LVTPC could be
Wouldn't a comma then be needed after APIC here too?
>> 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>
Thanks, Jan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/APIC: don't make wrong implications on constants
2014-10-01 8:31 ` Jan Beulich
@ 2014-10-01 8:36 ` Andrew Cooper
2014-10-01 8:41 ` David Vrabel
1 sibling, 0 replies; 5+ messages in thread
From: Andrew Cooper @ 2014-10-01 8:36 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Boris Ostrovsky, Keir Fraser
On 01/10/14 09:31, Jan Beulich wrote:
>>>> On 01.10.14 at 10:15, <andrew.cooper3@citrix.com> wrote:
>> 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.
> I had one there and the dropped it. I'm never really sure about the
> punctuation differences between German and English.
>
>>> For the virtual APIC a wrong assumption was made that LVTPC could be
> Wouldn't a comma then be needed after APIC here too?
Strictly speaking yes (and I thought so just after sending the email),
but "a wrong assumption" changes the structure sufficiently that it
isn't anything like as obvious.
~Andrew
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/APIC: don't make wrong implications on constants
2014-10-01 8:31 ` Jan Beulich
2014-10-01 8:36 ` Andrew Cooper
@ 2014-10-01 8:41 ` David Vrabel
1 sibling, 0 replies; 5+ messages in thread
From: David Vrabel @ 2014-10-01 8:41 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper; +Cc: xen-devel, Boris Ostrovsky, Keir Fraser
On 01/10/14 09:31, Jan Beulich wrote:
>>>> On 01.10.14 at 10:15, <andrew.cooper3@citrix.com> wrote:
>> 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.
>
> I had one there and the dropped it. I'm never really sure about the
> punctuation differences between German and English.
I think it's a missing "the"
"For the physical APIC the oprofile code was abusing APIC_DM_NMI as a mask."
David
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-10-01 8:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-01 7:30 [PATCH] x86/APIC: don't make wrong implications on constants Jan Beulich
2014-10-01 8:15 ` Andrew Cooper
2014-10-01 8:31 ` Jan Beulich
2014-10-01 8:36 ` Andrew Cooper
2014-10-01 8:41 ` David Vrabel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).