| Message ID | 1466601669-25398-8-git-send-email-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 22 Jun 2016, Julien Grall wrote: > The function do_trap_data_abort_guest assumes that a stage-2 data abort > can only be taken for a translation fault or permission fault today. > > Whilst this is true today, it might not be in the future. Rather than > emulating the MMIO for any fault other than the permission one, print > a warning message when the fault is not handled by Xen. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/traps.c | 51 ++++++++++++++++++++++++++++----------------------- > 1 file changed, 28 insertions(+), 23 deletions(-) > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index 785e3e9..591de3c 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -2468,35 +2468,40 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs, > /* Trap was triggered by mem_access, work here is done */ > if ( !rc ) > return; > + break; > } > - break; > - } > - > - if ( dabt.s1ptw ) > - goto bad_data_abort; > + case FSC_FLT_TRANS: > + if ( dabt.s1ptw ) > + goto bad_data_abort; > > - /* XXX: Decode the instruction if ISS is not valid */ > - if ( !dabt.valid ) > - goto bad_data_abort; > + /* XXX: Decode the instruction if ISS is not valid */ > + if ( !dabt.valid ) > + goto bad_data_abort; > > - /* > - * Erratum 766422: Thumb store translation fault to Hypervisor may > - * not have correct HSR Rt value. > - */ > - if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) && dabt.write ) > - { > - rc = decode_instruction(regs, &info.dabt); > - if ( rc ) > + /* > + * Erratum 766422: Thumb store translation fault to Hypervisor may > + * not have correct HSR Rt value. > + */ > + if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) && > + dabt.write ) > { > - gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); > - goto bad_data_abort; > + rc = decode_instruction(regs, &info.dabt); > + if ( rc ) > + { > + gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); > + goto bad_data_abort; > + } > } > - } > > - if (handle_mmio(&info)) > - { > - advance_pc(regs, hsr); > - return; > + if ( handle_mmio(&info) ) > + { > + advance_pc(regs, hsr); > + return; > + } > + break; > + default: > + gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n", > + hsr.bits, dabt.dfsc); Given that bad_data_abort, which is right after, will print HSR again, I would remove it from this message as it's redundant. > } > > bad_data_abort: > -- > 1.9.1 >
Hi Stefano, On 14/07/16 16:06, Stefano Stabellini wrote: > On Wed, 22 Jun 2016, Julien Grall wrote: >> - if (handle_mmio(&info)) >> - { >> - advance_pc(regs, hsr); >> - return; >> + if ( handle_mmio(&info) ) >> + { >> + advance_pc(regs, hsr); >> + return; >> + } >> + break; >> + default: >> + gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n", >> + hsr.bits, dabt.dfsc); > > Given that bad_data_abort, which is right after, will print HSR again, I > would remove it from this message as it's redundant. I agree this is redundant, however the message below will only be printed in debug build because a guest could trigger it easily. The message above is here to catch any possible misconfiguration of stage-2 page table or hardware issue (ECC error, address size, TLB conflict...) and could be seen in non-debug build. Cheers,
On Thu, 14 Jul 2016, Julien Grall wrote: > Hi Stefano, > > On 14/07/16 16:06, Stefano Stabellini wrote: > > On Wed, 22 Jun 2016, Julien Grall wrote: > > > - if (handle_mmio(&info)) > > > - { > > > - advance_pc(regs, hsr); > > > - return; > > > + if ( handle_mmio(&info) ) > > > + { > > > + advance_pc(regs, hsr); > > > + return; > > > + } > > > + break; > > > + default: > > > + gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n", > > > + hsr.bits, dabt.dfsc); > > > > Given that bad_data_abort, which is right after, will print HSR again, I > > would remove it from this message as it's redundant. > > I agree this is redundant, however the message below will only be printed in > debug build because a guest could trigger it easily. The message above is here > to catch any possible misconfiguration of stage-2 page table or hardware issue > (ECC error, address size, TLB conflict...) and could be seen in non-debug > build. Fair enough, it will just look a bit weird in the source code. Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
On 14/07/16 16:28, Stefano Stabellini wrote: > On Thu, 14 Jul 2016, Julien Grall wrote: >> Hi Stefano, >> >> On 14/07/16 16:06, Stefano Stabellini wrote: >>> On Wed, 22 Jun 2016, Julien Grall wrote: >>>> - if (handle_mmio(&info)) >>>> - { >>>> - advance_pc(regs, hsr); >>>> - return; >>>> + if ( handle_mmio(&info) ) >>>> + { >>>> + advance_pc(regs, hsr); >>>> + return; >>>> + } >>>> + break; >>>> + default: >>>> + gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n", >>>> + hsr.bits, dabt.dfsc); >>> >>> Given that bad_data_abort, which is right after, will print HSR again, I >>> would remove it from this message as it's redundant. >> >> I agree this is redundant, however the message below will only be printed in >> debug build because a guest could trigger it easily. The message above is here >> to catch any possible misconfiguration of stage-2 page table or hardware issue >> (ECC error, address size, TLB conflict...) and could be seen in non-debug >> build. > > Fair enough, it will just look a bit weird in the source code. Would a comment in the source code make it less weird? > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 785e3e9..591de3c 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -2468,35 +2468,40 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs, /* Trap was triggered by mem_access, work here is done */ if ( !rc ) return; + break; } - break; - } - - if ( dabt.s1ptw ) - goto bad_data_abort; + case FSC_FLT_TRANS: + if ( dabt.s1ptw ) + goto bad_data_abort; - /* XXX: Decode the instruction if ISS is not valid */ - if ( !dabt.valid ) - goto bad_data_abort; + /* XXX: Decode the instruction if ISS is not valid */ + if ( !dabt.valid ) + goto bad_data_abort; - /* - * Erratum 766422: Thumb store translation fault to Hypervisor may - * not have correct HSR Rt value. - */ - if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) && dabt.write ) - { - rc = decode_instruction(regs, &info.dabt); - if ( rc ) + /* + * Erratum 766422: Thumb store translation fault to Hypervisor may + * not have correct HSR Rt value. + */ + if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) && + dabt.write ) { - gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); - goto bad_data_abort; + rc = decode_instruction(regs, &info.dabt); + if ( rc ) + { + gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); + goto bad_data_abort; + } } - } - if (handle_mmio(&info)) - { - advance_pc(regs, hsr); - return; + if ( handle_mmio(&info) ) + { + advance_pc(regs, hsr); + return; + } + break; + default: + gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n", + hsr.bits, dabt.dfsc); } bad_data_abort:
The function do_trap_data_abort_guest assumes that a stage-2 data abort can only be taken for a translation fault or permission fault today. Whilst this is true today, it might not be in the future. Rather than emulating the MMIO for any fault other than the permission one, print a warning message when the fault is not handled by Xen. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/traps.c | 51 ++++++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 23 deletions(-)