Re: [PATCH 2/2] xen/arm: traps: drop unreachable stage 2 decoding from panic_PAR()

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 8/25/26 09:23, Michal Orzel wrote:

Hello Michal

> panic_PAR() is only called from va_to_par(), which translates using
> __va_to_par(), i.e. "at s1e2r" on arm64 and ATS1HR on arm32. Both
> perform an EL2 stage 1 only translation, so PAR.S (PAR_STAGE2) and
> PAR.PTW (PAR_STAGE21) can never be set.
>
> This has been the case since commit a14447dbcf171 ("xen: arm: do not
> panic when failing to translate a guest address"), which made
> gva_to_par() and gva_to_ipa() return -EFAULT instead of calling
> panic_PAR() for guest translations.
>
> Print stage 1 unconditionally and keep an ASSERT() to document the
> invariant. PAR_STAGE21 has no user left, so drop it. While at it, fix
> the spacing in the decode_fsc() call.
>
> No functional change intended.
>
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>


> ---
>   xen/arch/arm/include/asm/processor.h |  1 -
>   xen/arch/arm/traps.c                 | 17 +++++++++--------
>   2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/processor.h 
> b/xen/arch/arm/include/asm/processor.h
> index 509040a1cdc0..8ee8f88fb4cb 100644
> --- a/xen/arch/arm/include/asm/processor.h
> +++ b/xen/arch/arm/include/asm/processor.h
> @@ -507,7 +507,6 @@ extern register_t __cpu_logical_map[];
>   /* .... If F == 1 */
>   #define PAR_FSC_SHIFT   (1)
>   #define PAR_FSC_MASK    (_AC(0x3f,U)<<PAR_FSC_SHIFT)
> -#define PAR_STAGE21     (_AC(1,U)<<8)     /* Stage 2 Fault During Stage 1 Walk 
> */
>   #define PAR_STAGE2      (_AC(1,U)<<9)     /* Stage 2 Fault */
>   
>   /* If F == 0 */
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index dc0ec8a345ed..1c2bdb7d02c7 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -382,16 +382,17 @@ void panic_PAR(uint64_t par)
>   {
>       const char *msg;
>       int level = -1;
> -    int stage = par & PAR_STAGE2 ? 2 : 1;
> -    int second_in_first = !!(par & PAR_STAGE21);
>   
> -    msg = decode_fsc( (par&PAR_FSC_MASK) >> PAR_FSC_SHIFT, &level);
> +    /*
> +     * The only caller translates using "at s1e2r" (arm64) or ATS1HR
> +     * (arm32), i.e. an EL2 stage 1 only translation.
> +     */
> +    ASSERT(!(par & PAR_STAGE2));
> +
> +    msg = decode_fsc((par & PAR_FSC_MASK) >> PAR_FSC_SHIFT, &level);
>   
> -    printk("PAR: %016"PRIx64": %s stage %d%s%s\n",
> -           par, msg,
> -           stage,
> -           second_in_first ? " during second stage lookup" : "",
> -           fsc_level_str(level));
> +    printk("PAR: %016"PRIx64": %s stage 1%s\n",
> +           par, msg, fsc_level_str(level));
>   
>       panic("Error during Hypervisor-to-physical address translation\n");
>   }