diff mbox series

riscv: correct pt_level name via pgtable_l5/4_enabled

Message ID 20230711105202.842408-1-suagrfillet@gmail.com
State New
Headers
Series riscv: correct pt_level name via pgtable_l5/4_enabled |

Commit Message

Song Shuai July 11, 2023, 10:52 a.m. UTC 
  The pt_level uses CONFIG_PGTABLE_LEVELS to display page table names.
But if downgrading page mode from kernel cmdline in 64BIT, it will
give a wrong name.

Like, using no4lvl for sv39, ptdump named the 1G-mapping as "PUD"
that should be "PGD":

0xffffffd840000000-0xffffffd900000000    0x00000000c0000000         3G PUD     D A G . . W R V

So select "P4D/PUD" or "PGD" via pgtable_l5/4_enabled to correct it.

Fixes: 26e7aacb83df ("riscv: Allow to downgrade paging mode from the command line")
Signed-off-by: Song Shuai <suagrfillet@gmail.com>
---
 arch/riscv/mm/ptdump.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Alexandre Ghiti July 11, 2023, 11:14 a.m. UTC | #1 
Hi Song,


On 11/07/2023 12:52, Song Shuai wrote:
> The pt_level uses CONFIG_PGTABLE_LEVELS to display page table names.
> But if downgrading page mode from kernel cmdline in 64BIT, it will
> give a wrong name.
>
> Like, using no4lvl for sv39, ptdump named the 1G-mapping as "PUD"
> that should be "PGD":
>
> 0xffffffd840000000-0xffffffd900000000    0x00000000c0000000         3G PUD     D A G . . W R V
>
> So select "P4D/PUD" or "PGD" via pgtable_l5/4_enabled to correct it.
>
> Fixes: 26e7aacb83df ("riscv: Allow to downgrade paging mode from the command line")
> Signed-off-by: Song Shuai <suagrfillet@gmail.com>
> ---
>   arch/riscv/mm/ptdump.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
> index 20a9f991a6d7..cfdd327981ee 100644
> --- a/arch/riscv/mm/ptdump.c
> +++ b/arch/riscv/mm/ptdump.c
> @@ -384,6 +384,11 @@ static int __init ptdump_init(void)
>   
>   	kernel_ptd_info.base_addr = KERN_VIRT_START;
>   
> +#ifdef CONFIG_64BIT
> +	pg_level[1].name = pgtable_l5_enabled ? "P4D" : "PGD";
> +	pg_level[2].name = pgtable_l4_enabled ? "PUD" : "PGD";
> +#endif


You don't need the #ifdef here as pgtable_lX_enabled are always declared.


> +
>   	for (i = 0; i < ARRAY_SIZE(pg_level); i++)
>   		for (j = 0; j < ARRAY_SIZE(pte_bits); j++)
>   			pg_level[i].mask |= pte_bits[j].mask;


The Fixes tag is wrong to me, if satp mode is restricted by the hardware 
itself (not from the command line), the same problem happens. Maybe that 
should be the sv48 introduction patch? Or the sv57? It will be more 
cumbersome to backport with the sv48 introduction though as this patch 
won't apply as-is.

Otherwise, you can add:

Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>

Thanks,

Alex
Palmer Dabbelt July 12, 2023, 1:58 p.m. UTC | #2 
On Tue, 11 Jul 2023 04:14:10 PDT (-0700), alex@ghiti.fr wrote:
> Hi Song,
>
>
> On 11/07/2023 12:52, Song Shuai wrote:
>> The pt_level uses CONFIG_PGTABLE_LEVELS to display page table names.
>> But if downgrading page mode from kernel cmdline in 64BIT, it will
>> give a wrong name.
>>
>> Like, using no4lvl for sv39, ptdump named the 1G-mapping as "PUD"
>> that should be "PGD":
>>
>> 0xffffffd840000000-0xffffffd900000000    0x00000000c0000000         3G PUD     D A G . . W R V
>>
>> So select "P4D/PUD" or "PGD" via pgtable_l5/4_enabled to correct it.
>>
>> Fixes: 26e7aacb83df ("riscv: Allow to downgrade paging mode from the command line")
>> Signed-off-by: Song Shuai <suagrfillet@gmail.com>
>> ---
>>   arch/riscv/mm/ptdump.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
>> index 20a9f991a6d7..cfdd327981ee 100644
>> --- a/arch/riscv/mm/ptdump.c
>> +++ b/arch/riscv/mm/ptdump.c
>> @@ -384,6 +384,11 @@ static int __init ptdump_init(void)
>>
>>   	kernel_ptd_info.base_addr = KERN_VIRT_START;
>>
>> +#ifdef CONFIG_64BIT
>> +	pg_level[1].name = pgtable_l5_enabled ? "P4D" : "PGD";
>> +	pg_level[2].name = pgtable_l4_enabled ? "PUD" : "PGD";
>> +#endif
>
>
> You don't need the #ifdef here as pgtable_lX_enabled are always declared.
>
>
>> +
>>   	for (i = 0; i < ARRAY_SIZE(pg_level); i++)
>>   		for (j = 0; j < ARRAY_SIZE(pte_bits); j++)
>>   			pg_level[i].mask |= pte_bits[j].mask;
>
>
> The Fixes tag is wrong to me, if satp mode is restricted by the hardware
> itself (not from the command line), the same problem happens. Maybe that
> should be the sv48 introduction patch? Or the sv57? It will be more
> cumbersome to backport with the sv48 introduction though as this patch
> won't apply as-is.

