[PATCH] MIPS: change type of asid_cache to unsigned long

[PATCH] MIPS: change type of asid_cache to unsigned long

[Yong Zhang]

From: Yong Zhang <yong.zhang@windriver.com>

asid_cache must be unsigned long otherwise on 64bit system
it will become 0 if the value in get_new_mmu_context()
reaches 0xffffffff and in the end the assumption of
ASID_FIRST_VERSION is not true anymore thus leads to
more dangerous things.

Signed-off-by: Yong Zhang <yong.zhang@windriver.com>
---
 arch/mips/include/asm/cpu-info.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/include/asm/cpu-info.h b/arch/mips/include/asm/cpu-info.h
index f6299be..ebcc2ed 100644
--- a/arch/mips/include/asm/cpu-info.h
+++ b/arch/mips/include/asm/cpu-info.h
@@ -40,7 +40,7 @@ struct cache_desc {
 
 struct cpuinfo_mips {
 	unsigned int		udelay_val;
-	unsigned int		asid_cache;
+	unsigned long		asid_cache;
 
 	/*
 	 * Capability and feature descriptor structure for MIPS CPU
-- 
1.7.9.5

Re: [PATCH] MIPS: change type of asid_cache to unsigned long

[Yong Zhang]

Please check the V2 in which I add the reporter.
And thanks libin for reporting it :)

Thanks,
Yong

On Tue, May 20, 2014 at 04:09:04PM +0800, Yong Zhang wrote:
> From: Yong Zhang <yong.zhang@windriver.com>
> 
> asid_cache must be unsigned long otherwise on 64bit system
> it will become 0 if the value in get_new_mmu_context()
> reaches 0xffffffff and in the end the assumption of
> ASID_FIRST_VERSION is not true anymore thus leads to
> more dangerous things.
> 
> Signed-off-by: Yong Zhang <yong.zhang@windriver.com>
> ---
>  arch/mips/include/asm/cpu-info.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/mips/include/asm/cpu-info.h b/arch/mips/include/asm/cpu-info.h
> index f6299be..ebcc2ed 100644
> --- a/arch/mips/include/asm/cpu-info.h
> +++ b/arch/mips/include/asm/cpu-info.h
> @@ -40,7 +40,7 @@ struct cache_desc {
>  
>  struct cpuinfo_mips {
>  	unsigned int		udelay_val;
> -	unsigned int		asid_cache;
> +	unsigned long		asid_cache;
>  
>  	/*
>  	 * Capability and feature descriptor structure for MIPS CPU
> -- 
> 1.7.9.5

Re: [PATCH] MIPS: change type of asid_cache to unsigned long

[Yong Zhang]

Please check the V2 in which I add the reporter.
And thanks libin for reporting it :)

Thanks,
Yong

On Tue, May 20, 2014 at 04:09:04PM +0800, Yong Zhang wrote:
> From: Yong Zhang <yong.zhang@windriver.com>
> 
> asid_cache must be unsigned long otherwise on 64bit system
> it will become 0 if the value in get_new_mmu_context()
> reaches 0xffffffff and in the end the assumption of
> ASID_FIRST_VERSION is not true anymore thus leads to
> more dangerous things.
> 
> Signed-off-by: Yong Zhang <yong.zhang@windriver.com>
> ---
>  arch/mips/include/asm/cpu-info.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/mips/include/asm/cpu-info.h b/arch/mips/include/asm/cpu-info.h
> index f6299be..ebcc2ed 100644
> --- a/arch/mips/include/asm/cpu-info.h
> +++ b/arch/mips/include/asm/cpu-info.h
> @@ -40,7 +40,7 @@ struct cache_desc {
>  
>  struct cpuinfo_mips {
>  	unsigned int		udelay_val;
> -	unsigned int		asid_cache;
> +	unsigned long		asid_cache;
>  
>  	/*
>  	 * Capability and feature descriptor structure for MIPS CPU
> -- 
> 1.7.9.5

Re: [PATCH] MIPS: change type of asid_cache to unsigned long

[Ralf Baechle]

On Wed, May 21, 2014 at 01:38:53PM +0800, Yong Zhang wrote:

> Please check the V2 in which I add the reporter.
> And thanks libin for reporting it :)

The bug was introduced in 5636919b5c909fee54a6ef5226475ecae012ad02
[MIPS: Outline udelay and fix a few issues.] in 2009 btw.  I think
the intension was to avoid holes in the structure and minimize
the bloat.  I instead applied aptch which also moves another member
of the struct arond such that no hole will be created in the struct.
This is important because the strcture it accessed fairly frequently
so we want to fit the most important members into as few cache
lines as possible.

Thanks,

  Ralf

Re: [PATCH] MIPS: change type of asid_cache to unsigned long

[Yong Zhang]

On Wed, May 21, 2014 at 01:29:36PM +0200, Ralf Baechle wrote:
> On Wed, May 21, 2014 at 01:38:53PM +0800, Yong Zhang wrote:
> 
> > Please check the V2 in which I add the reporter.
> > And thanks libin for reporting it :)
> 
> The bug was introduced in 5636919b5c909fee54a6ef5226475ecae012ad02
> [MIPS: Outline udelay and fix a few issues.] in 2009 btw.  I think
> the intension was to avoid holes in the structure and minimize
> the bloat.  I instead applied aptch

Could you please show the patch?

> which also moves another member
> of the struct arond such that no hole will be created in the struct.
> This is important because the strcture it accessed fairly frequently
> so we want to fit the most important members into as few cache
> lines as possible.

I have tried to move the struct member around, but I found that the
hole cann't be avoided completely because for exampe struct cache_desc
is a bit special.

Thanks,
Yong


> 
> Thanks,
> 
>   Ralf

Re: [PATCH] MIPS: change type of asid_cache to unsigned long

[Ralf Baechle]

On Thu, May 22, 2014 at 10:06:11AM +0800, Yong Zhang wrote:

> On Wed, May 21, 2014 at 01:29:36PM +0200, Ralf Baechle wrote:
> > On Wed, May 21, 2014 at 01:38:53PM +0800, Yong Zhang wrote:
> > 
> > > Please check the V2 in which I add the reporter.
> > > And thanks libin for reporting it :)
> > 
> > The bug was introduced in 5636919b5c909fee54a6ef5226475ecae012ad02
> > [MIPS: Outline udelay and fix a few issues.] in 2009 btw.  I think
> > the intension was to avoid holes in the structure and minimize
> > the bloat.  I instead applied aptch
> 
> Could you please show the patch?
> 
> > which also moves another member
> > of the struct arond such that no hole will be created in the struct.
> > This is important because the strcture it accessed fairly frequently
> > so we want to fit the most important members into as few cache
> > lines as possible.
> 
> I have tried to move the struct member around, but I found that the
> hole cann't be avoided completely because for exampe struct cache_desc
> is a bit special.

Yes, struct cache_desc is still a problem.  Easily solvable though -
some of it's members are excessivly large; by using smaller data types
both the struct and its required alignment will shrink.  But that's
for another patch; as for this patch my goal to just not make things
any worse.

  Ralf

---
 arch/mips/include/asm/cpu-info.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/cpu-info.h b/arch/mips/include/asm/cpu-info.h
index dc2135b..ff2707a 100644
--- a/arch/mips/include/asm/cpu-info.h
+++ b/arch/mips/include/asm/cpu-info.h
@@ -39,14 +39,14 @@ struct cache_desc {
 #define MIPS_CACHE_PINDEX	0x00000020	/* Physically indexed cache */
 
 struct cpuinfo_mips {
-	unsigned int		udelay_val;
-	unsigned int		asid_cache;
+	unsigned long		asid_cache;
 
 	/*
 	 * Capability and feature descriptor structure for MIPS CPU
 	 */
 	unsigned long		options;
 	unsigned long		ases;
+	unsigned int		udelay_val;
 	unsigned int		processor_id;
 	unsigned int		fpu_id;
 	unsigned int		msa_id;

Re: [PATCH] MIPS: change type of asid_cache to unsigned long

[Yong Zhang]

On Thu, May 22, 2014 at 03:42:45PM +0200, Ralf Baechle wrote:
> Yes, struct cache_desc is still a problem.  Easily solvable though -
> some of it's members are excessivly large; by using smaller data types
> both the struct and its required alignment will shrink.  But that's
> for another patch; as for this patch my goal to just not make things
> any worse.

Agree. Anyway I have done the similiar change as your patch already :)

Thanks,
Yong

> 
>   Ralf
> 
> ---
>  arch/mips/include/asm/cpu-info.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/mips/include/asm/cpu-info.h b/arch/mips/include/asm/cpu-info.h
> index dc2135b..ff2707a 100644
> --- a/arch/mips/include/asm/cpu-info.h
> +++ b/arch/mips/include/asm/cpu-info.h
> @@ -39,14 +39,14 @@ struct cache_desc {
>  #define MIPS_CACHE_PINDEX	0x00000020	/* Physically indexed cache */
>  
>  struct cpuinfo_mips {
> -	unsigned int		udelay_val;
> -	unsigned int		asid_cache;
> +	unsigned long		asid_cache;
>  
>  	/*
>  	 * Capability and feature descriptor structure for MIPS CPU
>  	 */
>  	unsigned long		options;
>  	unsigned long		ases;
> +	unsigned int		udelay_val;
>  	unsigned int		processor_id;
>  	unsigned int		fpu_id;
>  	unsigned int		msa_id;

Re: [PATCH] MIPS: change type of asid_cache to unsigned long

[David Daney]

Regarding this patch (commit e5eb925a1804c4a52994ba57f4f68ee7a9132905), 
the fix is fine for 64-bit systems, as it is impossible to overflow a 
64-bit ASID value.

For 32-bit systems, there is still a problem, we don't see the type 
truncation issue that was present on 64-bit systems, but there can still 
be badness on ASID generation wrap.


Scenario:

  o Long live process (p0) that sleeps for a long time.  It acquires 
what we will call ASID_0 and then is scheduled off the CPU

  o We cycle through 2^32 ASIDs, and the asid_cache wraps around  (not 
difficult to do, just write a program that does nothing but mmap() 
munmap() in a loop).  We have seen this happen every 6 days with ebizzy 
benchmark program.

  o Start new program (p1) that happens to also get ASID_0

  o p0 wakes up, and is now sharing tlb entries with p1, chaos ensues.

A workaround for this would be to use u64 for both 32-bit and 64-bit for 
all ASID related variables.  I have a patch for this, is it worth 
testing on 32-bit systems, and sending it in?

David Daney


On 05/22/2014 06:42 AM, Ralf Baechle wrote:
> On Thu, May 22, 2014 at 10:06:11AM +0800, Yong Zhang wrote:
>
>> On Wed, May 21, 2014 at 01:29:36PM +0200, Ralf Baechle wrote:
>>> On Wed, May 21, 2014 at 01:38:53PM +0800, Yong Zhang wrote:
>>>
>>>> Please check the V2 in which I add the reporter.
>>>> And thanks libin for reporting it :)
>>>
>>> The bug was introduced in 5636919b5c909fee54a6ef5226475ecae012ad02
>>> [MIPS: Outline udelay and fix a few issues.] in 2009 btw.  I think
>>> the intension was to avoid holes in the structure and minimize
>>> the bloat.  I instead applied aptch
>>
>> Could you please show the patch?
>>
>>> which also moves another member
>>> of the struct arond such that no hole will be created in the struct.
>>> This is important because the strcture it accessed fairly frequently
>>> so we want to fit the most important members into as few cache
>>> lines as possible.
>>
>> I have tried to move the struct member around, but I found that the
>> hole cann't be avoided completely because for exampe struct cache_desc
>> is a bit special.
>
> Yes, struct cache_desc is still a problem.  Easily solvable though -
> some of it's members are excessivly large; by using smaller data types
> both the struct and its required alignment will shrink.  But that's
> for another patch; as for this patch my goal to just not make things
> any worse.
>
>    Ralf
>
> ---
>   arch/mips/include/asm/cpu-info.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/include/asm/cpu-info.h b/arch/mips/include/asm/cpu-info.h
> index dc2135b..ff2707a 100644
> --- a/arch/mips/include/asm/cpu-info.h
> +++ b/arch/mips/include/asm/cpu-info.h
> @@ -39,14 +39,14 @@ struct cache_desc {
>   #define MIPS_CACHE_PINDEX	0x00000020	/* Physically indexed cache */
>
>   struct cpuinfo_mips {
> -	unsigned int		udelay_val;
> -	unsigned int		asid_cache;
> +	unsigned long		asid_cache;
>
>   	/*
>   	 * Capability and feature descriptor structure for MIPS CPU
>   	 */
>   	unsigned long		options;
>   	unsigned long		ases;
> +	unsigned int		udelay_val;
>   	unsigned int		processor_id;
>   	unsigned int		fpu_id;
>   	unsigned int		msa_id;
>
>