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

[Julien Grall]

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 
---
  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 


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Andre Przywara]

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 
> ---
>  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 

Cheers,
Andre.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Julien Grall]

Walk the hypervisor page table for data/prefetch abort fault to help
diagnostics error in the page tables.

Signed-off-by: Julien Grall 
---
 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 )
+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);
-- 
2.11.0


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel