diff mbox series

[Xen-devel,14/27] xen/arm: traps: Improve logging for data/prefetch abort fault

Message ID 20170814142418.13267-15-julien.grall@arm.com
State Superseded
Headers show
Series xen/arm: Memory subsystem clean-up | expand

Commit Message

Julien Grall Aug. 14, 2017, 2:24 p.m. UTC 
Walk the hypervisor page table for data/prefetch abort fault to help
diagnostics error in the page tables.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/traps.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Andre Przywara Aug. 22, 2017, 5:20 p.m. UTC | #1 
Hi,

On 14/08/17 15:24, Julien Grall wrote:
> Walk the hypervisor page table for data/prefetch abort fault to help
> diagnostics error in the page tables.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/traps.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 819bdbc69e..dac4e54fa7 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2967,7 +2967,26 @@ asmlinkage void do_trap_hyp_sync(struct cpu_user_regs *regs)
>          do_trap_brk(regs, hsr);
>          break;
>  #endif
> +    case HSR_EC_DATA_ABORT_CURR_EL:
> +    case HSR_EC_INSTR_ABORT_CURR_EL:
> +    {
> +        bool is_data = (hsr.ec == HSR_EC_DATA_ABORT_CURR_EL);
> +        const char *fault = (is_data) ? "Data Abort" : "Instruction Abort";
> +
> +        printk("%s Trap. Syndrome=%#x\n", fault, hsr.iss);
> +        /*
> +         * FAR may not be valid for a Synchronous External abort other
> +         * than translation table walk.
> +         */
> +        if ( hsr.xabt.fsc != FSC_SEA || !hsr.xabt.fnv )

This is quite hard to read. Would the DeMorgan'ed version be better?
	   if ( hsr.xabt.fsc == FSC_SEA && hsr.xabt.fnv )
	       printk ....
           else
	       dump_hyp_walk ...

> +            dump_hyp_walk(get_hfar(is_data));
> +        else
> +            printk("Invalid FAR, don't walk the hypervisor tables\n");

Nit: "not walking" sounds less ambiguous.

> +        do_unexpected_trap(fault, regs);
>  
> +        break;
> +    }
>      default:
>          printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
>                 hsr.bits, hsr.ec, hsr.len, hsr.iss);

Ignoring the nits above:
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.
Julien Grall Aug. 23, 2017, 7:18 p.m. UTC | #2 
On 08/22/2017 06:20 PM, Andre Przywara wrote:
> Hi,

Hi Andre,


> On 14/08/17 15:24, Julien Grall wrote:
>> Walk the hypervisor page table for data/prefetch abort fault to help
>> diagnostics error in the page tables.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/traps.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 819bdbc69e..dac4e54fa7 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2967,7 +2967,26 @@ asmlinkage void do_trap_hyp_sync(struct cpu_user_regs *regs)
>>           do_trap_brk(regs, hsr);
>>           break;
>>   #endif
>> +    case HSR_EC_DATA_ABORT_CURR_EL:
>> +    case HSR_EC_INSTR_ABORT_CURR_EL:
>> +    {
>> +        bool is_data = (hsr.ec == HSR_EC_DATA_ABORT_CURR_EL);
>> +        const char *fault = (is_data) ? "Data Abort" : "Instruction Abort";
>> +
>> +        printk("%s Trap. Syndrome=%#x\n", fault, hsr.iss);
>> +        /*
>> +         * FAR may not be valid for a Synchronous External abort other
>> +         * than translation table walk.
>> +         */
>> +        if ( hsr.xabt.fsc != FSC_SEA || !hsr.xabt.fnv )
> 
> This is quite hard to read. Would the DeMorgan'ed version be better?
> 	   if ( hsr.xabt.fsc == FSC_SEA && hsr.xabt.fnv )
> 	       printk ....
>             else
> 	       dump_hyp_walk ...

Indeed it is better. I will use that.

Cheers,

> 
>> +            dump_hyp_walk(get_hfar(is_data));
>> +        else
>> +            printk("Invalid FAR, don't walk the hypervisor tables\n");
> 
> Nit: "not walking" sounds less ambiguous.
> 
>> +        do_unexpected_trap(fault, regs);
>>   
>> +        break;
>> +    }
>>       default:
>>           printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
>>                  hsr.bits, hsr.ec, hsr.len, hsr.iss);
> 
> Ignoring the nits above:

I will fix both and keep the reviewed-by if you don't mind.

> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 819bdbc69e..dac4e54fa7 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2967,7 +2967,26 @@  asmlinkage void do_trap_hyp_sync(struct cpu_user_regs *regs)
         do_trap_brk(regs, hsr);
         break;
 #endif
+    case HSR_EC_DATA_ABORT_CURR_EL:
+    case HSR_EC_INSTR_ABORT_CURR_EL:
+    {
+        bool is_data = (hsr.ec == HSR_EC_DATA_ABORT_CURR_EL);
+        const char *fault = (is_data) ? "Data Abort" : "Instruction Abort";
+
+        printk("%s Trap. Syndrome=%#x\n", fault, hsr.iss);
+        /*
+         * FAR may not be valid for a Synchronous External abort other
+         * than translation table walk.
+         */
+        if ( hsr.xabt.fsc != FSC_SEA || !hsr.xabt.fnv )
+            dump_hyp_walk(get_hfar(is_data));
+        else
+            printk("Invalid FAR, don't walk the hypervisor tables\n");
+
+        do_unexpected_trap(fault, regs);
 
+        break;
+    }
     default:
         printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
                hsr.bits, hsr.ec, hsr.len, hsr.iss);