* [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()
@ 2012-11-17 15:16 Dan Carpenter
2012-11-20 0:48 ` Russ Anderson
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2012-11-17 15:16 UTC (permalink / raw)
To: Thomas Gleixner, Jack Steiner, Dimitri Sivanich
Cc: Ingo Molnar, H. Peter Anvin, x86, Russ Anderson, linux-kernel,
kernel-janitors
This is a static checker fix. The problem is that we store the bits
from "uv_apicid_hibits" into "apicid" (the high 16 bits) but then we
shift it 16 bit to the left. "apicid" is an int so it wraps and we lose
them.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This is for an SGI product, and I can't test it.
diff --git a/arch/x86/include/asm/uv/uv_hub.h b/arch/x86/include/asm/uv/uv_hub.h
index 21f7385..f3a0f91 100644
--- a/arch/x86/include/asm/uv/uv_hub.h
+++ b/arch/x86/include/asm/uv/uv_hub.h
@@ -573,7 +573,7 @@ static inline void uv_set_cpu_scir_bits(int cpu, unsigned char value)
}
extern unsigned int uv_apicid_hibits;
-static unsigned long uv_hub_ipi_value(int apicid, int vector, int mode)
+static unsigned long uv_hub_ipi_value(ulong apicid, ulong vector, ulong mode)
{
apicid |= uv_apicid_hibits;
return (1UL << UVH_IPI_INT_SEND_SHFT) |
diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index 8cfade9..6d93b2f 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -194,13 +194,13 @@ static int __cpuinit uv_wakeup_secondary(int phys_apicid, unsigned long start_ri
pnode = uv_apicid_to_pnode(phys_apicid);
phys_apicid |= uv_apicid_hibits;
val = (1UL << UVH_IPI_INT_SEND_SHFT) |
- (phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
+ ((unsigned long)phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12) |
APIC_DM_INIT;
uv_write_global_mmr64(pnode, UVH_IPI_INT, val);
val = (1UL << UVH_IPI_INT_SEND_SHFT) |
- (phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
+ ((unsigned long)phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12) |
APIC_DM_STARTUP;
uv_write_global_mmr64(pnode, UVH_IPI_INT, val);
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()
2012-11-17 15:16 [patch] x86, UV: integer wrap bug in uv_hub_ipi_value() Dan Carpenter
@ 2012-11-20 0:48 ` Russ Anderson
2012-11-20 4:28 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Russ Anderson @ 2012-11-20 0:48 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Gleixner, Jack Steiner, Dimitri Sivanich, Ingo Molnar,
H. Peter Anvin, x86, linux-kernel, kernel-janitors, rja
On Sat, Nov 17, 2012 at 06:16:11PM +0300, Dan Carpenter wrote:
> This is a static checker fix. The problem is that we store the bits
> from "uv_apicid_hibits" into "apicid" (the high 16 bits) but then we
> shift it 16 bit to the left. "apicid" is an int so it wraps and we lose
> them.
Is this the complete patch? phys_apicid is an int, but gets
cast as unsigned long. Doesn't phys_apicid also have to be
changed to unsigned long? And why ulong instead of uint (on x86_64)?
I agree with changing signed to unsigned where appropriate, but
this looks like a partial fix. Am I missing something?
Thanks.
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> This is for an SGI product, and I can't test it.
>
> diff --git a/arch/x86/include/asm/uv/uv_hub.h b/arch/x86/include/asm/uv/uv_hub.h
> index 21f7385..f3a0f91 100644
> --- a/arch/x86/include/asm/uv/uv_hub.h
> +++ b/arch/x86/include/asm/uv/uv_hub.h
> @@ -573,7 +573,7 @@ static inline void uv_set_cpu_scir_bits(int cpu, unsigned char value)
> }
>
> extern unsigned int uv_apicid_hibits;
> -static unsigned long uv_hub_ipi_value(int apicid, int vector, int mode)
> +static unsigned long uv_hub_ipi_value(ulong apicid, ulong vector, ulong mode)
> {
> apicid |= uv_apicid_hibits;
> return (1UL << UVH_IPI_INT_SEND_SHFT) |
> diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
> index 8cfade9..6d93b2f 100644
> --- a/arch/x86/kernel/apic/x2apic_uv_x.c
> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
> @@ -194,13 +194,13 @@ static int __cpuinit uv_wakeup_secondary(int phys_apicid, unsigned long start_ri
> pnode = uv_apicid_to_pnode(phys_apicid);
> phys_apicid |= uv_apicid_hibits;
> val = (1UL << UVH_IPI_INT_SEND_SHFT) |
> - (phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
> + ((unsigned long)phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
> ((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12) |
> APIC_DM_INIT;
> uv_write_global_mmr64(pnode, UVH_IPI_INT, val);
>
> val = (1UL << UVH_IPI_INT_SEND_SHFT) |
> - (phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
> + ((unsigned long)phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
> ((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12) |
> APIC_DM_STARTUP;
> uv_write_global_mmr64(pnode, UVH_IPI_INT, val);
--
Russ Anderson, OS RAS/Partitioning Project Lead
SGI - Silicon Graphics Inc rja@sgi.com
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()
2012-11-20 0:48 ` Russ Anderson
@ 2012-11-20 4:28 ` Dan Carpenter
2012-11-20 4:55 ` H. Peter Anvin
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Dan Carpenter @ 2012-11-20 4:28 UTC (permalink / raw)
To: Russ Anderson
Cc: Thomas Gleixner, Jack Steiner, Dimitri Sivanich, Ingo Molnar,
H. Peter Anvin, x86, linux-kernel, kernel-janitors, rja
On Mon, Nov 19, 2012 at 06:48:34PM -0600, Russ Anderson wrote:
> On Sat, Nov 17, 2012 at 06:16:11PM +0300, Dan Carpenter wrote:
> > This is a static checker fix. The problem is that we store the bits
> > from "uv_apicid_hibits" into "apicid" (the high 16 bits) but then we
> > shift it 16 bit to the left. "apicid" is an int so it wraps and we lose
> > them.
>
> Is this the complete patch? phys_apicid is an int, but gets
> cast as unsigned long. Doesn't phys_apicid also have to be
> changed to unsigned long? And why ulong instead of uint (on x86_64)?
Uint is 32bit across all arches in linux and unix, according to
wikipedia. The wakeup_secondary_cpu() function pointer takes an int
so I couldn't change the parameter.
>
> I agree with changing signed to unsigned where appropriate, but
> this looks like a partial fix. Am I missing something?
>
I do feel a little embarrassed that I didn't use "unsigned long"
consistently. I just used ulong to make the line a bit shorter, but
I could redo it with "unsigned long" if you want.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()
2012-11-20 4:28 ` Dan Carpenter
@ 2012-11-20 4:55 ` H. Peter Anvin
2012-11-20 11:10 ` Alan Cox
2012-11-20 17:07 ` Russ Anderson
2 siblings, 0 replies; 14+ messages in thread
From: H. Peter Anvin @ 2012-11-20 4:55 UTC (permalink / raw)
To: Dan Carpenter
Cc: Russ Anderson, Thomas Gleixner, Jack Steiner, Dimitri Sivanich,
Ingo Molnar, x86, linux-kernel, kernel-janitors, rja
On 11/19/2012 08:28 PM, Dan Carpenter wrote:
>
> I do feel a little embarrassed that I didn't use "unsigned long"
> consistently. I just used ulong to make the line a bit shorter, but
> I could redo it with "unsigned long" if you want.
>
Yes, "ulong" is not idiomatic in the Linux kernel.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()
2012-11-20 4:28 ` Dan Carpenter
2012-11-20 4:55 ` H. Peter Anvin
@ 2012-11-20 11:10 ` Alan Cox
2012-11-20 16:27 ` Russ Anderson
2012-11-20 17:07 ` Russ Anderson
2 siblings, 1 reply; 14+ messages in thread
From: Alan Cox @ 2012-11-20 11:10 UTC (permalink / raw)
To: Dan Carpenter
Cc: Russ Anderson, Thomas Gleixner, Jack Steiner, Dimitri Sivanich,
Ingo Molnar, H. Peter Anvin, x86, linux-kernel, kernel-janitors,
rja
On Tue, 20 Nov 2012 07:28:56 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Nov 19, 2012 at 06:48:34PM -0600, Russ Anderson wrote:
> > On Sat, Nov 17, 2012 at 06:16:11PM +0300, Dan Carpenter wrote:
> > > This is a static checker fix. The problem is that we store the bits
> > > from "uv_apicid_hibits" into "apicid" (the high 16 bits) but then we
> > > shift it 16 bit to the left. "apicid" is an int so it wraps and we lose
> > > them.
> >
> > Is this the complete patch? phys_apicid is an int, but gets
> > cast as unsigned long. Doesn't phys_apicid also have to be
> > changed to unsigned long? And why ulong instead of uint (on x86_64)?
>
> Uint is 32bit across all arches in linux and unix, according to
> wikipedia.
For Linux yes, if Wackypedia claims it for "unix" it's wrong 8) and varies
between 16 and 36bits that I can think of.
If you specifically want 32bits use "u32", it's not going to make any
actual difference to unsigned int but it makes the requirement explicit.
Alan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()
2012-11-20 11:10 ` Alan Cox
@ 2012-11-20 16:27 ` Russ Anderson
2012-11-20 17:09 ` H. Peter Anvin
0 siblings, 1 reply; 14+ messages in thread
From: Russ Anderson @ 2012-11-20 16:27 UTC (permalink / raw)
To: Alan Cox
Cc: Dan Carpenter, Thomas Gleixner, Dimitri Sivanich, Ingo Molnar,
H. Peter Anvin, x86, linux-kernel, kernel-janitors, Russ Anderson
On Tue, Nov 20, 2012 at 11:10:55AM +0000, Alan Cox wrote:
> On Tue, 20 Nov 2012 07:28:56 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Mon, Nov 19, 2012 at 06:48:34PM -0600, Russ Anderson wrote:
> > > On Sat, Nov 17, 2012 at 06:16:11PM +0300, Dan Carpenter wrote:
> > > > This is a static checker fix. The problem is that we store the bits
> > > > from "uv_apicid_hibits" into "apicid" (the high 16 bits) but then we
> > > > shift it 16 bit to the left. "apicid" is an int so it wraps and we lose
> > > > them.
> > >
> > > Is this the complete patch? phys_apicid is an int, but gets
> > > cast as unsigned long. Doesn't phys_apicid also have to be
> > > changed to unsigned long? And why ulong instead of uint (on x86_64)?
> >
> > Uint is 32bit across all arches in linux and unix, according to
> > wikipedia.
>
> For Linux yes, if Wackypedia claims it for "unix" it's wrong 8) and varies
> between 16 and 36bits that I can think of.
64 bit int on old Cray cpus (unicos). :-)
> If you specifically want 32bits use "u32", it's not going to make any
> actual difference to unsigned int but it makes the requirement explicit.
I very much agree. I prefer u32, u64 (etc) because they are
unambiguous. It removes all doubt as to the actual meaning.
Conversly, the fact that "long" has different meanings makes
it at best problematic. Was the code written assuming "long"
was 32 or 64 bits? Having data types that can have different
sizes is just asking for trouble.
--
Russ Anderson, OS RAS/Partitioning Project Lead
SGI - Silicon Graphics Inc rja@sgi.com
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()
2012-11-20 16:27 ` Russ Anderson
@ 2012-11-20 17:09 ` H. Peter Anvin
0 siblings, 0 replies; 14+ messages in thread
From: H. Peter Anvin @ 2012-11-20 17:09 UTC (permalink / raw)
To: Russ Anderson
Cc: Alan Cox, Dan Carpenter, Thomas Gleixner, Dimitri Sivanich,
Ingo Molnar, x86, linux-kernel, kernel-janitors
On 11/20/2012 08:27 AM, Russ Anderson wrote:
>
> I very much agree. I prefer u32, u64 (etc) because they are
> unambiguous. It removes all doubt as to the actual meaning.
>
> Conversly, the fact that "long" has different meanings makes
> it at best problematic. Was the code written assuming "long"
> was 32 or 64 bits? Having data types that can have different
> sizes is just asking for trouble.
>
In the Linux kernel context, "long" effectively means the native size
(size_t/intptr_t/ptrdiff_t).
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()
2012-11-20 4:28 ` Dan Carpenter
2012-11-20 4:55 ` H. Peter Anvin
2012-11-20 11:10 ` Alan Cox
@ 2012-11-20 17:07 ` Russ Anderson
2012-11-21 7:39 ` Dan Carpenter
2 siblings, 1 reply; 14+ messages in thread
From: Russ Anderson @ 2012-11-20 17:07 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Gleixner, Dimitri Sivanich, Ingo Molnar, H. Peter Anvin,
x86, linux-kernel, kernel-janitors, Russ Anderson
On Tue, Nov 20, 2012 at 07:28:56AM +0300, Dan Carpenter wrote:
> On Mon, Nov 19, 2012 at 06:48:34PM -0600, Russ Anderson wrote:
> > On Sat, Nov 17, 2012 at 06:16:11PM +0300, Dan Carpenter wrote:
> > > This is a static checker fix. The problem is that we store the bits
> > > from "uv_apicid_hibits" into "apicid" (the high 16 bits) but then we
> > > shift it 16 bit to the left. "apicid" is an int so it wraps and we lose
> > > them.
> >
> > Is this the complete patch? phys_apicid is an int, but gets
> > cast as unsigned long. Doesn't phys_apicid also have to be
> > changed to unsigned long? And why ulong instead of uint (on x86_64)?
>
> Uint is 32bit across all arches in linux and unix, according to
> wikipedia.
But long isn't 32bit across all arches.
http://software.intel.com/en-us/articles/size-of-long-integer-type-on-different-architecture-and-os
> The wakeup_secondary_cpu() function pointer takes an int
> so I couldn't change the parameter.
Yes. The real problem is much of the apicid code is based
on signed int (ie parameters in struct apic). Not sure why
they chose to make it signed, but since they did that decision
ripples through the rest of the code. Changing it to unsigned
means changing struct apic, which likewise will ripple through
the rest of the code. That is a much bigger change than
your patch deals with.
> > I agree with changing signed to unsigned where appropriate, but
> > this looks like a partial fix. Am I missing something?
> >
>
> I do feel a little embarrassed that I didn't use "unsigned long"
> consistently. I just used ulong to make the line a bit shorter, but
> I could redo it with "unsigned long" if you want.
The issue isn't "ulong" vs "unsigned long". The issue
is int is 32 bit and long is 64 bit on x86_64. Your
patch is casting the value as an "unsigned long" (64 bit
on x86_64) into an int (32 bit). I don't think that
was your intent.
This does highlight the problematic nature of "long" being
different size on different architectures.
--
Russ Anderson, OS RAS/Partitioning Project Lead
SGI - Silicon Graphics Inc rja@sgi.com
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()
2012-11-20 17:07 ` Russ Anderson
@ 2012-11-21 7:39 ` Dan Carpenter
2012-12-02 10:44 ` [patch v2] " Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2012-11-21 7:39 UTC (permalink / raw)
To: Russ Anderson
Cc: Thomas Gleixner, Dimitri Sivanich, Ingo Molnar, H. Peter Anvin,
x86, linux-kernel, kernel-janitors
On Tue, Nov 20, 2012 at 11:07:25AM -0600, Russ Anderson wrote:
> The issue isn't "ulong" vs "unsigned long". The issue
> is int is 32 bit and long is 64 bit on x86_64. Your
> patch is casting the value as an "unsigned long" (64 bit
> on x86_64) into an int (32 bit). I don't think that
> was your intent.
Wait what? I only did int => long casts, not the other way around.
It occured to me to use u64 but this code is only compiled on x86_64
and I wrote my patch to match the surrounding context.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* [patch v2] x86, UV: integer wrap bug in uv_hub_ipi_value()
2012-11-21 7:39 ` Dan Carpenter
@ 2012-12-02 10:44 ` Dan Carpenter
2012-12-02 15:33 ` walter harms
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2012-12-02 10:44 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ingo Molnar, H. Peter Anvin, x86, Russ Anderson, linux-kernel,
kernel-janitors
This is a static checker fix. The problem is that we store the bits
from "uv_apicid_hibits" into "apicid" (the high 16 bits) but then we
shift it 16 bit to the left. "apicid" is an int so it wraps and we lose
them.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Style fix. Don't use ulong.
I don't have this hardware so I can't test it. There may also be other
bugs which this patch does not addressed. These files are only compiled
on x86_64 and "unsigned long" is used throughout to mean 64 bits.
diff --git a/arch/x86/include/asm/uv/uv_hub.h b/arch/x86/include/asm/uv/uv_hub.h
index 21f7385..e7a83d5 100644
--- a/arch/x86/include/asm/uv/uv_hub.h
+++ b/arch/x86/include/asm/uv/uv_hub.h
@@ -577,7 +577,7 @@ static unsigned long uv_hub_ipi_value(int apicid, int vector, int mode)
{
apicid |= uv_apicid_hibits;
return (1UL << UVH_IPI_INT_SEND_SHFT) |
- ((apicid) << UVH_IPI_INT_APIC_ID_SHFT) |
+ ((unsigned long)apicid << UVH_IPI_INT_APIC_ID_SHFT) |
(mode << UVH_IPI_INT_DELIVERY_MODE_SHFT) |
(vector << UVH_IPI_INT_VECTOR_SHFT);
}
diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index 8cfade9..6d93b2f 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -194,13 +194,13 @@ static int __cpuinit uv_wakeup_secondary(int phys_apicid, unsigned long start_ri
pnode = uv_apicid_to_pnode(phys_apicid);
phys_apicid |= uv_apicid_hibits;
val = (1UL << UVH_IPI_INT_SEND_SHFT) |
- (phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
+ ((unsigned long)phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12) |
APIC_DM_INIT;
uv_write_global_mmr64(pnode, UVH_IPI_INT, val);
val = (1UL << UVH_IPI_INT_SEND_SHFT) |
- (phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
+ ((unsigned long)phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12) |
APIC_DM_STARTUP;
uv_write_global_mmr64(pnode, UVH_IPI_INT, val);
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [patch v2] x86, UV: integer wrap bug in uv_hub_ipi_value()
2012-12-02 10:44 ` [patch v2] " Dan Carpenter
@ 2012-12-02 15:33 ` walter harms
2012-12-02 17:28 ` [patch v3] " Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: walter harms @ 2012-12-02 15:33 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Russ Anderson,
linux-kernel, kernel-janitors
Am 02.12.2012 11:44, schrieb Dan Carpenter:
> This is a static checker fix. The problem is that we store the bits
> from "uv_apicid_hibits" into "apicid" (the high 16 bits) but then we
> shift it 16 bit to the left. "apicid" is an int so it wraps and we lose
> them.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: Style fix. Don't use ulong.
>
> I don't have this hardware so I can't test it. There may also be other
> bugs which this patch does not addressed. These files are only compiled
> on x86_64 and "unsigned long" is used throughout to mean 64 bits.
>
> diff --git a/arch/x86/include/asm/uv/uv_hub.h b/arch/x86/include/asm/uv/uv_hub.h
> index 21f7385..e7a83d5 100644
> --- a/arch/x86/include/asm/uv/uv_hub.h
> +++ b/arch/x86/include/asm/uv/uv_hub.h
> @@ -577,7 +577,7 @@ static unsigned long uv_hub_ipi_value(int apicid, int vector, int mode)
> {
> apicid |= uv_apicid_hibits;
> return (1UL << UVH_IPI_INT_SEND_SHFT) |
> - ((apicid) << UVH_IPI_INT_APIC_ID_SHFT) |
> + ((unsigned long)apicid << UVH_IPI_INT_APIC_ID_SHFT) |
> (mode << UVH_IPI_INT_DELIVERY_MODE_SHFT) |
> (vector << UVH_IPI_INT_VECTOR_SHFT);
> }
> diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
> index 8cfade9..6d93b2f 100644
> --- a/arch/x86/kernel/apic/x2apic_uv_x.c
> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
> @@ -194,13 +194,13 @@ static int __cpuinit uv_wakeup_secondary(int phys_apicid, unsigned long start_ri
> pnode = uv_apicid_to_pnode(phys_apicid);
> phys_apicid |= uv_apicid_hibits;
> val = (1UL << UVH_IPI_INT_SEND_SHFT) |
> - (phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
> + ((unsigned long)phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
> ((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12) |
> APIC_DM_INIT;
> uv_write_global_mmr64(pnode, UVH_IPI_INT, val);
>
> val = (1UL << UVH_IPI_INT_SEND_SHFT) |
> - (phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
> + ((unsigned long)phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
> ((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12) |
> APIC_DM_STARTUP;
> uv_write_global_mmr64(pnode, UVH_IPI_INT, val);
making this more readable is hard but what is about:
val=(1UL << UVH_IPI_INT_SEND_SHFT) |
((unsigned long)phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12);
uv_write_global_mmr64(pnode, UVH_IPI_INT, val|APIC_DM_INIT);
uv_write_global_mmr64(pnode, UVH_IPI_INT, val|APIC_DM_STARTUP);
just my 2 cents,
re
wh
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* [patch v3] x86, UV: integer wrap bug in uv_hub_ipi_value()
2012-12-02 15:33 ` walter harms
@ 2012-12-02 17:28 ` Dan Carpenter
2012-12-02 17:35 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2012-12-02 17:28 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ingo Molnar, H. Peter Anvin, x86, Russ Anderson, linux-kernel,
kernel-janitors, walter harms
This is a static checker fix. The problem is that we store the bits
from "uv_apicid_hibits" into "apicid" (the high 16 bits) but then we
shift it 16 bit to the left. "apicid" is an int so it wraps and we lose
them.
I have also simplified uv_wakeup_secondary() a little based on a
suggestion.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Style fix. Don't use ulong.
v3: Another style fix to uv_wakeup_secondary() based on Walter Harms's
suggestion.
I don't have this hardware so I can't test it. There may also be other
bugs which this patch does not addressed. These files are only compiled
on x86_64 and "unsigned long" is used throughout to mean 64 bits.
diff --git a/arch/x86/include/asm/uv/uv_hub.h b/arch/x86/include/asm/uv/uv_hub.h
index 21f7385..e7a83d5 100644
--- a/arch/x86/include/asm/uv/uv_hub.h
+++ b/arch/x86/include/asm/uv/uv_hub.h
@@ -577,7 +577,7 @@ static unsigned long uv_hub_ipi_value(int apicid, int vector, int mode)
{
apicid |= uv_apicid_hibits;
return (1UL << UVH_IPI_INT_SEND_SHFT) |
- ((apicid) << UVH_IPI_INT_APIC_ID_SHFT) |
+ ((unsigned long)apicid << UVH_IPI_INT_APIC_ID_SHFT) |
(mode << UVH_IPI_INT_DELIVERY_MODE_SHFT) |
(vector << UVH_IPI_INT_VECTOR_SHFT);
}
diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index 8cfade9..251b36f 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -194,16 +194,10 @@ static int __cpuinit uv_wakeup_secondary(int phys_apicid, unsigned long start_ri
pnode = uv_apicid_to_pnode(phys_apicid);
phys_apicid |= uv_apicid_hibits;
val = (1UL << UVH_IPI_INT_SEND_SHFT) |
- (phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
- ((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12) |
- APIC_DM_INIT;
- uv_write_global_mmr64(pnode, UVH_IPI_INT, val);
-
- val = (1UL << UVH_IPI_INT_SEND_SHFT) |
- (phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
- ((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12) |
- APIC_DM_STARTUP;
- uv_write_global_mmr64(pnode, UVH_IPI_INT, val);
+ ((unsigned long)phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
+ ((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12);
+ uv_write_global_mmr64(pnode, UVH_IPI_INT, val | APIC_DM_INIT);
+ uv_write_global_mmr64(pnode, UVH_IPI_INT, val | APIC_DM_STARTUP);
atomic_set(&init_deasserted, 1);
#endif
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [patch v3] x86, UV: integer wrap bug in uv_hub_ipi_value()
2012-12-02 17:28 ` [patch v3] " Dan Carpenter
@ 2012-12-02 17:35 ` Dan Carpenter
2012-12-03 7:58 ` [patch v4] " Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2012-12-02 17:35 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ingo Molnar, H. Peter Anvin, x86, Russ Anderson, linux-kernel,
kernel-janitors, walter harms
On Sun, Dec 02, 2012 at 08:28:43PM +0300, Dan Carpenter wrote:
> This is a static checker fix. The problem is that we store the bits
> from "uv_apicid_hibits" into "apicid" (the high 16 bits) but then we
> shift it 16 bit to the left. "apicid" is an int so it wraps and we lose
> them.
>
> I have also simplified uv_wakeup_secondary() a little based on a
> suggestion.
This was supposed to say "suggestions from Walter Harms." but I
pressed "u" in vim instead of "i" because they are next to each
other so that that got deleted. @%$@#$%@%$. I'll resend.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* [patch v4] x86, UV: integer wrap bug in uv_hub_ipi_value()
2012-12-02 17:35 ` Dan Carpenter
@ 2012-12-03 7:58 ` Dan Carpenter
0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2012-12-03 7:58 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ingo Molnar, H. Peter Anvin, x86, Russ Anderson, linux-kernel,
kernel-janitors, walter harms
This is a static checker fix. The problem is that we store the bits
from "uv_apicid_hibits" into "apicid" (the high 16 bits) but then we
shift it 16 bit to the left. "apicid" is an int so it wraps and we lose
them.
I have also simplified uv_wakeup_secondary() a little based on a
suggestion from Walter Harms.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Style fix. Don't use ulong.
v3: Another style fix to uv_wakeup_secondary() based on Walter Harms's
suggestion.
v4: Fix typo in commit message.
I don't have this hardware so I can't test it. There may also be other
bugs which this patch does not addressed. These files are only compiled
on x86_64 and "unsigned long" is used throughout to mean 64 bits.
diff --git a/arch/x86/include/asm/uv/uv_hub.h b/arch/x86/include/asm/uv/uv_hub.h
index 21f7385..e7a83d5 100644
--- a/arch/x86/include/asm/uv/uv_hub.h
+++ b/arch/x86/include/asm/uv/uv_hub.h
@@ -577,7 +577,7 @@ static unsigned long uv_hub_ipi_value(int apicid, int vector, int mode)
{
apicid |= uv_apicid_hibits;
return (1UL << UVH_IPI_INT_SEND_SHFT) |
- ((apicid) << UVH_IPI_INT_APIC_ID_SHFT) |
+ ((unsigned long)apicid << UVH_IPI_INT_APIC_ID_SHFT) |
(mode << UVH_IPI_INT_DELIVERY_MODE_SHFT) |
(vector << UVH_IPI_INT_VECTOR_SHFT);
}
diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index 8cfade9..251b36f 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -194,16 +194,10 @@ static int __cpuinit uv_wakeup_secondary(int phys_apicid, unsigned long start_ri
pnode = uv_apicid_to_pnode(phys_apicid);
phys_apicid |= uv_apicid_hibits;
val = (1UL << UVH_IPI_INT_SEND_SHFT) |
- (phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
- ((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12) |
- APIC_DM_INIT;
- uv_write_global_mmr64(pnode, UVH_IPI_INT, val);
-
- val = (1UL << UVH_IPI_INT_SEND_SHFT) |
- (phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
- ((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12) |
- APIC_DM_STARTUP;
- uv_write_global_mmr64(pnode, UVH_IPI_INT, val);
+ ((unsigned long)phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
+ ((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12);
+ uv_write_global_mmr64(pnode, UVH_IPI_INT, val | APIC_DM_INIT);
+ uv_write_global_mmr64(pnode, UVH_IPI_INT, val | APIC_DM_STARTUP);
atomic_set(&init_deasserted, 1);
#endif
^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2012-12-03 7:58 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-17 15:16 [patch] x86, UV: integer wrap bug in uv_hub_ipi_value() Dan Carpenter
2012-11-20 0:48 ` Russ Anderson
2012-11-20 4:28 ` Dan Carpenter
2012-11-20 4:55 ` H. Peter Anvin
2012-11-20 11:10 ` Alan Cox
2012-11-20 16:27 ` Russ Anderson
2012-11-20 17:09 ` H. Peter Anvin
2012-11-20 17:07 ` Russ Anderson
2012-11-21 7:39 ` Dan Carpenter
2012-12-02 10:44 ` [patch v2] " Dan Carpenter
2012-12-02 15:33 ` walter harms
2012-12-02 17:28 ` [patch v3] " Dan Carpenter
2012-12-02 17:35 ` Dan Carpenter
2012-12-03 7:58 ` [patch v4] " Dan Carpenter
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).