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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.09.2026 11:34, 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@xxxxxxxxxxx>
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>

The change is okay as is, so
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
But see below.

> ---
> 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.

The mask here is applied to an incoming linear address, so imo unsigned
long is the correct type.

> --- 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;

level_map_mask is used exclusively here. Without that intermediate variable
no problem would have existed in the first place. Hence perhaps worth
considering

    unsigned long aligned_load_start =
        load_start & XEN_PT_LEVEL_MAP_MASK(page_table_level);

as an alternative?

Jan