[patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[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>
---
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);

[patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[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>
---
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);

Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[Russ Anderson]

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

Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[Russ Anderson]

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

Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[Dan Carpenter]

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

Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[Dan Carpenter]

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

Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[H. Peter Anvin]

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.

Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[H. Peter Anvin]

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.

Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[Alan Cox]

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

Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[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

Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[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

Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[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

Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[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

Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[H. Peter Anvin]

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.

Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[H. Peter Anvin]

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.

Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[Dan Carpenter]

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

Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

[Dan Carpenter]

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

[patch v2] x86, UV: integer wrap bug in uv_hub_ipi_value()

[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);

[patch v2] x86, UV: integer wrap bug in uv_hub_ipi_value()

[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);

Re: [patch v2] x86, UV: integer wrap bug in uv_hub_ipi_value()

[walter harms]

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

Re: [patch v2] x86, UV: integer wrap bug in uv_hub_ipi_value()

[walter harms]

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

[patch v3] x86, UV: integer wrap bug in uv_hub_ipi_value()

[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.

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

[patch v3] x86, UV: integer wrap bug in uv_hub_ipi_value()

[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.

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

Re: [patch v3] x86, UV: integer wrap bug in uv_hub_ipi_value()

[Dan Carpenter]

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

Re: [patch v3] x86, UV: integer wrap bug in uv_hub_ipi_value()

[Dan Carpenter]

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

[patch v4] x86, UV: integer wrap bug in uv_hub_ipi_value()

[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.

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

[patch v4] x86, UV: integer wrap bug in uv_hub_ipi_value()

[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.

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