Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a7bb09c9-870a-d8f2-23c3-7f6fd2c68e2b@ghiti.fr>
Date:   Tue, 11 Jul 2023 13:14:10 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH] riscv: correct pt_level name via pgtable_l5/4_enabled
Content-Language: en-US
To:     Song Shuai <suagrfillet@gmail.com>, paul.walmsley@sifive.com,
        palmer@dabbelt.com, aou@eecs.berkeley.edu, alexghiti@rivosinc.com,
        bjorn@rivosinc.com
Cc:     linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20230711105202.842408-1-suagrfillet@gmail.com>
From:   Alexandre Ghiti <alex@ghiti.fr>
In-Reply-To: <20230711105202.842408-1-suagrfillet@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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