| Message ID | 20170814142418.13267-15-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 --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);
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(+)