| Message ID | 1410279788-27167-5-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi Ian, On 09/09/14 09:23, Ian Campbell wrote: > Previously userspace access to PM* would have been incorrectly (but benignly) > implemented as RAZ/WI when running on a 32-bit kernel and would cause a > hypervisor exception (host crash) when running a 64-bit kernel (this was > already solved via the fix to XSA-102). > > CLIDR, CCSIDR, DCCISW, ACTLR, PMINTENSET, PMINTENCLR are EL1 only, attempts to > access from EL0 will trap to EL1 not to us, hence BUG_ON is appropriate now. In the unlikely case it happens, I don't think it will harm Xen, but only the guest. So is the BUG_ON really necessary on these registers? I think we should use BUG_ON when we know that it will harm the Xen and it's not possible to come back from the state. Regards,
On Tue, 2014-09-09 at 16:42 -0700, Julien Grall wrote: > Hi Ian, > > On 09/09/14 09:23, Ian Campbell wrote: > > Previously userspace access to PM* would have been incorrectly (but benignly) > > implemented as RAZ/WI when running on a 32-bit kernel and would cause a > > hypervisor exception (host crash) when running a 64-bit kernel (this was > > already solved via the fix to XSA-102). > > > > CLIDR, CCSIDR, DCCISW, ACTLR, PMINTENSET, PMINTENCLR are EL1 only, attempts to > > access from EL0 will trap to EL1 not to us, hence BUG_ON is appropriate now. > > In the unlikely case it happens, I don't think it will harm Xen, but > only the guest. So is the BUG_ON really necessary on these registers? > > I think we should use BUG_ON when we know that it will harm the Xen and > it's not possible to come back from the state. AIUI it would be a hardware bug to see these traps in Xen. I think a BUG_ON is acceptable for such an occurrence. Ian.
On 10/09/14 02:48, Ian Campbell wrote: > On Tue, 2014-09-09 at 16:42 -0700, Julien Grall wrote: >> Hi Ian, >> >> On 09/09/14 09:23, Ian Campbell wrote: >>> Previously userspace access to PM* would have been incorrectly (but benignly) >>> implemented as RAZ/WI when running on a 32-bit kernel and would cause a >>> hypervisor exception (host crash) when running a 64-bit kernel (this was >>> already solved via the fix to XSA-102). >>> >>> CLIDR, CCSIDR, DCCISW, ACTLR, PMINTENSET, PMINTENCLR are EL1 only, attempts to >>> access from EL0 will trap to EL1 not to us, hence BUG_ON is appropriate now. >> >> In the unlikely case it happens, I don't think it will harm Xen, but >> only the guest. So is the BUG_ON really necessary on these registers? >> >> I think we should use BUG_ON when we know that it will harm the Xen and >> it's not possible to come back from the state. > > AIUI it would be a hardware bug to see these traps in Xen. I think a > BUG_ON is acceptable for such an occurrence. I would turn into an ASSERT to avoid checking it in non-debug build. Regards,
On Wed, 2014-09-10 at 11:56 -0700, Julien Grall wrote: > > On 10/09/14 02:48, Ian Campbell wrote: > > On Tue, 2014-09-09 at 16:42 -0700, Julien Grall wrote: > >> Hi Ian, > >> > >> On 09/09/14 09:23, Ian Campbell wrote: > >>> Previously userspace access to PM* would have been incorrectly (but benignly) > >>> implemented as RAZ/WI when running on a 32-bit kernel and would cause a > >>> hypervisor exception (host crash) when running a 64-bit kernel (this was > >>> already solved via the fix to XSA-102). > >>> > >>> CLIDR, CCSIDR, DCCISW, ACTLR, PMINTENSET, PMINTENCLR are EL1 only, attempts to > >>> access from EL0 will trap to EL1 not to us, hence BUG_ON is appropriate now. > >> > >> In the unlikely case it happens, I don't think it will harm Xen, but > >> only the guest. So is the BUG_ON really necessary on these registers? > >> > >> I think we should use BUG_ON when we know that it will harm the Xen and > >> it's not possible to come back from the state. > > > > AIUI it would be a hardware bug to see these traps in Xen. I think a > > BUG_ON is acceptable for such an occurrence. > > I would turn into an ASSERT to avoid checking it in non-debug build. BUG_ON is what we normally use in these circumstances. If we did hit this case we would want to stop, not carry on with hardware in a potentially unknown/unpredictable state, even for a non-debug build. Ian.
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 46ed21d..e7a2791 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1446,6 +1446,7 @@ static void do_cp15_32(struct cpu_user_regs *regs, switch ( hsr.bits & HSR_CP32_REGS_MASK ) { case HSR_CPREG32(CLIDR): + BUG_ON(psr_mode_is_user(regs)); if ( !cp32.read ) { dprintk(XENLOG_ERR, @@ -1455,6 +1456,7 @@ static void do_cp15_32(struct cpu_user_regs *regs, *r = READ_SYSREG32(CLIDR_EL1); break; case HSR_CPREG32(CCSIDR): + BUG_ON(psr_mode_is_user(regs)); if ( !cp32.read ) { dprintk(XENLOG_ERR, @@ -1464,6 +1466,7 @@ static void do_cp15_32(struct cpu_user_regs *regs, *r = READ_SYSREG32(CCSIDR_EL1); break; case HSR_CPREG32(DCCISW): + BUG_ON(psr_mode_is_user(regs)); if ( cp32.read ) { dprintk(XENLOG_ERR, @@ -1481,6 +1484,7 @@ static void do_cp15_32(struct cpu_user_regs *regs, goto undef_cp15_32; case HSR_CPREG32(ACTLR): + BUG_ON(psr_mode_is_user(regs)); if ( cp32.read ) *r = v->arch.actlr; break; @@ -1493,6 +1497,18 @@ static void do_cp15_32(struct cpu_user_regs *regs, * always support PMCCNTR (the cyle counter): we just RAZ/WI for all * PM register, which doesn't crash the kernel at least */ + case HSR_CPREG32(PMUSERENR): + /* RO at EL0. RAZ/WI at EL1 */ + if ( psr_mode_is_user(regs) && !hsr.cp32.read ) + goto undef_cp15_32; + goto cp15_32_raz_wi; + + case HSR_CPREG32(PMINTENSET): + case HSR_CPREG32(PMINTENCLR): + /* EL1 only */ + BUG_ON(psr_mode_is_user(regs)); + goto cp15_32_raz_wi; + case HSR_CPREG32(PMCR): case HSR_CPREG32(PMCNTENSET): case HSR_CPREG32(PMCNTENCLR): @@ -1504,12 +1520,19 @@ static void do_cp15_32(struct cpu_user_regs *regs, case HSR_CPREG32(PMCCNTR): case HSR_CPREG32(PMXEVTYPER): case HSR_CPREG32(PMXEVCNTR): - case HSR_CPREG32(PMUSERENR): - case HSR_CPREG32(PMINTENSET): - case HSR_CPREG32(PMINTENCLR): case HSR_CPREG32(PMOVSSET): + /* + * Accessible at EL0 only if PMUSERENR_EL0.EN is set. We + * emulate that register as 0 above. + */ + if ( psr_mode_is_user(regs) ) + goto undef_cp15_32; + /* Fall thru */ + + cp15_32_raz_wi: if ( cp32.read ) *r = 0; + /* else: write ignored */ break; default: @@ -1908,8 +1931,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) advance_pc(regs, hsr); break; case HSR_EC_CP15_32: - if ( !is_32bit_domain(current->domain) ) - goto bad_trap; + BUG_ON(!psr_mode_is_32bit(regs->cpsr)); do_cp15_32(regs, hsr); break; case HSR_EC_CP15_64:
Previously userspace access to PM* would have been incorrectly (but benignly) implemented as RAZ/WI when running on a 32-bit kernel and would cause a hypervisor exception (host crash) when running a 64-bit kernel (this was already solved via the fix to XSA-102). CLIDR, CCSIDR, DCCISW, ACTLR, PMINTENSET, PMINTENCLR are EL1 only, attempts to access from EL0 will trap to EL1 not to us, hence BUG_ON is appropriate now. PMUSERENR is R/O at EL0 and we implement as RAZ/WI at EL1 as before. The remaining PM* registers are accessible to EL0 only if PMUSERENR_EL0.EN is set, since we emulate this as RAZ/WI the bit is never set so we inject a trap on attempted access. We weren't previously handling PMCCNTR. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- xen/arch/arm/traps.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-)