Sometimes it's easier to base the fix on the original offending commit 
and then forward-port it via a merge, I haven't looked at this one to 
know for sure though.

> Otherwise, you can add:
>
> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>

Thanks.

Song: do you have a v2 that addresses the comments?

>
> Thanks,
>
> Alex
Song Shuai July 13, 2023, 1:24 a.m. UTC | #3 
在 2023/7/12 21:58, Palmer Dabbelt 写道:
> On Tue, 11 Jul 2023 04:14:10 PDT (-0700), alex@ghiti.fr wrote:
>> Hi Song,
>>
>>
>> On 11/07/2023 12:52, Song Shuai wrote:
>>> The pt_level uses CONFIG_PGTABLE_LEVELS to display page table names.
>>> But if downgrading page mode from kernel cmdline in 64BIT, it will
>>> give a wrong name.
>>>
>>> Like, using no4lvl for sv39, ptdump named the 1G-mapping as "PUD"
>>> that should be "PGD":
>>>
>>> 0xffffffd840000000-0xffffffd900000000    0x00000000c0000000         
>>> 3G PUD     D A G . . W R V
>>>
>>> So select "P4D/PUD" or "PGD" via pgtable_l5/4_enabled to correct it.
>>>
>>> Fixes: 26e7aacb83df ("riscv: Allow to downgrade paging mode from the 
>>> command line")
>>> Signed-off-by: Song Shuai <suagrfillet@gmail.com>
>>> ---
>>>   arch/riscv/mm/ptdump.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
>>> index 20a9f991a6d7..cfdd327981ee 100644
>>> --- a/arch/riscv/mm/ptdump.c
>>> +++ b/arch/riscv/mm/ptdump.c
>>> @@ -384,6 +384,11 @@ static int __init ptdump_init(void)
>>>
>>>       kernel_ptd_info.base_addr = KERN_VIRT_START;
>>>
>>> +#ifdef CONFIG_64BIT
>>> +    pg_level[1].name = pgtable_l5_enabled ? "P4D" : "PGD";
>>> +    pg_level[2].name = pgtable_l4_enabled ? "PUD" : "PGD";
>>> +#endif
>>
>>
>> You don't need the #ifdef here as pgtable_lX_enabled are always declared.
>>
>>
>>> +
>>>       for (i = 0; i < ARRAY_SIZE(pg_level); i++)
>>>           for (j = 0; j < ARRAY_SIZE(pte_bits); j++)
>>>               pg_level[i].mask |= pte_bits[j].mask;
>>
>>
>> The Fixes tag is wrong to me, if satp mode is restricted by the hardware
>> itself (not from the command line), the same problem happens. Maybe that
>> should be the sv48 introduction patch? Or the sv57? It will be more
>> cumbersome to backport with the sv48 introduction though as this patch
>> won't apply as-is.
> 
> Sometimes it's easier to base the fix on the original offending commit 
> and then forward-port it via a merge, I haven't looked at this one to 
> know for sure though.
> 
>> Otherwise, you can add:
>>
>> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> 
> Thanks.
> 
> Song: do you have a v2 that addresses the comments?
> 
Here is V2 :

https://lore.kernel.org/linux-riscv/20230712115740.943324-1-suagrfillet@gmail.com/
>>
>> Thanks,
>>
>> Alex
diff mbox series

Patch

diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
index 20a9f991a6d7..cfdd327981ee 100644
--- a/arch/riscv/mm/ptdump.c
+++ b/arch/riscv/mm/ptdump.c
@@ -384,6 +384,11 @@  static int __init ptdump_init(void)
 
 	kernel_ptd_info.base_addr = KERN_VIRT_START;
 
+#ifdef CONFIG_64BIT
+	pg_level[1].name = pgtable_l5_enabled ? "P4D" : "PGD";
+	pg_level[2].name = pgtable_l4_enabled ? "PUD" : "PGD";
+#endif
+
 	for (i = 0; i < ARRAY_SIZE(pg_level); i++)
 		for (j = 0; j < ARRAY_SIZE(pte_bits); j++)
 			pg_level[i].mask |= pte_bits[j].mask;