Re: [PATCH 1/2] xen/arm: traps: report level 0 faults in panic_PAR()

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

Hello Michal


> decode_fsc() derives the fault level from the low two bits of the FSC, so
> level 0 is a valid output: FSC_FLT_TRANS is 0x04, i.e. "translation fault,
> level 0".
> 
> This is reachable on arm64 because xen_pgtable is the zeroeth-level root,
> but fsc_level_str() has no case for it and prints " (level invalid)"
> instead. At the time the function was created Xen used only three levels.
> 
> Add the missing case. On arm32 the zeroeth level does not exist, hence
> guard the case by CONFIG_ARM_64.
> 
> While at it, make decode_fsc() decode also address size faults.
> 
> Signed-off-by: Michal Orzel <[email protected]>


Patch looks ok to me, so:
Reviewed-by: Oleksandr Tyshchenko <[email protected]>

but I have a comment below:

> ---
>   xen/arch/arm/include/asm/processor.h | 2 ++
>   xen/arch/arm/traps.c                 | 8 ++++++++
>   2 files changed, 10 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h
> index a3753c317fff..509040a1cdc0 100644
> --- a/xen/arch/arm/include/asm/processor.h
> +++ b/xen/arch/arm/include/asm/processor.h
> @@ -521,6 +521,7 @@ extern register_t __cpu_logical_map[];
>   /*
>    * 543210 BIT
>    * 00XXLL -- XX Fault Level LL
> + * ..00LL -- Address Size Fault LL
>    * ..01LL -- Translation Fault LL
>    * ..10LL -- Access Fault LL
>    * ..11LL -- Permission Fault LL
> @@ -534,6 +535,7 @@ extern register_t __cpu_logical_map[];
>   #define FSC_TYPE_OTH   (_AC(0x02,U)<<4)
>   #define FSC_TYPE_IMPL  (_AC(0x03,U)<<4)
>   
> +#define FSC_FLT_ADDR_SIZE (0x00)
>   #define FSC_FLT_TRANS  (0x04)
>   #define FSC_FLT_ACCESS (0x08)
>   #define FSC_FLT_PERM   (0x0c)
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 0c01f37ad6b4..dc0ec8a345ed 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -307,6 +307,10 @@ static const char *decode_fsc(uint32_t fsc, int *level)
>   
>       switch ( fsc & 0x3f )
>       {
> +    case FSC_FLT_ADDR_SIZE ... FSC_FLT_ADDR_SIZE + 3:
> +        msg = "Address size fault";
> +        *level = fsc & FSC_LL_MASK;
> +        break;
>       case FSC_FLT_TRANS ... FSC_FLT_TRANS + 3:
>           msg = "Translation fault";
>           *level = fsc & FSC_LL_MASK;
> @@ -363,6 +367,10 @@ static const char *fsc_level_str(int level)
>       switch ( level )
>       {
>       case -1: return "";
> +#ifdef CONFIG_ARM_64
> +    /* On arm32 the zeroeth level does not exist */
> +    case 0:  return " at level 0";
> +#endif


NIT: Before this patch fsc of 0x00 fell through to default, so it 
printed "Unknown Failure" and level stayed -1. After the patch Arm32 
decodes 0x00 as an address size fault and sets *level = 0, while case 0: 
in fsc_level_str() is compiled out there, so the print becomes "Address 
size fault (level invalid)". So I would either drop the #ifdef (to keep 
the two hunks consistent), or not set the level on Arm32.

That said, I will not insist on the change, my R-b stands either way.


>       case 1:  return " at level 1";
>       case 2:  return " at level 2";
>       case 3:  return " at level 3";