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

On 26-Aug-26 17:47, Oleksandr Tyshchenko wrote:
> 
> 
> 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.
Actually, FSC 0 means address size fault at...:
 - on AArch64: level 0 OR translation table base register
 - on AArch32: translation table base register
so I will say: "at level 0 or TTBR" and drop the #ifdef.

~Michal