Re: [XEN v3 8/9] xen/arm: Restrict zeroeth_table_offset for ARM_64

[Julien Grall <julien@xxxxxxx>]

Hi,

On 08/02/2023 12:05, Ayan Kumar Halder wrote:
> When 32 bit physical addresses are used (ie ARM_PA_32=y),
> "va >> ZEROETH_SHIFT" causes an overflow.
> Also, there is no zeroeth level page table on Arm 32-bit.
>
> Also took the opportunity to clean up dump_pt_walk(). One could use
> DECLARE_OFFSETS() macro instead of declaring the declaring an array
> of page table offsets.
Technically this is unrelated to the goal of the patch. So this should 
have been split in a separate patch.
No need to split it this time, but in the future please consider to 
split a patch when a change is unrelated.
In general, I would only consider to bundle clean-up if all the below 
applies:
  1) the patch is not doing code movement
  2) the change is simple (e.g. typo, coding style fix)
  3) the change is in the vicinity of the the rest of code (i.e. it 
would be visible in the diff).
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

> ---
> Changes from -
>
> v1 - Removed the duplicate declaration for DECLARE_OFFSETS.
>
> v2 - 1. Reworded the commit message.
> 2. Use CONFIG_ARM_PA_32 to restrict zeroeth_table_offset.
>
>   xen/arch/arm/include/asm/lpae.h | 4 ++++
>   xen/arch/arm/mm.c               | 7 +------
>   2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/lpae.h b/xen/arch/arm/include/asm/lpae.h
> index 3fdd5d0de2..998edeed6e 100644
> --- a/xen/arch/arm/include/asm/lpae.h
> +++ b/xen/arch/arm/include/asm/lpae.h
> @@ -259,7 +259,11 @@ lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned int attr);
>   #define first_table_offset(va)  TABLE_OFFSET(first_linear_offset(va))
>   #define second_table_offset(va) TABLE_OFFSET(second_linear_offset(va))
>   #define third_table_offset(va)  TABLE_OFFSET(third_linear_offset(va))
> +#ifdef CONFIG_ARM_PA_32
> +#define zeroeth_table_offset(va)  0
> +#else
>   #define zeroeth_table_offset(va)  TABLE_OFFSET(zeroeth_linear_offset(va))
> +#endif
>   
>   /*
>    * Macros to define page-tables:
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index b99806af99..44942c6a4f 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -221,12 +221,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
>   {
>       static const char *level_strs[4] = { "0TH", "1ST", "2ND", "3RD" };
>       const mfn_t root_mfn = maddr_to_mfn(ttbr);
> -    const unsigned int offsets[4] = {
> -        zeroeth_table_offset(addr),
> -        first_table_offset(addr),
> -        second_table_offset(addr),
> -        third_table_offset(addr)
> -    };
> +    DECLARE_OFFSETS(offsets, addr);
>       lpae_t pte, *mapping;
>       unsigned int level, root_table;
>   
Cheers,

--
Julien Grall