Re: [PATCH v2 6/6] xen/riscv: fix level_map_mask truncation on load_start

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 9/10/26 11:34 AM, Baptiste Le Duc wrote:
> check_pgtbl_mode_support() declares level_map_mask as bare `unsigned`, i.e.
> a 32-bit type while it derives from a paddr_t which is in both RV32/RV64 a
> 64-bit type. Storing that value into a 32-bit local silently drops any set
> bits above bit 31.
> 
> The mask is then used as:
> 
>      aligned_load_start = load_start & level_map_mask;
> 
> load_start is `unsigned long` (64-bit on riscv64) and if it requires more
> than 32 bits to represent, because load_start zero-extend to 64 bits, we
> would drop some load_start's bits during the AND.
> 
> Widen level_map_mask to `unsigned long`, matching the width of the physical
> address.
> 
> Fixes: e66003e7be19 ("xen/riscv: introduce setup_initial_pages")
> Reported-by: Zheng Zhang <zhangzheng@iscas.ac.cn>
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> ---
> Changes since v1:
> - new patch
> ---
> Question:
> I would think replacing unsigned long by paddr_t would be better in this
> case but for consistency with other variables in the function I just kept
> unsigned long.
> 
> However, there are many variables in mm.c which are unsigned long while
> they are, in reality, physical addresses and could technically be paddr_t.
> Using paddr_t would also let us bypass the compiler's decision on what
> unsigned long extends to (u32 or u64, depending on the target), and
> therefore be more generic. I've seen similar code in Arm using this
> convention, and found nothing on the mailing list explaining the original
> choice of unsigned long over paddr_t.
> 
> Replacing every such field would be a fairly large change, so I'm asking
> for your opinion on whether it's worth doing.
> ---

I think if to do that it will better to do step by step where real use 
cases which lead to some problem will happen.

Specifically here it looks like `unsigned long` should be enough ...

>   xen/arch/riscv/mm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> index 53bebbcabf..e7f2491257 100644
> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -180,7 +180,7 @@ static bool __init check_pgtbl_mode_support(struct mmu_desc *mmu_desc,
>       bool is_mode_supported = false;
>       unsigned int index;
>       unsigned int page_table_level = (mmu_desc->num_levels - 1);
> -    unsigned level_map_mask = XEN_PT_LEVEL_MAP_MASK(page_table_level);
> +    unsigned long level_map_mask = XEN_PT_LEVEL_MAP_MASK(page_table_level);
>   
>       unsigned long aligned_load_start = load_start & level_map_mask;
>       unsigned long aligned_page_size = XEN_PT_LEVEL_SIZE(page_table_level);
>
... what you actually did.

LGTM: Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks for the fix!

~ Oleksii