| Message ID | 1467743871-9644-2-git-send-email-tamas.lengyel@zentific.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/07/16 19:37, Tamas K Lengyel wrote: > In AArch32 state, the ARMv8-A architecture permits, but does not require, > this trap to apply to conditional SMC instructions that fail their condition > code check, in the same way as with traps on other conditional instructions. Please add a quote to the spec. > Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> > Suggested-by: Julien Grall <julien.grall@arm.com> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/traps.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index 44926ca..627e8c9 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -2507,6 +2507,17 @@ bad_data_abort: > inject_dabt_exception(regs, info.gva, hsr.len); > } > > +static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr) > +{ > + if ( !check_conditional_instr(regs, hsr) ) This function is checking the EC, it considers that EC > 0x10 will be unconditional. However, the SMC exception class is 0x13 when taken from AArch32 and 0x17 when taken from AArch64. Furthermore, for ARMv7, the register is Reserved UNK/SBZP (see B3-1431 in ARM DDI 0406C.c). I.e the software should not rely on the field reading as all 0s (see Glossary-2736). For ARMv8, when the SMC is taken from AArch64 (see D7-1942 in ARM DDI 0487A.j), the register is RES0 which means the software should not rely on the value to always be 0 (see Glossary-5734). When the SMC is taken from AArch32, the field CV is only valid if CCKNOWNPASS is 1 (this field does not exist for the other exception class). So this code need more extra care. It would also be nice to have a description in the code explaining what's going on. > + { > + advance_pc(regs, hsr); > + return; > + } > + > + inject_undef_exception(regs, hsr); > +} > + > static void enter_hypervisor_head(struct cpu_user_regs *regs) > { > if ( guest_mode(regs) ) > @@ -2582,7 +2593,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) > */ > GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); > perfc_incr(trap_smc32); > - inject_undef32_exception(regs); > + do_trap_smc(regs, hsr); > break; > case HSR_EC_HVC32: > GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); > @@ -2615,7 +2626,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) > */ > GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr)); > perfc_incr(trap_smc64); > - inject_undef64_exception(regs, hsr.len); > + do_trap_smc(regs, hsr); > break; > case HSR_EC_SYSREG: > GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr)); > Regards,
On Wed, Jul 6, 2016 at 11:31 AM, Julien Grall <julien.grall@arm.com> wrote: > On 05/07/16 19:37, Tamas K Lengyel wrote: >> >> In AArch32 state, the ARMv8-A architecture permits, but does not require, >> this trap to apply to conditional SMC instructions that fail their >> condition >> code check, in the same way as with traps on other conditional >> instructions. > > > Please add a quote to the spec. > >> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> >> Suggested-by: Julien Grall <julien.grall@arm.com> >> --- >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Julien Grall <julien.grall@arm.com> >> --- >> xen/arch/arm/traps.c | 15 +++++++++++++-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >> index 44926ca..627e8c9 100644 >> --- a/xen/arch/arm/traps.c >> +++ b/xen/arch/arm/traps.c >> @@ -2507,6 +2507,17 @@ bad_data_abort: >> inject_dabt_exception(regs, info.gva, hsr.len); >> } >> >> +static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr) >> +{ >> + if ( !check_conditional_instr(regs, hsr) ) > > > This function is checking the EC, it considers that EC > 0x10 will be > unconditional. However, the SMC exception class is 0x13 when taken from > AArch32 and 0x17 when taken from AArch64. > > Furthermore, for ARMv7, the register is Reserved UNK/SBZP (see B3-1431 in > ARM DDI 0406C.c). I.e the software should not rely on the field reading as > all 0s (see Glossary-2736). > > For ARMv8, when the SMC is taken from AArch64 (see D7-1942 in ARM DDI > 0487A.j), the register is RES0 which means the software should not rely on > the value to always be 0 (see Glossary-5734). When the SMC is taken from > AArch32, the field CV is only valid if CCKNOWNPASS is 1 (this field does not > exist for the other exception class). > > So this code need more extra care. It would also be nice to have a > description in the code explaining what's going on. Ack, there seems to be enough corner-cases here that I'm unfamiliar with that I would rather not attempt implementing this as I likely would miss something. I'll wait for your patch with the rest of my series as that route will likely have less overhead anyway. Thanks, Tamas
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 44926ca..627e8c9 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -2507,6 +2507,17 @@ bad_data_abort: inject_dabt_exception(regs, info.gva, hsr.len); } +static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr) +{ + if ( !check_conditional_instr(regs, hsr) ) + { + advance_pc(regs, hsr); + return; + } + + inject_undef_exception(regs, hsr); +} + static void enter_hypervisor_head(struct cpu_user_regs *regs) { if ( guest_mode(regs) ) @@ -2582,7 +2593,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) */ GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); perfc_incr(trap_smc32); - inject_undef32_exception(regs); + do_trap_smc(regs, hsr); break; case HSR_EC_HVC32: GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); @@ -2615,7 +2626,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) */ GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr)); perfc_incr(trap_smc64); - inject_undef64_exception(regs, hsr.len); + do_trap_smc(regs, hsr); break; case HSR_EC_SYSREG: GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
In AArch32 state, the ARMv8-A architecture permits, but does not require, this trap to apply to conditional SMC instructions that fail their condition code check, in the same way as with traps on other conditional instructions. Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> Suggested-by: Julien Grall <julien.grall@arm.com> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/traps.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)