[PATCH v2] powerpc/book3s/mm: Update Oops message to print the correct translation in use

[PATCH v2] powerpc/book3s/mm: Update Oops message to print the correct translation in use

[Aneesh Kumar K.V]

Avoids confusion when printing Oops message like below

 Faulting instruction address: 0xc00000000008bdb4
 Oops: Kernel access of bad area, sig: 11 [#1]
 LE PAGE_SIZE=64K MMU=Radix MMU=Hash SMP NR_CPUS=2048 NUMA PowerNV

This was because we never clear the MMU_FTR_HPTE_TABLE feature flag
even if we run with radix translation. It was discussed that we should
look at this feature flag as an indication of the capability to run
hash translation and we should not clear the flag even if we run in
radix translation. All the code paths check for radix_enabled() check and
if found true consider we are running with radix translation. Follow the
same sequence for finding the MMU translation string to be used in Oops
message.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---

Changes from V1:
* Don't clear the HPTE feature flag.

 arch/powerpc/kernel/traps.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 11caa0291254..b181d6860f28 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -250,15 +250,22 @@ static void oops_end(unsigned long flags, struct pt_regs *regs,
 }
 NOKPROBE_SYMBOL(oops_end);
 
+static char *get_mmu_str(void)
+{
+	if (early_radix_enabled())
+		return " MMU=Radix";
+	if (early_mmu_has_feature(MMU_FTR_HPTE_TABLE))
+		return " MMU=Hash";
+	return "";
+}
+
 static int __die(const char *str, struct pt_regs *regs, long err)
 {
 	printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
 
-	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s%s %s\n",
+	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
 	       IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
-	       PAGE_SIZE / 1024,
-	       early_radix_enabled() ? " MMU=Radix" : "",
-	       early_mmu_has_feature(MMU_FTR_HPTE_TABLE) ? " MMU=Hash" : "",
+	       PAGE_SIZE / 1024, get_mmu_str(),
 	       IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
 	       IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
 	       IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
-- 
2.21.0

Re: [PATCH v2] powerpc/book3s/mm: Update Oops message to print the correct translation in use

[Nicholas Piggin]

Aneesh Kumar K.V's on July 12, 2019 12:58 am:
> Avoids confusion when printing Oops message like below
> 
>  Faulting instruction address: 0xc00000000008bdb4
>  Oops: Kernel access of bad area, sig: 11 [#1]
>  LE PAGE_SIZE=64K MMU=Radix MMU=Hash SMP NR_CPUS=2048 NUMA PowerNV
> 
> This was because we never clear the MMU_FTR_HPTE_TABLE feature flag
> even if we run with radix translation. It was discussed that we should
> look at this feature flag as an indication of the capability to run
> hash translation and we should not clear the flag even if we run in
> radix translation. All the code paths check for radix_enabled() check and
> if found true consider we are running with radix translation. Follow the
> same sequence for finding the MMU translation string to be used in Oops
> message.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> 
> Changes from V1:
> * Don't clear the HPTE feature flag.

Thanks,

Acked-by: Nicholas Piggin <npiggin@gmail.com>

> 
>  arch/powerpc/kernel/traps.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 11caa0291254..b181d6860f28 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -250,15 +250,22 @@ static void oops_end(unsigned long flags, struct pt_regs *regs,
>  }
>  NOKPROBE_SYMBOL(oops_end);
>  
> +static char *get_mmu_str(void)
> +{
> +	if (early_radix_enabled())
> +		return " MMU=Radix";
> +	if (early_mmu_has_feature(MMU_FTR_HPTE_TABLE))
> +		return " MMU=Hash";
> +	return "";
> +}
> +
>  static int __die(const char *str, struct pt_regs *regs, long err)
>  {
>  	printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
>  
> -	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s%s %s\n",
> +	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
>  	       IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
> -	       PAGE_SIZE / 1024,
> -	       early_radix_enabled() ? " MMU=Radix" : "",
> -	       early_mmu_has_feature(MMU_FTR_HPTE_TABLE) ? " MMU=Hash" : "",
> +	       PAGE_SIZE / 1024, get_mmu_str(),
>  	       IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
>  	       IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
>  	       IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
> -- 
> 2.21.0
> 
> 

Re: [PATCH v2] powerpc/book3s/mm: Update Oops message to print the correct translation in use

[Christophe Leroy]

Le 11/07/2019 à 16:58, Aneesh Kumar K.V a écrit :
> Avoids confusion when printing Oops message like below
> 
>   Faulting instruction address: 0xc00000000008bdb4
>   Oops: Kernel access of bad area, sig: 11 [#1]
>   LE PAGE_SIZE=64K MMU=Radix MMU=Hash SMP NR_CPUS=2048 NUMA PowerNV
> 
> This was because we never clear the MMU_FTR_HPTE_TABLE feature flag
> even if we run with radix translation. It was discussed that we should
> look at this feature flag as an indication of the capability to run
> hash translation and we should not clear the flag even if we run in
> radix translation. All the code paths check for radix_enabled() check and
> if found true consider we are running with radix translation. Follow the
> same sequence for finding the MMU translation string to be used in Oops
> message.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>

Looks good.

Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>

> ---
> 
> Changes from V1:
> * Don't clear the HPTE feature flag.
> 
>   arch/powerpc/kernel/traps.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 11caa0291254..b181d6860f28 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -250,15 +250,22 @@ static void oops_end(unsigned long flags, struct pt_regs *regs,
>   }
>   NOKPROBE_SYMBOL(oops_end);
>   
> +static char *get_mmu_str(void)
> +{
> +	if (early_radix_enabled())
> +		return " MMU=Radix";
> +	if (early_mmu_has_feature(MMU_FTR_HPTE_TABLE))
> +		return " MMU=Hash";
> +	return "";
> +}
> +
>   static int __die(const char *str, struct pt_regs *regs, long err)
>   {
>   	printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
>   
> -	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s%s %s\n",
> +	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
>   	       IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
> -	       PAGE_SIZE / 1024,
> -	       early_radix_enabled() ? " MMU=Radix" : "",
> -	       early_mmu_has_feature(MMU_FTR_HPTE_TABLE) ? " MMU=Hash" : "",
> +	       PAGE_SIZE / 1024, get_mmu_str(),
>   	       IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
>   	       IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
>   	       IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
> 

Re: [PATCH v2] powerpc/book3s/mm: Update Oops message to print the correct translation in use

[Michael Ellerman]

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:

> Avoids confusion when printing Oops message like below
>
>  Faulting instruction address: 0xc00000000008bdb4
>  Oops: Kernel access of bad area, sig: 11 [#1]
>  LE PAGE_SIZE=64K MMU=Radix MMU=Hash SMP NR_CPUS=2048 NUMA PowerNV
>
> This was because we never clear the MMU_FTR_HPTE_TABLE feature flag
> even if we run with radix translation. It was discussed that we should
> look at this feature flag as an indication of the capability to run
> hash translation and we should not clear the flag even if we run in
> radix translation. All the code paths check for radix_enabled() check and
> if found true consider we are running with radix translation. Follow the
> same sequence for finding the MMU translation string to be used in Oops
> message.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>
> Changes from V1:
> * Don't clear the HPTE feature flag.
>
>  arch/powerpc/kernel/traps.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 11caa0291254..b181d6860f28 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -250,15 +250,22 @@ static void oops_end(unsigned long flags, struct pt_regs *regs,
>  }
>  NOKPROBE_SYMBOL(oops_end);
>  
> +static char *get_mmu_str(void)
> +{
> +	if (early_radix_enabled())
> +		return " MMU=Radix";
> +	if (early_mmu_has_feature(MMU_FTR_HPTE_TABLE))
> +		return " MMU=Hash";
> +	return "";
> +}

We don't change MMU once we're up, so just do this logic once and stash
it into a static string, rather than rechecking on every oops.

cheers

>  static int __die(const char *str, struct pt_regs *regs, long err)
>  {
>  	printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
>  
> -	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s%s %s\n",
> +	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
>  	       IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
> -	       PAGE_SIZE / 1024,
> -	       early_radix_enabled() ? " MMU=Radix" : "",
> -	       early_mmu_has_feature(MMU_FTR_HPTE_TABLE) ? " MMU=Hash" : "",
> +	       PAGE_SIZE / 1024, get_mmu_str(),
>  	       IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
>  	       IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
>  	       IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
> -- 
> 2.21.0

Re: [PATCH v2] powerpc/book3s/mm: Update Oops message to print the correct translation in use

[Christophe Leroy]

Le 12/07/2019 à 08:25, Michael Ellerman a écrit :
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> 
>> Avoids confusion when printing Oops message like below
>>
>>   Faulting instruction address: 0xc00000000008bdb4
>>   Oops: Kernel access of bad area, sig: 11 [#1]
>>   LE PAGE_SIZE=64K MMU=Radix MMU=Hash SMP NR_CPUS=2048 NUMA PowerNV
>>
>> This was because we never clear the MMU_FTR_HPTE_TABLE feature flag
>> even if we run with radix translation. It was discussed that we should
>> look at this feature flag as an indication of the capability to run
>> hash translation and we should not clear the flag even if we run in
>> radix translation. All the code paths check for radix_enabled() check and
>> if found true consider we are running with radix translation. Follow the
>> same sequence for finding the MMU translation string to be used in Oops
>> message.
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>
>> Changes from V1:
>> * Don't clear the HPTE feature flag.
>>
>>   arch/powerpc/kernel/traps.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
>> index 11caa0291254..b181d6860f28 100644
>> --- a/arch/powerpc/kernel/traps.c
>> +++ b/arch/powerpc/kernel/traps.c
>> @@ -250,15 +250,22 @@ static void oops_end(unsigned long flags, struct pt_regs *regs,
>>   }
>>   NOKPROBE_SYMBOL(oops_end);
>>   
>> +static char *get_mmu_str(void)
>> +{
>> +	if (early_radix_enabled())
>> +		return " MMU=Radix";
>> +	if (early_mmu_has_feature(MMU_FTR_HPTE_TABLE))
>> +		return " MMU=Hash";
>> +	return "";
>> +}
> 
> We don't change MMU once we're up, so just do this logic once and stash
> it into a static string, rather than rechecking on every oops.

Do we really have oops so often that we have to worry about that ?

Christophe

> 
> cheers
> 
>>   static int __die(const char *str, struct pt_regs *regs, long err)
>>   {
>>   	printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
>>   
>> -	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s%s %s\n",
>> +	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
>>   	       IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
>> -	       PAGE_SIZE / 1024,
>> -	       early_radix_enabled() ? " MMU=Radix" : "",
>> -	       early_mmu_has_feature(MMU_FTR_HPTE_TABLE) ? " MMU=Hash" : "",
>> +	       PAGE_SIZE / 1024, get_mmu_str(),
>>   	       IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
>>   	       IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
>>   	       IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
>> -- 
>> 2.21.0

Re: [PATCH v2] powerpc/book3s/mm: Update Oops message to print the correct translation in use

[Michael Ellerman]

Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Le 12/07/2019 à 08:25, Michael Ellerman a écrit :
>> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
...
>>> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
>>> index 11caa0291254..b181d6860f28 100644
>>> --- a/arch/powerpc/kernel/traps.c
>>> +++ b/arch/powerpc/kernel/traps.c
>>> @@ -250,15 +250,22 @@ static void oops_end(unsigned long flags, struct pt_regs *regs,
>>>   }
>>>   NOKPROBE_SYMBOL(oops_end);
>>>   
>>> +static char *get_mmu_str(void)
>>> +{
>>> +	if (early_radix_enabled())
>>> +		return " MMU=Radix";
>>> +	if (early_mmu_has_feature(MMU_FTR_HPTE_TABLE))
>>> +		return " MMU=Hash";
>>> +	return "";
>>> +}
>> 
>> We don't change MMU once we're up, so just do this logic once and stash
>> it into a static string, rather than rechecking on every oops.
>
> Do we really have oops so often that we have to worry about that ?

Sometimes :)

But no I don't mean it's a performance issue, it just seems simpler to
compute the value once and store it. In fact for most platforms it can
just be a static string at compile time, it's only 64-bit Book3S that
needs to do anything at runtime.

cheers

Re: [PATCH v2] powerpc/book3s/mm: Update Oops message to print the correct translation in use

[Christophe Leroy]

Le 12/07/2019 à 14:22, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> Le 12/07/2019 à 08:25, Michael Ellerman a écrit :
>>> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> ...
>>>> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
>>>> index 11caa0291254..b181d6860f28 100644
>>>> --- a/arch/powerpc/kernel/traps.c
>>>> +++ b/arch/powerpc/kernel/traps.c
>>>> @@ -250,15 +250,22 @@ static void oops_end(unsigned long flags, struct pt_regs *regs,
>>>>    }
>>>>    NOKPROBE_SYMBOL(oops_end);
>>>>    
>>>> +static char *get_mmu_str(void)
>>>> +{
>>>> +	if (early_radix_enabled())
>>>> +		return " MMU=Radix";
>>>> +	if (early_mmu_has_feature(MMU_FTR_HPTE_TABLE))
>>>> +		return " MMU=Hash";
>>>> +	return "";
>>>> +}
>>>
>>> We don't change MMU once we're up, so just do this logic once and stash
>>> it into a static string, rather than rechecking on every oops.
>>
>> Do we really have oops so often that we have to worry about that ?
> 
> Sometimes :)
> 
> But no I don't mean it's a performance issue, it just seems simpler to
> compute the value once and store it. In fact for most platforms it can
> just be a static string at compile time, it's only 64-bit Book3S that
> needs to do anything at runtime.

Right, but I'm sure GCC will take care of that since the function is 
static and called only once.

Christophe
> 
> cheers
> 

Re: [PATCH v2] powerpc/book3s/mm: Update Oops message to print the correct translation in use

[Michael Ellerman]

On Thu, 2019-07-11 at 14:58:14 UTC, "Aneesh Kumar K.V" wrote:
> Avoids confusion when printing Oops message like below
> 
>  Faulting instruction address: 0xc00000000008bdb4
>  Oops: Kernel access of bad area, sig: 11 [#1]
>  LE PAGE_SIZE=64K MMU=Radix MMU=Hash SMP NR_CPUS=2048 NUMA PowerNV
> 
> This was because we never clear the MMU_FTR_HPTE_TABLE feature flag
> even if we run with radix translation. It was discussed that we should
> look at this feature flag as an indication of the capability to run
> hash translation and we should not clear the flag even if we run in
> radix translation. All the code paths check for radix_enabled() check and
> if found true consider we are running with radix translation. Follow the
> same sequence for finding the MMU translation string to be used in Oops
> message.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/d7e02f7b7991dbe14a2acfb0e53d675cd149001c

cheers