xen: arm: cleanup traps.c
cover xen: arm: cleanup traps.c
While working on reenabling 32-bit user space on arm63 I concluded that the trap handling in traps.c had grown into a twisty confusing mess. Lets try and sort that out.
This series contains to halves (after a couple of preparatory cleanups).
First clean up the goto maze which we've found ourselves in, by providing a selection of handle_* helpers e.g. for raz/ro etc and by calling those and the existing inject_* helpers directly instead of trying to have only one call to each of the latter by using goto. The handle_* helpers can also deal with the minimum allowable exception level, which simplifies things further.
To keep things simpler I've used "return handle_..." when the caller and callee both return void, since that avoids the need for 3 more lines (2 braces and the return), I think this improves clarity.
Second go through init_traps and for each bit there consolidate the handling for each type of trap (e.g. do_cp15_32, do_cp15_64, do_sysreg etc) such that all the registers whose traps are associated with that bit are kept together beneath a comment which documents why those bits are trapped, references the appropriate section of the ARMv7 and ARMv8 ARM (the v8 one in particular has a series of very useful tables per bit) and notes which registers are not explicitly handled (and therefore take the default case).
For traps which have no explicit handling (i.e. those which trap implementation defined registers) and which always hit the default case add the comment above that instead.
Do the same for the GICv3 ICC traps and timer traps.
There is probably scope for doing more, i.e. refactoring related functionality into subsystem helpers (like we do for vtimer) and even moving into separate files, but I think this is a good start.
This is a lot of patches, sorry, because I wanted to mostly go through the trap bits one at a time per patch to keep each one manageable, although I did end up compressing some of the more obvious ones.
Ian.
PATCH 1/19 xen: arm: constify union hsr and struct hsr_* where possible.
--- xen/arch/arm/traps.c | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-)
xen/arch/arm/traps.c +21 -20
@@ -447,7 +447,7 @@ static vaddr_t exception_handler64(struct cpu_user_regs *regs, vaddr_t offset)static void inject_undef64_exception (struct cpu_user_regs *regs,int instr_len) {vaddr_t handler; -union hsr esr = { +const union hsr esr = { .iss =0 , .len = instr_len, .ec =HSR_EC_UNKNOWN ,
@@ -1141,7 +1141,7 @@ int do_bug_frame(struct cpu_user_regs *regs, vaddr_t pc) }#ifdef CONFIG_ARM_64 -static void do_trap_brk (struct cpu_user_regs *regs,union hsr hsr) +static void do_trap_brk (struct cpu_user_regs *regs,const union hsr hsr) {/* HCR_EL2.TGE and MDCR_EL2.TDE are not set so we never receive * software breakpoint exception for EL1 and EL0 here.
@@ -1488,7 +1488,8 @@ static const unsigned short cc_map[16] = { 0 /* NV */ }; -static int check_conditional_instr (struct cpu_user_regs *regs,union hsr hsr) +static int check_conditional_instr (struct cpu_user_regs *regs, +const union hsr hsr) {unsigned long cpsr, cpsr_cond;int cond;
@@ -1533,7 +1534,7 @@ static int check_conditional_instr(struct cpu_user_regs *regs, union hsr hsr)return 1 ; } -static void advance_pc (struct cpu_user_regs *regs,union hsr hsr) +static void advance_pc (struct cpu_user_regs *regs,const union hsr hsr) {unsigned long itbits, cond, cpsr = regs->cpsr;
@@ -1574,9 +1575,9 @@ static void advance_pc(struct cpu_user_regs *regs, union hsr hsr) }static void do_cp15_32 (struct cpu_user_regs *regs, -union hsr hsr) +const union hsr hsr) { -struct hsr_cp32 cp32 = hsr.cp32; +const struct hsr_cp32 cp32 = hsr.cp32;uint32_t *r = (uint32_t *)select_user_reg (regs, cp32.reg);struct vcpu *v = current;
@@ -1659,7 +1660,7 @@ static void do_cp15_32(struct cpu_user_regs *regs, }static void do_cp15_64 (struct cpu_user_regs *regs, -union hsr hsr) +const union hsr hsr) {if ( !check_conditional_instr (regs, hsr) ) {
@@ -1676,7 +1677,7 @@ static void do_cp15_64(struct cpu_user_regs *regs,break ;default : { -struct hsr_cp64 cp64 = hsr.cp64; +const struct hsr_cp64 cp64 = hsr.cp64; gdprintk(XENLOG_ERR, "%s p15, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
@@ -1692,9 +1693,9 @@ static void do_cp15_64(struct cpu_user_regs *regs,advance_pc (regs, hsr); } -static void do_cp14_32 (struct cpu_user_regs *regs,union hsr hsr) +static void do_cp14_32 (struct cpu_user_regs *regs,const union hsr hsr) { -struct hsr_cp32 cp32 = hsr.cp32; +const struct hsr_cp32 cp32 = hsr.cp32;uint32_t *r = (uint32_t *)select_user_reg (regs, cp32.reg);struct domain *d = current->domain;
@@ -1784,9 +1785,9 @@ static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)advance_pc (regs, hsr); } -static void do_cp14_dbg (struct cpu_user_regs *regs,union hsr hsr) +static void do_cp14_dbg (struct cpu_user_regs *regs,const union hsr hsr) { -struct hsr_cp64 cp64 = hsr.cp64; +const struct hsr_cp64 cp64 = hsr.cp64;if ( !check_conditional_instr (regs, hsr) ) {
@@ -1804,9 +1805,9 @@ static void do_cp14_dbg(struct cpu_user_regs *regs, union hsr hsr)inject_undef_exception (regs, hsr.len); } -static void do_cp (struct cpu_user_regs *regs,union hsr hsr) +static void do_cp (struct cpu_user_regs *regs,const union hsr hsr) { -struct hsr_cp cp = hsr.cp; +const struct hsr_cp cp = hsr.cp;if ( !check_conditional_instr (regs, hsr) ) {
@@ -1821,7 +1822,7 @@ static void do_cp(struct cpu_user_regs *regs, union hsr hsr)#ifdef CONFIG_ARM_64 static void do_sysreg (struct cpu_user_regs *regs, -union hsr hsr) +const union hsr hsr) {register_t *x =select_user_reg (regs, hsr.sysreg.reg);
@@ -1918,7 +1919,7 @@ static void do_sysreg(struct cpu_user_regs *regs,inject_undef64_exception (regs, hsr.len);default : { -struct hsr_sysreg sysreg = hsr.sysreg; +const struct hsr_sysreg sysreg = hsr.sysreg; gdprintk(XENLOG_ERR, "%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
@@ -1997,16 +1998,16 @@ done: }static void do_trap_instr_abort_guest (struct cpu_user_regs *regs, -union hsr hsr) +const union hsr hsr) {register_t addr =READ_SYSREG (FAR_EL2 );inject_iabt_exception (regs, addr, hsr.len); }static void do_trap_data_abort_guest (struct cpu_user_regs *regs, -union hsr hsr) +const union hsr hsr) { -struct hsr_dabt dabt = hsr.dabt; +const struct hsr_dabt dabt = hsr.dabt;int rc;mmio_info_t info;
@@ -2066,7 +2067,7 @@ static void enter_hypervisor_head(struct cpu_user_regs *regs) asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) { -union hsr hsr = { .bits =READ_SYSREG32 (ESR_EL2 ) }; +const union hsr hsr = { .bits =READ_SYSREG32 (ESR_EL2 ) };enter_hypervisor_head (regs);
Signature · 1 line
1.7.10.4
Re: [PATCH 01/19] xen: arm: constify union hsr and struct hsr_* where possible.
Hi Ian,
Quoted 1 line · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>
Regards,
Quoted 159 lines
--- xen/arch/arm/traps.c | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-)xen/arch/arm/traps.c +21
-20@@ -447,7 +447,7 @@ static vaddr_t exception_handler64(struct cpu_user_regs *regs, vaddr_t offset)static void inject_undef64_exception (struct cpu_user_regs *regs,int instr_len) {vaddr_t handler; -union hsr esr = { +const union hsr esr = { .iss =0 , .len = instr_len, .ec =HSR_EC_UNKNOWN ,@@ -1141,7 +1141,7 @@ int do_bug_frame(struct cpu_user_regs *regs, vaddr_t pc) }#ifdef CONFIG_ARM_64 -static void do_trap_brk (struct cpu_user_regs *regs,union hsr hsr) +static void do_trap_brk (struct cpu_user_regs *regs,const union hsr hsr) {/* HCR_EL2.TGE and MDCR_EL2.TDE are not set so we never receive * software breakpoint exception for EL1 and EL0 here. @@ -1488,7 +1488,8 @@ static const unsigned short cc_map[16] = { 0 /* NV */ }; -static int check_conditional_instr (struct cpu_user_regs *regs,union hsr hsr) +static int check_conditional_instr (struct cpu_user_regs *regs, +const union hsr hsr) {unsigned long cpsr, cpsr_cond;int cond;@@ -1533,7 +1534,7 @@ static int check_conditional_instr(struct cpu_user_regs *regs, union hsr hsr)return 1 ; } -static void advance_pc (struct cpu_user_regs *regs,union hsr hsr) +static void advance_pc (struct cpu_user_regs *regs,const union hsr hsr) {unsigned long itbits, cond, cpsr = regs->cpsr;@@ -1574,9 +1575,9 @@ static void advance_pc(struct cpu_user_regs *regs, union hsr hsr) }static void do_cp15_32 (struct cpu_user_regs *regs, -union hsr hsr) +const union hsr hsr) { -struct hsr_cp32 cp32 = hsr.cp32; +const struct hsr_cp32 cp32 = hsr.cp32;uint32_t *r = (uint32_t *)select_user_reg (regs, cp32.reg);struct vcpu *v = current;@@ -1659,7 +1660,7 @@ static void do_cp15_32(struct cpu_user_regs *regs, }static void do_cp15_64 (struct cpu_user_regs *regs, -union hsr hsr) +const union hsr hsr) {if ( !check_conditional_instr (regs, hsr) ) {@@ -1676,7 +1677,7 @@ static void do_cp15_64(struct cpu_user_regs *regs,break ;default : { -struct hsr_cp64 cp64 = hsr.cp64; +const struct hsr_cp64 cp64 = hsr.cp64; gdprintk(XENLOG_ERR, "%s p15, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",@@ -1692,9 +1693,9 @@ static void do_cp15_64(struct cpu_user_regs *regs,advance_pc (regs, hsr); } -static void do_cp14_32 (struct cpu_user_regs *regs,union hsr hsr) +static void do_cp14_32 (struct cpu_user_regs *regs,const union hsr hsr) { -struct hsr_cp32 cp32 = hsr.cp32; +const struct hsr_cp32 cp32 = hsr.cp32;uint32_t *r = (uint32_t *)select_user_reg (regs, cp32.reg);struct domain *d = current->domain;@@ -1784,9 +1785,9 @@ static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)advance_pc (regs, hsr); } -static void do_cp14_dbg (struct cpu_user_regs *regs,union hsr hsr) +static void do_cp14_dbg (struct cpu_user_regs *regs,const union hsr hsr) { -struct hsr_cp64 cp64 = hsr.cp64; +const struct hsr_cp64 cp64 = hsr.cp64;if ( !check_conditional_instr (regs, hsr) ) {@@ -1804,9 +1805,9 @@ static void do_cp14_dbg(struct cpu_user_regs *regs, union hsr hsr)inject_undef_exception (regs, hsr.len); } -static void do_cp (struct cpu_user_regs *regs,union hsr hsr) +static void do_cp (struct cpu_user_regs *regs,const union hsr hsr) { -struct hsr_cp cp = hsr.cp; +const struct hsr_cp cp = hsr.cp;if ( !check_conditional_instr (regs, hsr) ) {@@ -1821,7 +1822,7 @@ static void do_cp(struct cpu_user_regs *regs, union hsr hsr)#ifdef CONFIG_ARM_64 static void do_sysreg (struct cpu_user_regs *regs, -union hsr hsr) +const union hsr hsr) {register_t *x =select_user_reg (regs, hsr.sysreg.reg);@@ -1918,7 +1919,7 @@ static void do_sysreg(struct cpu_user_regs *regs,inject_undef64_exception (regs, hsr.len);default : { -struct hsr_sysreg sysreg = hsr.sysreg; +const struct hsr_sysreg sysreg = hsr.sysreg; gdprintk(XENLOG_ERR, "%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",@@ -1997,16 +1998,16 @@ done: }static void do_trap_instr_abort_guest (struct cpu_user_regs *regs, -union hsr hsr) +const union hsr hsr) {register_t addr =READ_SYSREG (FAR_EL2 );inject_iabt_exception (regs, addr, hsr.len); }static void do_trap_data_abort_guest (struct cpu_user_regs *regs, -union hsr hsr) +const union hsr hsr) { -struct hsr_dabt dabt = hsr.dabt; +const struct hsr_dabt dabt = hsr.dabt;int rc;mmio_info_t info;@@ -2066,7 +2067,7 @@ static void enter_hypervisor_head(struct cpu_user_regs *regs) asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) { -union hsr hsr = { .bits =READ_SYSREG32 (ESR_EL2 ) }; +const union hsr hsr = { .bits =READ_SYSREG32 (ESR_EL2 ) };enter_hypervisor_head (regs);
Signature · 1 line
Julien Grall
Julien Grall
Reviewed-byRe: [PATCH 01/19] xen: arm: constify union hsr and struct hsr_* where possible.
Quoted 5 lines · Julien Grall
Hi Ian,
Quoted 1 line · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>Reviewed-by Julien Grall <[email protected]>
Hmmm... Sorry I'm not use to use my citrix mail anymore. It should be
Signature · 1 line
Julien Grall
PATCH 2/19 xen: arm: add missing break
In reply to Ian Campbellxen: arm: Fix handling of ICC_{SGI1R,SGI0R,ASGI1R}_EL1
Having injected an undefined instruction we don't want to also advance pc. So return.
THe ICC_{SGI0R,ASGI1R}_EL1 case was previously missing a break, so would have fallen through to the default case and injected a second undef, corrupting SPSR_EL1 and ELR_EL1 for the guest.
--- xen/arch/arm/traps.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
xen/arch/arm/traps.c +2 -2
@@ -1908,7 +1908,7 @@ static void do_sysreg(struct cpu_user_regs *regs, {dprintk (XENLOG_WARNING ,"failed emulation of sysreg ICC_SGI1R_EL1 access\n" ); -inject_undef64_exception (regs, hsr.len); +return inject_undef64_exception (regs, hsr.len); }break ;case HSR_SYSREG_ICC_SGI0R_EL1 :
@@ -1916,7 +1916,7 @@ static void do_sysreg(struct cpu_user_regs *regs,/* TBD: Implement to support secure grp0/1 SGI forwarding */ dprintk (XENLOG_WARNING ,"Emulation of sysreg ICC_SGI0R_EL1/ASGI1R_EL1 not supported\n" ); -inject_undef64_exception (regs, hsr.len); +return inject_undef64_exception (regs, hsr.len);default : {const struct hsr_sysreg sysreg = hsr.sysreg;
Signature · 1 line
1.7.10.4
Re: [PATCH 02/19] xen: arm: add missing break
Hi Ian,
Quoted 4 lines · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>xen: arm: Fix handling of ICC_{SGI1R,SGI0R,ASGI1R}_EL1
Quoted 8 lines
Having injected an undefined instruction we don't want to also advance pc. So return.
THe ICC_{SGI0R,ASGI1R}_EL1 case was previously missing a break, so would have fallen through to the default case and injected a second undef, corrupting
SPSR_EL1andELR_EL1for the guest.Signed-off-by Ian Campbell <[email protected]>
Regards,
Quoted 27 lines
--- xen/arch/arm/traps.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)xen/arch/arm/traps.c +2
-2@@ -1908,7 +1908,7 @@ static void do_sysreg(struct cpu_user_regs *regs, {dprintk (XENLOG_WARNING ,"failed emulation of sysreg ICC_SGI1R_EL1 access\n" ); -inject_undef64_exception (regs, hsr.len); +return inject_undef64_exception (regs, hsr.len); }break ;case HSR_SYSREG_ICC_SGI0R_EL1 :@@ -1916,7 +1916,7 @@ static void do_sysreg(struct cpu_user_regs *regs,/* TBD: Implement to support secure grp0/1 SGI forwarding */ dprintk (XENLOG_WARNING ,"Emulation of sysreg ICC_SGI0R_EL1/ASGI1R_EL1 not supported\n" ); -inject_undef64_exception (regs, hsr.len); +return inject_undef64_exception (regs, hsr.len);default : {const struct hsr_sysreg sysreg = hsr.sysreg;
Signature · 1 line
Julien Grall
Re: [PATCH 02/19] xen: arm: add missing break
Quoted 10 lines · Julien Grall
Hi Ian,
Quoted 7 lines · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>xen: arm: Fix handling of ICC_{SGI1R,SGI0R,ASGI1R}_EL1
Having injected an undefined instruction we don't want to also advance pc. So return.
Quoted 6 lines
Quoted 5 lines
THe ICC_{SGI0R,ASGI1R}_EL1 case was previously missing a break, so would have fallen through to the default case and injected a second undef, corrupting
SPSR_EL1andELR_EL1for the guest.Signed-off-by Ian Campbell <[email protected]>Reviewed-by Julien Grall <[email protected]>
Thanks. I seem to have two commit messages for this one, I think from a botched patch squash.
I've dropped the "add missing break" commit message, since I've actually written about it in the final paragraph. I've kept your R-by regardless, which I hope is ok.
Ian.
Re: [PATCH 02/19] xen: arm: add missing break
Hi Ian,
Quoted 16 lines · Ian Campbell
Quoted 15 lines · Julien Grall
Hi Ian,
Quoted 12 lines · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>xen: arm: Fix handling of ICC_{SGI1R,SGI0R,ASGI1R}_EL1
Having injected an undefined instruction we don't want to also advance pc. So return.
THe ICC_{SGI0R,ASGI1R}_EL1 case was previously missing a break, so would have fallen through to the default case and injected a second undef, corrupting
SPSR_EL1andELR_EL1for the guest.Signed-off-by Ian Campbell <[email protected]>
Quoted 8 lines
Quoted 1 line
Reviewed-by Julien Grall <[email protected]>Thanks. I seem to have two commit messages for this one, I think from a botched patch squash.
I've dropped the "add missing break" commit message, since I've actually written about it in the final paragraph. I've kept your R-by regardless, which I hope is ok.
I'm fine with the change.
Regards,
Signature · 1 line
Julien Grall
PATCH 3/19 xen: arm: call inject_undef_exception directly
In reply to Ian CampbellReducing the amount of goto maze considerably.
--- xen/arch/arm/traps.c | 56 +++++++++++++++++++++++--------------------------- 1 file changed, 26 insertions(+), 30 deletions(-)
xen/arch/arm/traps.c +26 -30
@@ -518,13 +518,13 @@ static void inject_iabt64_exception(struct cpu_user_regs *regs,#endif static void inject_undef_exception (struct cpu_user_regs *regs, -int instr_len) +const union hsr hsr) {if (is_32bit_domain (current->domain) )inject_undef32_exception (regs);#ifdef CONFIG_ARM_64 else -inject_undef64_exception (regs ,instr_len ); + inject_undef64_exception(regs, hsr.len);#endif }
@@ -1592,11 +1592,11 @@ static void do_cp15_32(struct cpu_user_regs *regs,case HSR_CPREG32 (CNTP_CTL ):case HSR_CPREG32 (CNTP_TVAL ):if ( !vtimer_emulate (regs, hsr) ) - goto undef_cp15_32; +return inject_undef_exception (regs, hsr);break ;case HSR_CPREG32 (ACTLR ):if (psr_mode_is_user (regs) ) - goto undef_cp15_32; +return inject_undef_exception (regs, hsr);if ( cp32.read ) *r = v->arch.actlr;break ;
@@ -1612,14 +1612,14 @@ static void do_cp15_32(struct cpu_user_regs *regs,case HSR_CPREG32 (PMUSERENR ):/* RO at EL0. RAZ/WI at EL1 */ if (psr_mode_is_user (regs) && !hsr.cp32.read ) - goto undef_cp15_32; +return inject_undef_exception (regs, hsr); goto cp15_32_raz_wi;case HSR_CPREG32 (PMINTENSET ):case HSR_CPREG32 (PMINTENCLR ):/* EL1 only, however MDCR_EL2.TPM==1 means EL0 may trap here also. */ if (psr_mode_is_user (regs) ) - goto undef_cp15_32; +return inject_undef_exception (regs, hsr); goto cp15_32_raz_wi;case HSR_CPREG32 (PMCR ):case HSR_CPREG32 (PMCNTENSET ):
@@ -1638,7 +1638,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,* emulate that register as 0 above. */ if (psr_mode_is_user (regs) ) - goto undef_cp15_32; +return inject_undef_exception (regs, hsr); cp15_32_raz_wi:if ( cp32.read ) *r =0 ;
@@ -1652,8 +1652,7 @@ static void do_cp15_32(struct cpu_user_regs *regs, cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2, regs->pc);gdprintk (XENLOG_ERR ,"unhandled 32-bit CP15 access %#x\n" , hsr.bits &HSR_CP32_REGS_MASK ); - undef_cp15_32: -inject_undef_exception (regs, hsr.len); +inject_undef_exception (regs, hsr);return ; }advance_pc (regs, hsr);
@@ -1673,7 +1672,7 @@ static void do_cp15_64(struct cpu_user_regs *regs,case HSR_CPREG64 (CNTPCT ):case HSR_CPREG64 (CNTP_CVAL ):if ( !vtimer_emulate (regs, hsr) ) - goto undef_cp15_64; +return inject_undef_exception (regs, hsr);break ;default : {
@@ -1685,8 +1684,7 @@ static void do_cp15_64(struct cpu_user_regs *regs, cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc);gdprintk (XENLOG_ERR ,"unhandled 64-bit CP15 access %#x\n" , hsr.bits &HSR_CP64_REGS_MASK ); - undef_cp15_64: -inject_undef_exception (regs, hsr.len); +inject_undef_exception (regs, hsr);return ; } }
@@ -1713,7 +1711,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)* is set to 0, which we emulated below. */ if ( !cp32.read ) - goto undef_cp14_32; +return inject_undef_exception (regs, hsr);/* Implement the minimum requirements: * - Number of watchpoints: 1
@@ -1731,14 +1729,14 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)* is set to 0, which we emulated below. */ if ( !cp32.read ) - goto undef_cp14_32; +return inject_undef_exception (regs, hsr); *r =0 ;break ;case HSR_CPREG32 (DBGDSCREXT ):if (usr_mode (regs) ) - goto undef_cp14_32; +return inject_undef_exception (regs, hsr);/* Implement debug status and control register as RAZ/WI. * The OS won't use Hardware debug if MDBGen not set
@@ -1756,7 +1754,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)case HSR_CPREG32 (DBGBCR1 ):case HSR_CPREG32 (DBGOSDLR ):if (usr_mode (regs) ) - goto undef_cp14_32; +return inject_undef_exception (regs, hsr);/* RAZ/WI */ if ( cp32.read ) *r =0 ;
@@ -1764,10 +1762,10 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)case HSR_CPREG32 (DBGOSLAR ):if (usr_mode (regs) ) - goto undef_cp14_32; +return inject_undef_exception (regs, hsr);/* WO */ if ( cp32.read ) - goto undef_cp14_32; +return inject_undef_exception (regs, hsr);/* else: ignore */ break ;default :
@@ -1777,8 +1775,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr) cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2, regs->pc);gdprintk (XENLOG_ERR ,"unhandled 32-bit cp14 access %#x\n" , hsr.bits &HSR_CP32_REGS_MASK ); - undef_cp14_32: -inject_undef_exception (regs, hsr.len); +inject_undef_exception (regs, hsr);return ; }
@@ -1802,7 +1799,7 @@ static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)gdprintk (XENLOG_ERR ,"unhandled 64-bit CP14 access %#x\n" , hsr.bits &HSR_CP64_REGS_MASK ); -inject_undef_exception (regs, hsr.len); +inject_undef_exception (regs, hsr); }static void do_cp (struct cpu_user_regs *regs,const union hsr hsr)
@@ -1817,7 +1814,7 @@ static void do_cp(struct cpu_user_regs *regs, const union hsr hsr)ASSERT (!cp.tas);/* We don't trap SIMD instruction */ gdprintk (XENLOG_ERR ,"unhandled CP%d access\n" , cp.coproc); -inject_undef_exception (regs, hsr.len); +inject_undef_exception (regs, hsr); }#ifdef CONFIG_ARM_64
@@ -1847,7 +1844,7 @@ static void do_sysreg(struct cpu_user_regs *regs,* undef. */ if (psr_mode_is_user (regs) ) - goto undef_sysreg; +return inject_undef_exception (regs, hsr); goto sysreg_raz_wi;case HSR_SYSREG_MDCCSR_EL0 :
@@ -1856,7 +1853,7 @@ static void do_sysreg(struct cpu_user_regs *regs,* register as RAZ/WI above. So RO at both EL0 and EL1. */ if ( !hsr.sysreg.read ) - goto undef_sysreg; +return inject_undef_exception (regs, hsr); *x =0 ;break ;
@@ -1865,7 +1862,7 @@ static void do_sysreg(struct cpu_user_regs *regs,case HSR_SYSREG_PMUSERENR_EL0 :/* RO at EL0. RAZ/WI at EL1 */ if (psr_mode_is_user (regs) && !hsr.sysreg.read ) - goto undef_sysreg; +return inject_undef_exception (regs, hsr); goto sysreg_raz_wi;case HSR_SYSREG_PMCR_EL0 :case HSR_SYSREG_PMCNTENSET_EL0 :
@@ -1884,7 +1881,7 @@ static void do_sysreg(struct cpu_user_regs *regs,* emulate that register as 0 above. */ if (psr_mode_is_user (regs) ) - goto undef_sysreg; +return inject_undef_exception (regs, hsr); sysreg_raz_wi:if ( hsr.sysreg.read ) *x =0 ;
@@ -1894,14 +1891,14 @@ static void do_sysreg(struct cpu_user_regs *regs,/* Write only, Write ignore registers: */ case HSR_SYSREG_OSLAR_EL1 :if ( hsr.sysreg.read ) - goto undef_sysreg; +return inject_undef_exception (regs, hsr);/* else: write ignored */ break ;case HSR_SYSREG_CNTP_CTL_EL0 :case HSR_SYSREG_CNTP_TVAL_EL0 :case HSR_SYSREG_CNTP_CVAL_EL0 :if ( !vtimer_emulate (regs, hsr) ) - goto undef_sysreg; +return inject_undef_exception (regs, hsr);break ;case HSR_SYSREG_ICC_SGI1R_EL1 :if ( !vgic_emulate (regs, hsr) )
@@ -1931,8 +1928,7 @@ static void do_sysreg(struct cpu_user_regs *regs, sysreg.reg, regs->pc);gdprintk (XENLOG_ERR ,"unhandled 64-bit sysreg access %#x\n" , hsr.bits &HSR_SYSREG_REGS_MASK ); - undef_sysreg: -inject_undef_exception (regs, hsr.sysreg.len); +inject_undef_exception (regs, hsr);return ; } }
Signature · 1 line
1.7.10.4
Re: [PATCH 03/19] xen: arm: call inject_undef_exception directly
Hi Ian,
Quoted 3 lines · Ian Campbell
Reducing the amount of goto maze considerably.
Signed-off-by Ian Campbell <[email protected]>
Regards,
Signature · 1 line
Julien Grall
PATCH 4/19 xen: arm: provide and use a handle_raz_wi helper
In reply to Ian CampbellReduces the use of goto in the trap handlers to none.
Some explcitily 32-bit types become register_t here, but that's OK, on 32-bit they are 32-bit already and on 64-bit it is fine/harmless to set the larger register, a 32-bit guest won't see the top half in any case.
Unlike the previous code the advancing of PC is handled within the helper, rather than after the end of the switch as before. So return as the handler is called.
--- xen/arch/arm/traps.c | 51 +++++++++++++++++++++++++------------------------- 1 file changed, 26 insertions(+), 25 deletions(-)
xen/arch/arm/traps.c +26 -25
@@ -1574,11 +1574,24 @@ static void advance_pc(struct cpu_user_regs *regs, const union hsr hsr) regs->pc += hsr.len ?4 :2 ; } +/* Read as zero + write ignore */ +static void handle_raz_wi (struct cpu_user_regs *regs, +register_t *reg, +bool_t read, +const union hsr hsr) +{ +if ( read ) + *reg =0 ; +/* else: write ignored */ + +advance_pc (regs, hsr); +} +static void do_cp15_32 (struct cpu_user_regs *regs,const union hsr hsr) {const struct hsr_cp32 cp32 = hsr.cp32; -uint32_t *r = (uint32_t *)select_user_reg (regs, cp32.reg); +register_t *r =select_user_reg (regs, cp32.reg);struct vcpu *v = current;if ( !check_conditional_instr (regs, hsr) )
@@ -1613,14 +1626,14 @@ static void do_cp15_32(struct cpu_user_regs *regs,/* RO at EL0. RAZ/WI at EL1 */ if (psr_mode_is_user (regs) && !hsr.cp32.read )return inject_undef_exception (regs, hsr); - goto cp15_32_raz_wi; +return handle_raz_wi (regs, r, cp32.read, hsr);case HSR_CPREG32 (PMINTENSET ):case HSR_CPREG32 (PMINTENCLR ):/* EL1 only, however MDCR_EL2.TPM==1 means EL0 may trap here also. */ if (psr_mode_is_user (regs) )return inject_undef_exception (regs, hsr); - goto cp15_32_raz_wi; +return handle_raz_wi (regs, r, cp32.read, hsr);case HSR_CPREG32 (PMCR ):case HSR_CPREG32 (PMCNTENSET ):case HSR_CPREG32 (PMCNTENCLR ):
@@ -1639,11 +1652,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,*/ if (psr_mode_is_user (regs) )return inject_undef_exception (regs, hsr); - cp15_32_raz_wi: -if ( cp32.read ) - *r =0 ; -/* else: write ignored */ -break ; +return handle_raz_wi (regs, r, cp32.read, hsr);default : gdprintk(XENLOG_ERR,
@@ -1694,7 +1703,7 @@ static void do_cp15_64(struct cpu_user_regs *regs,static void do_cp14_32 (struct cpu_user_regs *regs,const union hsr hsr) {const struct hsr_cp32 cp32 = hsr.cp32; -uint32_t *r = (uint32_t *)select_user_reg (regs, cp32.reg); +register_t *r =select_user_reg (regs, cp32.reg);struct domain *d = current->domain;if ( !check_conditional_instr (regs, hsr) )
@@ -1738,12 +1747,11 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)if (usr_mode (regs) )return inject_undef_exception (regs, hsr); -/* Implement debug status and control register as RAZ/WI. -* The OS won't use Hardware debug if MDBGen not set +/* +* Implement debug status and control register as RAZ/WI. +* The OS won't use Hardware debug if MDBGen not set. */ -if ( cp32.read ) - *r =0 ; -break ; +return handle_raz_wi (regs, r, cp32.read, hsr);case HSR_CPREG32 (DBGVCR ):case HSR_CPREG32 (DBGBVR0 ):
@@ -1755,10 +1763,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)case HSR_CPREG32 (DBGOSDLR ):if (usr_mode (regs) )return inject_undef_exception (regs, hsr); -/* RAZ/WI */ -if ( cp32.read ) - *r =0 ; -break ; +return handle_raz_wi (regs, r, cp32.read, hsr);case HSR_CPREG32 (DBGOSLAR ):if (usr_mode (regs) )
@@ -1845,7 +1850,7 @@ static void do_sysreg(struct cpu_user_regs *regs,*/ if (psr_mode_is_user (regs) )return inject_undef_exception (regs, hsr); - goto sysreg_raz_wi; +return handle_raz_wi (regs, x, hsr.sysreg.read, hsr);case HSR_SYSREG_MDCCSR_EL0 :/*
@@ -1863,7 +1868,7 @@ static void do_sysreg(struct cpu_user_regs *regs,/* RO at EL0. RAZ/WI at EL1 */ if (psr_mode_is_user (regs) && !hsr.sysreg.read )return inject_undef_exception (regs, hsr); - goto sysreg_raz_wi; +return handle_raz_wi (regs, x, hsr.sysreg.read, hsr);case HSR_SYSREG_PMCR_EL0 :case HSR_SYSREG_PMCNTENSET_EL0 :case HSR_SYSREG_PMCNTENCLR_EL0 :
@@ -1882,11 +1887,7 @@ static void do_sysreg(struct cpu_user_regs *regs,*/ if (psr_mode_is_user (regs) )return inject_undef_exception (regs, hsr); - sysreg_raz_wi: -if ( hsr.sysreg.read ) - *x =0 ; -/* else: write ignored */ -break ; +return handle_raz_wi (regs, x, hsr.sysreg.read, hsr);/* Write only, Write ignore registers: */ case HSR_SYSREG_OSLAR_EL1 :
Signature · 1 line
1.7.10.4
Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper
Hi Ian,
Quoted 3 lines · Ian Campbell
Reduces the use of goto in the trap handlers to none.
Some explcitily 32-bit types become
register_there, but that's OK, on
s/explcitily/explicitly/
Quoted 3 lines
32-bit they are 32-bit already and on 64-bit it is fine/harmless to set the larger register, a 32-bit guest won't see the top half in any case.
What about 32-bit userspace on 64-bit kernel? Are we sure that a guest kernel won't only save the bottom half of the register?
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper
Quoted 7 lines · Julien Grall
Hi Ian,
Quoted 3 lines · Ian Campbell
Reduces the use of goto in the trap handlers to none.
Some explcitily 32-bit types become
register_there, but that's OK, on
Quoted 8 lines
s/explcitily/explicitly/
Quoted 3 lines
32-bit they are 32-bit already and on 64-bit it is fine/harmless to set the larger register, a 32-bit guest won't see the top half in any case.
What about 32-bit userspace on 64-bit kernel? Are we sure that a guest kernel won't only save the bottom half of the register?
That would be fine, since the userspace couldn't see the top half anyway so not saving it doesn't hurt.
In any case, the trap here has been talking from 32-bit mode and that is where we will return, so I'm not sure the guest kernel enters the picture, does it?
Ian.
Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper
Quoted 17 lines · Ian Campbell
Quoted 7 lines · Julien Grall
Hi Ian,
Quoted 3 lines · Ian Campbell
Reduces the use of goto in the trap handlers to none.
Some explcitily 32-bit types become
register_there, but that's OK, onQuoted 8 lines
s/explcitily/explicitly/
Quoted 3 lines
32-bit they are 32-bit already and on 64-bit it is fine/harmless to set the larger register, a 32-bit guest won't see the top half in any case.
What about 32-bit userspace on 64-bit kernel? Are we sure that a guest kernel won't only save the bottom half of the register?
Quoted 6 lines
That would be fine, since the userspace couldn't see the top half anyway so not saving it doesn't hurt.
In any case, the trap here has been talking from 32-bit mode and that is where we will return, so I'm not sure the guest kernel enters the picture, does it?
It's possible for the kernel to access only a part of the 64 bit registers and preserve the other part with a valid data to use later.
AFAICT, nothing prevent a guest to use the top half of the registers for his own purpose. It would only need to save/restore the bottom half of a 64 bit registers.
By resetting the 64-bit register, we will corrupt the top half of the registers and potentially (if the use case is valid) crash the kernel.
Although I didn't say that I would write a such guest ;)
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper
Quoted 26 lines · Julien Grall
Quoted 17 lines · Ian Campbell
Quoted 7 lines · Julien Grall
Hi Ian,
Quoted 3 lines · Ian Campbell
Reduces the use of goto in the trap handlers to none.
Some explcitily 32-bit types become
register_there, but that's OK, onQuoted 8 lines
s/explcitily/explicitly/
Quoted 3 lines
32-bit they are 32-bit already and on 64-bit it is fine/harmless to set the larger register, a 32-bit guest won't see the top half in any case.
What about 32-bit userspace on 64-bit kernel? Are we sure that a guest kernel won't only save the bottom half of the register?
Quoted 6 lines
That would be fine, since the userspace couldn't see the top half anyway so not saving it doesn't hurt.
In any case, the trap here has been talking from 32-bit mode and that is where we will return, so I'm not sure the guest kernel enters the picture, does it?
Quoted 6 lines
It's possible for the kernel to access only a part of the 64 bit registers and preserve the other part with a valid data to use later.
AFAICT, nothing prevent a guest to use the top half of the registers for his own purpose. It would only need to save/restore the bottom half of a 64 bit registers.
Writing to the bottom half (e.g. w0) of a register implicitly clears the top half, IIRC, so I think a kernel is unlikely to want to do this, even if it could (which I'm not quite convinced of).
Quoted 8 lines
By resetting the 64-bit register, we will corrupt the top half of the registers and potentially (if the use case is valid) crash the kernel.
Although I didn't say that I would write a such guest ;)
Regards,
Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper
Quoted 3 lines · Ian Campbell
Writing to the bottom half (e.g. w0) of a register implicitly clears the top half, IIRC, so I think a kernel is unlikely to want to do this, even if it could (which I'm not quite convinced of).
That said, I'll see if I can make something work with the handle_* taking the reg number instead of a pointer and calling select_user_reg in each.
Ian.
Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper
Quoted 9 lines · Ian Campbell
Quoted 3 lines · Ian Campbell
Writing to the bottom half (e.g. w0) of a register implicitly clears the top half, IIRC, so I think a kernel is unlikely to want to do this, even if it could (which I'm not quite convinced of).
That said, I'll see if I can make something work with the handle_* taking the reg number instead of a pointer and calling
select_user_regin each.
Actually don't even need that, I think the following does what is needed. I'm not 100% convinced it is needed though, but it's simple enough, and I can't find anything in the ARM ARM right now which rules out what you are suggesting, even if it is unlikely.
Ian.
xen/arch/arm/traps.c +14 -2
@@ -1585,7 +1585,14 @@ static void handle_raz_wi(struct cpu_user_regs *regs,return inject_undef_exception (regs, hsr);if ( read ) - *reg =0 ; + { +#ifdef CONFIG_ARM_64 +if (psr_mode_is_32bit (regs->cpsr) ) + *reg &= ~0xffffffffUL ; + else +#endif + *reg =0 ; + }/* else: write ignored */ advance_pc (regs, hsr);
@@ -1622,7 +1629,12 @@ static void handle_ro_raz(struct cpu_user_regs *regs,return inject_undef_exception (regs, hsr);/* else: raz */ - *reg =0 ; +#ifdef CONFIG_ARM_64 +if (psr_mode_is_32bit (regs->cpsr) ) + *reg &= ~0xffffffffUL ; + else +#endif + *reg =0 ;advance_pc (regs, hsr); }
Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper
Quoted 7 lines · Ian Campbell
Quoted 6 lines · Ian Campbell
Quoted 3 lines · Ian Campbell
Writing to the bottom half (e.g. w0) of a register implicitly clears the top half, IIRC, so I think a kernel is unlikely to want to do this, even if it could (which I'm not quite convinced of).
Quoted 8 lines
Quoted 3 lines
That said, I'll see if I can make something work with the handle_* taking the reg number instead of a pointer and calling
select_user_regin each.Actually don't even need that, I think the following does what is needed. I'm not 100% convinced it is needed though, but it's simple enough, and I can't find anything in the ARM ARM right now which rules out what you are suggesting, even if it is unlikely.
The paragraph "Pseudocode description of registers in AArch64 state" in section B1.2.1 (ARMv8 DDI0487 A.d) confirms your previous mail. I.e "writing to the bottom half (e.g. w0) of a register implicitly clears the top half".
I think it may be worth to mention the paragraph somewhere in the patch.
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper
Quoted 18 lines · Julien Grall
Quoted 7 lines · Ian Campbell
Quoted 6 lines · Ian Campbell
Quoted 3 lines · Ian Campbell
Writing to the bottom half (e.g. w0) of a register implicitly clears the top half, IIRC, so I think a kernel is unlikely to want to do this, even if it could (which I'm not quite convinced of).
Quoted 8 lines
Quoted 3 lines
That said, I'll see if I can make something work with the handle_* taking the reg number instead of a pointer and calling
select_user_regin each.Actually don't even need that, I think the following does what is needed. I'm not 100% convinced it is needed though, but it's simple enough, and I can't find anything in the ARM ARM right now which rules out what you are suggesting, even if it is unlikely.
Quoted 6 lines
The paragraph "Pseudocode description of registers in AArch64 state" in section B1.2.1 (ARMv8 DDI0487 A.d) confirms your previous mail. I.e "writing to the bottom half (e.g. w0) of a register implicitly clears the top half".
I think it may be worth to mention the paragraph somewhere in the patch.
Yes, I shall, in the commit log most likely.
Ian.
PATCH 5/19 xen: arm: Add and use r/o+raz and w/o+wi helpers
In reply to Ian Campbell--- xen/arch/arm/traps.c | 52 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 19 deletions(-)
xen/arch/arm/traps.c +33 -19
@@ -1587,6 +1587,34 @@ static void handle_raz_wi(struct cpu_user_regs *regs,advance_pc (regs, hsr); } +/* Write only + write ignore */ +static void handle_wo_wi (struct cpu_user_regs *regs, +register_t *reg, +bool_t read, +const union hsr hsr) +{ +if ( read ) +return inject_undef_exception (regs, hsr); +/* else: ignore */ + +advance_pc (regs, hsr); +} + +/* Read only + read as zero */ +static void handle_ro_raz (struct cpu_user_regs *regs, +register_t *reg, +bool_t read, +const union hsr hsr) +{ +if ( !read ) +return inject_undef_exception (regs, hsr); +/* else: raz */ + + *reg =0 ; + +advance_pc (regs, hsr); +} +static void do_cp15_32 (struct cpu_user_regs *regs,const union hsr hsr) {
@@ -1737,11 +1765,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)* Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis * is set to 0, which we emulated below. */ -if ( !cp32.read ) -return inject_undef_exception (regs, hsr); - - *r =0 ; -break ; +return handle_ro_raz (regs, r, cp32.read, hsr,1 );case HSR_CPREG32 (DBGDSCREXT ):if (usr_mode (regs) )
@@ -1768,11 +1792,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)case HSR_CPREG32 (DBGOSLAR ):if (usr_mode (regs) )return inject_undef_exception (regs, hsr); -/* WO */ -if ( cp32.read ) -return inject_undef_exception (regs, hsr); -/* else: ignore */ -break ; +return handle_wo_wi (regs, r, cp32.read, hsr);default : gdprintk(XENLOG_ERR, "%s p14, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
@@ -1857,11 +1877,7 @@ static void do_sysreg(struct cpu_user_regs *regs,* Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate that * register as RAZ/WI above. So RO at both EL0 and EL1. */ -if ( !hsr.sysreg.read ) -return inject_undef_exception (regs, hsr); - - *x =0 ; -break ; +return handle_ro_raz (regs, x, hsr.sysreg.read, hsr);/* - Perf monitors */ case HSR_SYSREG_PMUSERENR_EL0 :
@@ -1891,10 +1907,8 @@ static void do_sysreg(struct cpu_user_regs *regs,/* Write only, Write ignore registers: */ case HSR_SYSREG_OSLAR_EL1 : -if ( hsr.sysreg.read ) -return inject_undef_exception (regs, hsr); -/* else: write ignored */ -break ; +return handle_wo_wi (regs, x, hsr.sysreg.read, hsr); +case HSR_SYSREG_CNTP_CTL_EL0 :case HSR_SYSREG_CNTP_TVAL_EL0 :case HSR_SYSREG_CNTP_CVAL_EL0 :
Signature · 1 line
1.7.10.4
Re: [PATCH 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers
Hi Ian,
Quoted 10 lines · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 52 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 19 deletions(-)xen/arch/arm/traps.c +0
-0
xen/arch/arm/traps.c static void handle_raz_wi(struct cpu_user_regs *regs, · line 1592
Quoted 6 lines
xen/arch/arm/traps.c +3
-0@@ -1587,6 +1587,34 @@ static void handle_raz_wi(struct cpu_user_regs *regs,advance_pc (regs, hsr); } +/* Write only + write ignore */ +static void handle_wo_wi(struct cpu_user_regs *regs, + register_t *reg,
This helper handles WO and WI which doesn't require to modify the register.
I would pass a register_t rather than register_t* in order to make clear that the register of the guest won't change.
xen/arch/arm/traps.c static void handle_raz_wi(struct cpu_user_regs *regs, · line 1603
Quoted 11 lines
xen/arch/arm/traps.c +11
-0+bool_t read, + const union hsr hsr) +{ + if ( read ) + return inject_undef_exception(regs, hsr); +/* else: ignore */ + +advance_pc (regs ,hsr ); +} + +/* Read only + read as zero */
This comment may confuse developer who wants to implement RO register which another value than 0.
I got confuse too. It would be nice to expand the comment for the RO case.
Quoted 18 lines
xen/arch/arm/traps.c +14
-0+static void handle_ro_raz (struct cpu_user_regs *regs, +register_t *reg, +bool_t read, +const union hsr hsr) +{ +if ( !read ) +return inject_undef_exception (regs, hsr); +/* else: raz */ + + *reg =0 ; + +advance_pc (regs, hsr); +} +static void do_cp15_32 (struct cpu_user_regs *regs,const union hsr hsr) {
xen/arch/arm/traps.c static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr) · line 1768
Quoted 9 lines
xen/arch/arm/traps.c +1
-5@@ -1737,11 +1765,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)* Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis * is set to 0, which we emulated below. */ -if ( !cp32.read ) -return inject_undef_exception (regs, hsr); - - *r =0 ; -break ; +return handle_ro_raz (regs, r, cp32.read, hsr,1 );
The function call has too many argument.
I guess the last argument is the minimum level exception, we should be part of the next patch (#6).
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers
Quoted 16 lines · Julien Grall
Hi Ian,
Quoted 13 lines · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 52 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 19 deletions(-)xen/arch/arm/traps.c +0
-0@@ -1587,6 +1587,34 @@ static void handle_raz_wi(struct cpu_user_regs *regs,advance_pc (regs, hsr); }
Quoted 8 lines
xen/arch/arm/traps.c static void handle_raz_wi(struct cpu_user_regs *regs, · line 1592
Quoted 3 lines
xen/arch/arm/traps.c +3
-0+/* Write only + write ignore */ +static void handle_wo_wi(struct cpu_user_regs *regs, + register_t *reg,This helper handles WO and WI which doesn't require to modify the register.
I would pass a
register_trather than register_t* in order to make clear that the register of the guest won't change.
I deliberately made handle_* all have the same prototype for consistency.
Quoted 6 lines
xen/arch/arm/traps.c static void handle_raz_wi(struct cpu_user_regs *regs, · line 1603
Quoted 1 line
xen/arch/arm/traps.c +1
-0+/* Read only + read as zero */ This comment may confuse developer who wants to implement RO register which another value than 0.
I got confuse too. It would be nice to expand the comment for the RO case.
I'm afraid I don't understand the confusion so I'm not sure how to clarify. What did you think this comment was saying?
Quoted 6 lines
xen/arch/arm/traps.c static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr) · line 1768
Quoted 1 line
xen/arch/arm/traps.c +1
-0+return handle_ro_raz (regs, r, cp32.read, hsr,1 );The function call has too many argument.
I guess the last argument is the minimum level exception, we should be part of the next patch (#6).
Fixed, thanks.
Ian.
Re: [PATCH 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers
Hi Ian,
Quoted 21 lines · Ian Campbell
Quoted 20 lines · Julien Grall
Quoted 10 lines · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 52 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 19 deletions(-)xen/arch/arm/traps.c +0
-0xen/arch/arm/traps.c static void handle_raz_wi(struct cpu_user_regs *regs, · line 1592
Quoted 6 lines
xen/arch/arm/traps.c +3
-0@@ -1587,6 +1587,34 @@ static void handle_raz_wi(struct cpu_user_regs *regs,advance_pc (regs, hsr); } +/* Write only + write ignore */ +static void handle_wo_wi(struct cpu_user_regs *regs, + register_t *reg,This helper handles WO and WI which doesn't require to modify the register.
Quoted 5 lines
Quoted 2 lines
I would pass a
register_trather than register_t* in order to make clear that the register of the guest won't change.I deliberately made handle_* all have the same prototype for consistency.
Ok.
Quoted 9 lines
Quoted 6 lines
xen/arch/arm/traps.c static void handle_raz_wi(struct cpu_user_regs *regs, · line 1603
Quoted 1 line
xen/arch/arm/traps.c +1
-0+/* Read only + read as zero */ This comment may confuse developer who wants to implement RO register which another value than 0.
I got confuse too. It would be nice to expand the comment for the RO case.
I'm afraid I don't understand the confusion so I'm not sure how to clarify. What did you think this comment was saying?
When I read the comment I though you were implemented two distinct part:
RO and RAZ.
As the value return by RO may not always be 0 (we have a handful of cases in traps.c), I didn't understand why you were implementing both within the same helper.
Although this helper choose to implement RO as RAZ. So I think it would be good to mention it in order to avoid confusion later.
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers
Quoted 5 lines · Julien Grall
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
+/* Read only + read as zero */
This comment may confuse developer who wants to implement RO register which another value than 0.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
I got confuse too. It would be nice to expand the comment for the RO case.
I'm afraid I don't understand the confusion so I'm not sure how to clarify. What did you think this comment was saying?
When I read the comment I though you were implemented two distinct part:
RO and RAZ.
Well, in a sense I am ;-)
Quoted 6 lines
As the value return by RO may not always be 0 (we have a handful of cases in
traps.c), I didn't understand why you were implementing both within the same helper.Although this helper choose to implement RO as RAZ. So I think it would be good to mention it in order to avoid confusion later.
Perhaps rather than:
/* Read as zero + write ignore */
/* Write only + write ignore */
/* Read only + read as zero */
this would be clearer:
/* Read as zero and write ignore */
/* Write only as write ignore */
/* Read only as read as zero */
?
IOW I suspect it is the + which has confused you, especially since it is used differently in the first vs the second two.
Ian.
PATCH 6/19 xen: arm: add minimum exception level argument to trap handler helpers
In reply to Ian CampbellRemoves a load of boiler plate.
--- xen/arch/arm/traps.c | 65 +++++++++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 33 deletions(-)
xen/arch/arm/traps.c +32 -33
@@ -1578,8 +1578,12 @@ static void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)static void handle_raz_wi (struct cpu_user_regs *regs,register_t *reg,bool_t read, -const union hsr hsr) +const union hsr hsr, +int min_el) { +if ( min_el >0 &&psr_mode_is_user (regs) ) +return inject_undef_exception (regs, hsr); +if ( read ) *reg =0 ;/* else: write ignored */
@@ -1591,8 +1595,12 @@ static void handle_raz_wi(struct cpu_user_regs *regs,static void handle_wo_wi (struct cpu_user_regs *regs,register_t *reg,bool_t read, -const union hsr hsr) +const union hsr hsr, +int min_el) { +if ( min_el >0 &&psr_mode_is_user (regs) ) +return inject_undef_exception (regs, hsr); +if ( read )return inject_undef_exception (regs, hsr);/* else: ignore */
@@ -1604,8 +1612,12 @@ static void handle_wo_wi(struct cpu_user_regs *regs,static void handle_ro_raz (struct cpu_user_regs *regs,register_t *reg,bool_t read, -const union hsr hsr) +const union hsr hsr, +int min_el) { +if ( min_el >0 &&psr_mode_is_user (regs) ) +return inject_undef_exception (regs, hsr); +if ( !read )return inject_undef_exception (regs, hsr);/* else: raz */
@@ -1652,16 +1664,15 @@ static void do_cp15_32(struct cpu_user_regs *regs,*/ case HSR_CPREG32 (PMUSERENR ):/* RO at EL0. RAZ/WI at EL1 */ -if (psr_mode_is_user (regs) && !hsr.cp32.read ) -return inject_undef_exception (regs, hsr); -return handle_raz_wi (regs, r, cp32.read, hsr); +if (psr_mode_is_user (regs) ) +return handle_ro_raz (regs, r, cp32.read, hsr,0 ); +else +return handle_raz_wi (regs, r, cp32.read, hsr,1 );case HSR_CPREG32 (PMINTENSET ):case HSR_CPREG32 (PMINTENCLR ):/* EL1 only, however MDCR_EL2.TPM==1 means EL0 may trap here also. */ -if (psr_mode_is_user (regs) ) -return inject_undef_exception (regs, hsr); -return handle_raz_wi (regs, r, cp32.read, hsr); +return handle_raz_wi (regs, r, cp32.read, hsr,1 );case HSR_CPREG32 (PMCR ):case HSR_CPREG32 (PMCNTENSET ):case HSR_CPREG32 (PMCNTENCLR ):
@@ -1678,9 +1689,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,* Accessible at EL0 only if PMUSERENR_EL0.EN is set. We * emulate that register as 0 above. */ -if (psr_mode_is_user (regs) ) -return inject_undef_exception (regs, hsr); -return handle_raz_wi (regs, r, cp32.read, hsr); +return handle_raz_wi (regs, r, cp32.read, hsr,1 );default : gdprintk(XENLOG_ERR,
@@ -1768,14 +1777,11 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)return handle_ro_raz (regs, r, cp32.read, hsr,1 );case HSR_CPREG32 (DBGDSCREXT ): -if (usr_mode (regs) ) -return inject_undef_exception (regs, hsr); -/* * Implement debug status and control register as RAZ/WI. * The OS won't use Hardware debug if MDBGen not set. */ -return handle_raz_wi (regs, r, cp32.read, hsr); +return handle_raz_wi (regs, r, cp32.read, hsr,1 );case HSR_CPREG32 (DBGVCR ):case HSR_CPREG32 (DBGBVR0 ):
@@ -1785,14 +1791,10 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)case HSR_CPREG32 (DBGBVR1 ):case HSR_CPREG32 (DBGBCR1 ):case HSR_CPREG32 (DBGOSDLR ): -if (usr_mode (regs) ) -return inject_undef_exception (regs, hsr); -return handle_raz_wi (regs, r, cp32.read, hsr); +return handle_raz_wi (regs, r, cp32.read, hsr,1 );case HSR_CPREG32 (DBGOSLAR ): -if (usr_mode (regs) ) -return inject_undef_exception (regs, hsr); -return handle_wo_wi (regs, r, cp32.read, hsr); +return handle_wo_wi (regs, r, cp32.read, hsr,1 );default : gdprintk(XENLOG_ERR, "%s p14, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
@@ -1868,23 +1870,22 @@ static void do_sysreg(struct cpu_user_regs *regs,* Accessible from EL1 only, but if EL0 trap happens handle as * undef. */ -if (psr_mode_is_user (regs) ) -return inject_undef_exception (regs, hsr); -return handle_raz_wi (regs, x, hsr.sysreg.read, hsr); +return handle_raz_wi (regs, x, hsr.sysreg.read, hsr,1 );case HSR_SYSREG_MDCCSR_EL0 :/* * Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate that * register as RAZ/WI above. So RO at both EL0 and EL1. */ -return handle_ro_raz (regs, x, hsr.sysreg.read, hsr); +return handle_ro_raz (regs, x, hsr.sysreg.read, hsr,0 );/* - Perf monitors */ case HSR_SYSREG_PMUSERENR_EL0 :/* RO at EL0. RAZ/WI at EL1 */ -if (psr_mode_is_user (regs) && !hsr.sysreg.read ) -return inject_undef_exception (regs, hsr); -return handle_raz_wi (regs, x, hsr.sysreg.read, hsr); +if (psr_mode_is_user (regs) ) +return handle_ro_raz (regs, x, hsr.sysreg.read, hsr,0 ); +else +return handle_raz_wi (regs, x, hsr.sysreg.read, hsr,1 );case HSR_SYSREG_PMCR_EL0 :case HSR_SYSREG_PMCNTENSET_EL0 :case HSR_SYSREG_PMCNTENCLR_EL0 :
@@ -1901,13 +1902,11 @@ static void do_sysreg(struct cpu_user_regs *regs,* Accessible at EL0 only if PMUSERENR_EL0.EN is set. We * emulate that register as 0 above. */ -if (psr_mode_is_user (regs) ) -return inject_undef_exception (regs, hsr); -return handle_raz_wi (regs, x, hsr.sysreg.read, hsr); +return handle_raz_wi (regs, x, hsr.sysreg.read, hsr,1 );/* Write only, Write ignore registers: */ case HSR_SYSREG_OSLAR_EL1 : -return handle_wo_wi (regs, x, hsr.sysreg.read, hsr); +return handle_wo_wi (regs, x, hsr.sysreg.read, hsr,1 );case HSR_SYSREG_CNTP_CTL_EL0 :case HSR_SYSREG_CNTP_TVAL_EL0 :
Signature · 1 line
1.7.10.4
Re: [PATCH 06/19] xen: arm: add minimum exception level argument to trap handler helpers
Hi Ian,
Quoted 12 lines · Ian Campbell
Removes a load of boiler plate.
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 65 +++++++++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 33 deletions(-)xen/arch/arm/traps.c +0
-0
xen/arch/arm/traps.c static void advance_pc(struct cpu_user_regs *regs, const union hsr hsr) · line 1583
Quoted 7 lines
xen/arch/arm/traps.c +2
-1@@ -1578,8 +1578,12 @@ static void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)static void handle_raz_wi (struct cpu_user_regs *regs,register_t *reg,bool_t read, -const union hsr hsr) +const union hsr hsr, +int min_el) {
I would add an ASSERT((min_el == 0) || (min_el == 1)) in order to make clear that min_el should be either EL0 or EL1.
Quoted 7 lines
xen/arch/arm/traps.c +3
-0+if ( min_el >0 &&psr_mode_is_user (regs) ) +return inject_undef_exception (regs, hsr); +if ( read ) *reg =0 ;/* else: write ignored */
xen/arch/arm/traps.c static void handle_raz_wi(struct cpu_user_regs *regs, · line 1600
Quoted 7 lines
xen/arch/arm/traps.c +2
-1@@ -1591,8 +1595,12 @@ static void handle_raz_wi(struct cpu_user_regs *regs,static void handle_wo_wi (struct cpu_user_regs *regs,register_t *reg,bool_t read, -const union hsr hsr) +const union hsr hsr, +int min_el) {
Ditto
Quoted 7 lines
xen/arch/arm/traps.c +3
-0+if ( min_el >0 &&psr_mode_is_user (regs) ) +return inject_undef_exception (regs, hsr); +if ( read )return inject_undef_exception (regs, hsr);/* else: ignore */
xen/arch/arm/traps.c static void handle_wo_wi(struct cpu_user_regs *regs, · line 1617
Quoted 7 lines
xen/arch/arm/traps.c +2
-1@@ -1604,8 +1612,12 @@ static void handle_wo_wi(struct cpu_user_regs *regs,static void handle_ro_raz (struct cpu_user_regs *regs,register_t *reg,bool_t read, -const union hsr hsr) +const union hsr hsr, +int min_el) {
Ditto
xen/arch/arm/traps.c static void handle_wo_wi(struct cpu_user_regs *regs, · line 1623
Quoted 6 lines
xen/arch/arm/traps.c +3
-0+if ( min_el >0 &&psr_mode_is_user (regs) ) +return inject_undef_exception (regs, hsr); +if ( !read )return inject_undef_exception (regs, hsr);/* else: raz */
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH 06/19] xen: arm: add minimum exception level argument to trap handler helpers
Quoted 10 lines · Julien Grall
Hi Ian,
Quoted 7 lines · Ian Campbell
Removes a load of boiler plate.
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 65 +++++++++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 33 deletions(-)
Quoted 15 lines
xen/arch/arm/traps.c static void advance_pc(struct cpu_user_regs *regs, const union hsr hsr) · line 1583
Quoted 12 lines
xen/arch/arm/traps.c +2
-1@@ -1578,8 +1578,12 @@ static void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)static void handle_raz_wi (struct cpu_user_regs *regs,register_t *reg,bool_t read, -const union hsr hsr) +const union hsr hsr, +int min_el) {I would add an
ASSERT((min_el == 0) || (min_el == 1))in order to make clear thatmin_elshould be either EL0 or EL1.
Done for all cases, thanks.
PATCH 7/19 xen: arm: Annotate trap handler for HCR_EL2.{TWI, TWE, TSC}
In reply to Ian CampbellReference the bit which enables the trap and the section/page which describes what that bit enables.
These ones are pretty trivial, included for completeness.
--- xen/arch/arm/traps.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
xen/arch/arm/traps.c +17 -0
@@ -2083,6 +2083,12 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)switch (hsr.ec) {case HSR_EC_WFI_WFE : +/* +* HSR_EL2.TWI, HSR_EL2.TWE +* +* ARMv7 (DDI 0406C.b): B1.14.9 +* ARMv8 (DDI 0487A.d): D1-1505 Table D1-51 +*/ if ( !check_conditional_instr (regs, hsr) ) {advance_pc (regs, hsr);
@@ -2125,6 +2131,12 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)do_cp (regs, hsr);break ;case HSR_EC_SMC32 : +/* +* HSR_EL2.TSC +* +* ARMv7 (DDI 0406C.b): B1.14.8 +* ARMv8 (DDI 0487A.d): D1-1501 Table D1-44 +*/ GUEST_BUG_ON (!psr_mode_is_32bit (regs->cpsr));perfc_incr (trap_smc32);inject_undef32_exception (regs);
@@ -2153,6 +2165,11 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)do_trap_hypercall (regs, ®s->x16, hsr.iss);break ;case HSR_EC_SMC64 : +/* +* HSR_EL2.TSC +* +* ARMv8 (DDI 0487A.d): D1-1501 Table D1-44 +*/ GUEST_BUG_ON (psr_mode_is_32bit (regs->cpsr));perfc_incr (trap_smc64);inject_undef64_exception (regs, hsr.len);
Signature · 1 line
1.7.10.4
Re: [PATCH 07/19] xen: arm: Annotate trap handler for HCR_EL2.{TWI, TWE, TSC}
Hi Ian,
Quoted 15 lines · Ian Campbell
Reference the bit which enables the trap and the section/page which describes what that bit enables.
These ones are pretty trivial, included for completeness.
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)xen/arch/arm/traps.c +0
-0
xen/arch/arm/traps.c asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) · line 2087
Quoted 5 lines
xen/arch/arm/traps.c +2
-0@@ -2083,6 +2083,12 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)switch (hsr.ec) {case HSR_EC_WFI_WFE : +/* +* HSR_EL2.TWI, HSR_EL2.TWE
Typo: I should be HCR not HSR.
xen/arch/arm/traps.c asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) · line 2090
Quoted 3 lines
xen/arch/arm/traps.c +3
-0+* +* ARMv7 (DDI 0406C.b): B1.14.9 +* ARMv8 (DDI 0487A.d): D1-1505 Table D1-51
It's a bit confusing that you are using section for ARMv7 and page for ARMv8.
xen/arch/arm/traps.c asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) · line 2135
Quoted 10 lines
xen/arch/arm/traps.c +3
-0+*/ if ( !check_conditional_instr (regs, hsr) ) {advance_pc (regs, hsr);@@ -2125,6 +2131,12 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)do_cp (regs, hsr);break ;case HSR_EC_SMC32 : +/* +* HSR_EL2.TSC
HCR
xen/arch/arm/traps.c asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) · line 2169
Quoted 13 lines
xen/arch/arm/traps.c +6
-0+* +* ARMv7 (DDI 0406C.b): B1.14.8 +* ARMv8 (DDI 0487A.d): D1-1501 Table D1-44 +*/ GUEST_BUG_ON (!psr_mode_is_32bit (regs->cpsr));perfc_incr (trap_smc32);inject_undef32_exception (regs);@@ -2153,6 +2165,11 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)do_trap_hypercall (regs, ®s->x16, hsr.iss);break ;case HSR_EC_SMC64 : +/* +* HSR_EL2.TSC
HCR
xen/arch/arm/traps.c asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) · line 2175
Quoted 7 lines
xen/arch/arm/traps.c +3
-0+* +* ARMv8 (DDI 0487A.d): D1-1501 Table D1-44 +*/ GUEST_BUG_ON (psr_mode_is_32bit (regs->cpsr));perfc_incr (trap_smc64);inject_undef64_exception (regs, hsr.len);
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH 07/19] xen: arm: Annotate trap handler for HCR_EL2.{TWI, TWE, TSC}
Quoted 19 lines · Julien Grall
Hi Ian,
Quoted 16 lines · Ian Campbell
Reference the bit which enables the trap and the section/page which describes what that bit enables.
These ones are pretty trivial, included for completeness.
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)xen/arch/arm/traps.c +0
-0@@ -2083,6 +2083,12 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
Quoted 6 lines
xen/arch/arm/traps.c asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) · line 2087
Quoted 4 lines
xen/arch/arm/traps.c +2
-0switch (hsr.ec) {case HSR_EC_WFI_WFE : +/* +* HSR_EL2.TWI, HSR_EL2.TWE Typo: I should be HCR not HSR.
Gah, at least I was consistent pretty much throughout!
Quoted 7 lines
xen/arch/arm/traps.c asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) · line 2090
Quoted 4 lines
xen/arch/arm/traps.c +3
-0+* +* ARMv7 (DDI 0406C.b): B1.14.9 +* ARMv8 (DDI 0487A.d): D1-1505 Table D1-51 It's a bit confusing that you are using section for ARMv7 and page for ARMv8.
I wanted to use section numbers everywhere, since in the ARMv7 ARM they are nice and precise. But the ARMv8 ARM doesn't really use them in the same way.
In this case D1-1505 is in section D1.15.3 which starts on page D1-1495 and covers the whole of HCR_EL2, which isn't a useful reference since it would be the same for 2/3 of the patches (AKA bits in HCR_EL2) in this series. D1-1505 isn't perfect since there is other stuff on that page, but it's as close I can get.
Bit in the ARMv7 ARM B1.14.9 is exactly about these two traps.
Part of my goal here is to make it so that people (especially me) don't spend half their life searching for the doc associated with a given HCR_EL2 trap bit.
Ian.
Re: [PATCH 07/19] xen: arm: Annotate trap handler for HCR_EL2.{TWI, TWE, TSC}
Hi Ian,
Quoted 41 lines · Ian Campbell
Quoted 19 lines · Julien Grall
Hi Ian,
Quoted 16 lines · Ian Campbell
Reference the bit which enables the trap and the section/page which describes what that bit enables.
These ones are pretty trivial, included for completeness.
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)xen/arch/arm/traps.c +0
-0@@ -2083,6 +2083,12 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)Quoted 6 lines
xen/arch/arm/traps.c asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) · line 2087
Quoted 4 lines
xen/arch/arm/traps.c +2
-0switch (hsr.ec) {case HSR_EC_WFI_WFE : +/* +* HSR_EL2.TWI, HSR_EL2.TWE Typo: I should be HCR not HSR.
Gah, at least I was consistent pretty much throughout!
Quoted 7 lines
xen/arch/arm/traps.c asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) · line 2090
Quoted 4 lines
xen/arch/arm/traps.c +3
-0+* +* ARMv7 (DDI 0406C.b): B1.14.9 +* ARMv8 (DDI 0487A.d): D1-1505 Table D1-51 It's a bit confusing that you are using section for ARMv7 and page for ARMv8.
I wanted to use section numbers everywhere, since in the ARMv7 ARM they are nice and precise. But the ARMv8 ARM doesn't really use them in the same way.
Quoted 5 lines
In this case D1-1505 is in section D1.15.3 which starts on page D1-1495 and covers the whole of
HCR_EL2, which isn't a useful reference since it would be the same for 2/3 of the patches (AKA bits inHCR_EL2) in this series. D1-1505 isn't perfect since there is other stuff on that page, but it's as close I can get.
Right, it's better to have a specific reference. I was surprised to see that ARMv8 didn't have number for small section.
Quoted 5 lines
Bit in the ARMv7 ARM B1.14.9 is exactly about these two traps.
Part of my goal here is to make it so that people (especially me) don't spend half their life searching for the doc associated with a given
HCR_EL2trap bit.
Thank you for the explanation. When I reviewed the series, it was very handy to have a reference to the docs.
Regards,
Signature · 1 line
Julien Grall
PATCH 8/19 xen: arm: implement handling of ACTLR_EL1 trap
In reply to Ian CampbellWhile annotating ACTLR I noticed that we don't appear to handle the 64-bit version of this trap. Do so and annotate everything.
--- xen/arch/arm/traps.c | 20 ++++++++++++++++++++ xen/include/asm-arm/sysregs.h | 1 + 2 files changed, 21 insertions(+)
xen/arch/arm/traps.c +20 -0
@@ -1647,6 +1647,13 @@ static void do_cp15_32(struct cpu_user_regs *regs,if ( !vtimer_emulate (regs, hsr) )return inject_undef_exception (regs, hsr);break ; + +/* +* HSR_EL2.TASC / HSR.TAC +* +* ARMv7 (DDI 0406C.b): B1.14.6 +* ARMv8 (DDI 0487A.d): G6.2.1 +*/ case HSR_CPREG32 (ACTLR ):if (psr_mode_is_user (regs) )return inject_undef_exception (regs, hsr);
@@ -1849,9 +1856,22 @@ static void do_sysreg(struct cpu_user_regs *regs, const union hsr hsr) {register_t *x =select_user_reg (regs, hsr.sysreg.reg); +struct vcpu *v = current;switch ( hsr.bits &HSR_SYSREG_REGS_MASK ) { +/* +* HSR_EL2.TASC +* +* ARMv8 (DDI 0487A.d): D7.2.1 +*/ +case HSR_SYSREG_ACTLR_EL1 : +if (psr_mode_is_user (regs) ) +return inject_undef_exception (regs, hsr); +if ( hsr.sysreg.read ) + *x = v->arch.actlr; +break ; +/* RAZ/WI registers: */ /* - Debug */ case HSR_SYSREG_MDSCR_EL1 :
xen/include/asm-arm/sysregs.h +1 -0
@@ -72,6 +72,7 @@ case HSR_SYSREG_##REG##n_EL1(15)#define HSR_SYSREG_SCTLR_EL1 HSR_SYSREG(3,0,c1, c0,0) +#define HSR_SYSREG_ACTLR_EL1 HSR_SYSREG(3,0,c1, c0,1)#define HSR_SYSREG_TTBR0_EL1 HSR_SYSREG(3,0,c2, c0,0)#define HSR_SYSREG_TTBR1_EL1 HSR_SYSREG(3,0,c2, c0,1)#define HSR_SYSREG_TCR_EL1 HSR_SYSREG(3,0,c2, c0,2)
Signature · 1 line
1.7.10.4
Re: [PATCH 08/19] xen: arm: implement handling of ACTLR_EL1 trap
Hi Ian,
Quoted 2 lines · Ian Campbell
While annotating ACTLR I noticed that we don't appear to handle the 64-bit version of this trap. Do so and annotate everything.
While Linux doesn't use ACTLR_EL1 on aarch64, another OS may use it.
I'm not sure if we should consider it as a possible security issue as at least the Cortex A53 implements the register RES0.
Quoted 11 lines
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 20 ++++++++++++++++++++ xen/include/asm-arm/sysregs.h | 1 + 2 files changed, 21 insertions(+)xen/arch/arm/traps.c +0
-0
xen/arch/arm/traps.c static void do_cp15_32(struct cpu_user_regs *regs, · line 1652
Quoted 6 lines
xen/arch/arm/traps.c +3
-0@@ -1647,6 +1647,13 @@ static void do_cp15_32(struct cpu_user_regs *regs,if ( !vtimer_emulate (regs, hsr) )return inject_undef_exception (regs, hsr);break ; + +/* +* HSR_EL2.TASC / HSR.TAC
I don't find any TASC in the ARMv8 doc. Did you intend to say TACR?
Also it's not HSR but HCR.
Quoted 8 lines
xen/arch/arm/traps.c +4
-0+* +* ARMv7 (DDI 0406C.b): B1.14.6 +* ARMv8 (DDI 0487A.d): G6.2.1 +*/ case HSR_CPREG32 (ACTLR ):if (psr_mode_is_user (regs) )return inject_undef_exception (regs, hsr);
xen/arch/arm/traps.c static void do_sysreg(struct cpu_user_regs *regs, · line 1864
Quoted 9 lines
xen/arch/arm/traps.c +3
-0@@ -1849,9 +1856,22 @@ static void do_sysreg(struct cpu_user_regs *regs, const union hsr hsr) {register_t *x =select_user_reg (regs, hsr.sysreg.reg); +struct vcpu *v = current;switch ( hsr.bits &HSR_SYSREG_REGS_MASK ) { +/* +* HSR_EL2.TASC
Same question here for TASC.
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH 08/19] xen: arm: implement handling of ACTLR_EL1 trap
Quoted 10 lines · Julien Grall
Hi Ian,
Quoted 2 lines · Ian Campbell
While annotating ACTLR I noticed that we don't appear to handle the 64-bit version of this trap. Do so and annotate everything.
While Linux doesn't use
ACTLR_EL1on aarch64, another OS may use it.I'm not sure if we should consider it as a possible security issue as at least the Cortex A53 implements the register RES0.
Without this patch we would end up logging a debug message and injecting undef into the guest. Since this is an EL1 register all a malicious guest can do is send itself exceptions.
Quoted 7 lines
Quoted 7 lines
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 20 ++++++++++++++++++++ xen/include/asm-arm/sysregs.h | 1 + 2 files changed, 21 insertions(+)
Quoted 13 lines
xen/arch/arm/traps.c static void do_cp15_32(struct cpu_user_regs *regs, · line 1652
Quoted 11 lines
xen/arch/arm/traps.c +3
-0@@ -1647,6 +1647,13 @@ static void do_cp15_32(struct cpu_user_regs *regs,if ( !vtimer_emulate (regs, hsr) )return inject_undef_exception (regs, hsr);break ; + +/* +* HSR_EL2.TASC / HSR.TAC I don't find any TASC in the ARMv8 doc. Did you intend to say TACR?
Indeed, I did.
Quoted 1 line
Also it's not HSR but HCR.
Yes, sigh :-(
Ian.
PATCH 9/19 xen: arm: Annotate registers trapped by HSR_EL1.TIDCP
In reply to Ian CampbellThis traps variety of implementation defined registers, so add a note to the default case of the respective handler.
--- xen/arch/arm/traps.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
xen/arch/arm/traps.c +16 -0
@@ -1698,6 +1698,14 @@ static void do_cp15_32(struct cpu_user_regs *regs,*/ return handle_raz_wi (regs, r, cp32.read, hsr,1 ); +/* +* HCR_EL2.TIDCP +* +* ARMv7 (DDI 0406C.b): B1.14.3 +* ARMv8 (DDI 0487A.d): D1-1501 Table D1-43 +* +* And all other unknown registers. +*/ default : gdprintk(XENLOG_ERR, "%s p15, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
@@ -1948,6 +1956,14 @@ static void do_sysreg(struct cpu_user_regs *regs,dprintk (XENLOG_WARNING ,"Emulation of sysreg ICC_SGI0R_EL1/ASGI1R_EL1 not supported\n" );return inject_undef64_exception (regs, hsr.len); + +/* +* HCR_EL2.TIDCP +* +* ARMv8 (DDI 0487A.d): D1-1501 Table D1-43 +* +* And all other unknown registers. +*/ default : {const struct hsr_sysreg sysreg = hsr.sysreg;
Signature · 1 line
1.7.10.4
Re: [PATCH 09/19] xen: arm: Annotate registers trapped by HSR_EL1.TIDCP
Hi Ian,
Subject: s/HSR/HCR/
Quoted 4 lines · Ian Campbell
This traps variety of implementation defined registers, so add a note to the default case of the respective handler.
Signed-off-by Ian Campbell <[email protected]>
Other than the typo in the subject:
Regards,
Quoted 39 lines
--- xen/arch/arm/traps.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)xen/arch/arm/traps.c +16
-0@@ -1698,6 +1698,14 @@ static void do_cp15_32(struct cpu_user_regs *regs,*/ return handle_raz_wi (regs, r, cp32.read, hsr,1 ); +/* +* HCR_EL2.TIDCP +* +* ARMv7 (DDI 0406C.b): B1.14.3 +* ARMv8 (DDI 0487A.d): D1-1501 Table D1-43 +* +* And all other unknown registers. +*/ default : gdprintk(XENLOG_ERR, "%s p15, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",@@ -1948,6 +1956,14 @@ static void do_sysreg(struct cpu_user_regs *regs,dprintk (XENLOG_WARNING ,"Emulation of sysreg ICC_SGI0R_EL1/ASGI1R_EL1 not supported\n" );return inject_undef64_exception (regs, hsr.len); + +/* +* HCR_EL2.TIDCP +* +* ARMv8 (DDI 0487A.d): D1-1501 Table D1-43 +* +* And all other unknown registers. +*/ default : {const struct hsr_sysreg sysreg = hsr.sysreg;
Signature · 1 line
Julien Grall
PATCH 10/19 xen: arm: Annotate registers trapped by CPTR_EL2.TTA
In reply to Ian CampbellAdd explicit handler for 64-bit CP14 accesses, with more relevant debug message (as per other handlers) and to provide a place for the comment.
--- xen/arch/arm/traps.c | 43 +++++++++++++++++++++++++++++++++++++- xen/include/asm-arm/perfc_defn.h | 1 + 2 files changed, 43 insertions(+), 1 deletion(-)
xen/arch/arm/traps.c +42 -1
@@ -1810,6 +1810,13 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)case HSR_CPREG32 (DBGOSLAR ):return handle_wo_wi (regs, r, cp32.read, hsr,1 ); + +/* +* CPTR_EL2.TTA +* +* ARMv7 (DDI 0406C.b): B1.14.16 +* ARMv8 (DDI 0487A.d): D1-1507 Table D1-54 +*/ default : gdprintk(XENLOG_ERR, "%s p14, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
@@ -1824,7 +1831,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)advance_pc (regs, hsr); } -static void do_cp14_dbg (struct cpu_user_regs *regs,const union hsr hsr) +static void do_cp14_64 (struct cpu_user_regs *regs,const union hsr hsr) {const struct hsr_cp64 cp64 = hsr.cp64;
@@ -1834,12 +1841,37 @@ static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)return ; } +/* +* CPTR_EL2.TTA +* +* ARMv7 (DDI 0406C.b): B1.14.16 +* ARMv8 (DDI 0487A.d): D1-1507 Table D1-54 +*/ gdprintk (XENLOG_ERR ,"%s p14, %d, r%d, r%d, cr%d @ 0x%" PRIregister"\n" , cp64.read ?"mrrc" :"mcrr" , cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc);gdprintk (XENLOG_ERR ,"unhandled 64-bit CP14 access %#x\n" , hsr.bits &HSR_CP64_REGS_MASK ); +inject_undef_exception (regs, hsr); +} + +static void do_cp14_dbg (struct cpu_user_regs *regs,const union hsr hsr) +{ +struct hsr_cp64 cp64 = hsr.cp64; + +if ( !check_conditional_instr (regs, hsr) ) + { +advance_pc (regs, hsr); +return ; + } + +gdprintk (XENLOG_ERR , +"%s p14, %d, r%d, r%d, cr%d @ 0x%" PRIregister"\n" , + cp64.read ?"mrrc" :"mcrr" , + cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc); +gdprintk (XENLOG_ERR ,"unhandled 64-bit CP14 DBG access %#x\n" , + hsr.bits &HSR_CP64_REGS_MASK );inject_undef_exception (regs, hsr); }
@@ -1962,6 +1994,10 @@ static void do_sysreg(struct cpu_user_regs *regs,* * ARMv8 (DDI 0487A.d): D1-1501 Table D1-43 * +* CPTR_EL2.TTA +* +* ARMv8 (DDI 0487A.d): D1-1507 Table D1-54 +* * And all other unknown registers. */ default :
@@ -2156,6 +2192,11 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)perfc_incr (trap_cp14_32);do_cp14_32 (regs, hsr);break ; +case HSR_EC_CP14_64 : +GUEST_BUG_ON (!psr_mode_is_32bit (regs->cpsr)); +perfc_incr (trap_cp14_64); +do_cp14_64 (regs, hsr); +break ;case HSR_EC_CP14_DBG :GUEST_BUG_ON (!psr_mode_is_32bit (regs->cpsr));perfc_incr (trap_cp14_dbg);
xen/include/asm-arm/perfc_defn.h +1 -0
@@ -9,6 +9,7 @@ PERFCOUNTER(trap_wfe, "trap: wfe") PERFCOUNTER(trap_cp15_32, "trap: cp15 32-bit access") PERFCOUNTER(trap_cp15_64, "trap: cp15 64-bit access") PERFCOUNTER(trap_cp14_32, "trap: cp14 32-bit access") +PERFCOUNTER(trap_cp14_64, "trap: cp14 64-bit access") PERFCOUNTER(trap_cp14_dbg, "trap: cp14 dbg access") PERFCOUNTER(trap_cp, "trap: cp access")PERFCOUNTER (trap_smc32,"trap: 32-bit smc" )
Signature · 1 line
1.7.10.4
Re: [PATCH 10/19] xen: arm: Annotate registers trapped by CPTR_EL2.TTA
Hi Ian,
Quoted 3 lines · Ian Campbell
Add explicit handler for 64-bit CP14 accesses, with more relevant debug message (as per other handlers) and to provide a place for the comment.
It's a bit strange to name the patch "Annotate..." while the main change is 64-bit CP14 accesses.
AFAICT, this was a bug in Xen implementation. Although, I'm not sure if the current platform we support have Trace registers (maybe the Arndale?).
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH 10/19] xen: arm: Annotate registers trapped by CPTR_EL2.TTA
Quoted 9 lines · Julien Grall
Hi Ian,
Quoted 3 lines · Ian Campbell
Add explicit handler for 64-bit CP14 accesses, with more relevant debug message (as per other handlers) and to provide a place for the comment.
It's a bit strange to name the patch "Annotate..." while the main change is 64-bit CP14 accesses.
I mostly started off annotating things, which lead me to notice things which are missing. I've retitled to "xen: arm: implement handling of registers trapped by CPTR_EL2.TTA"
Quoted 2 lines
AFAICT, this was a bug in Xen implementation. Although, I'm not sure if the current platform we support have Trace registers (maybe the Arndale?).
Hrm, good point. I'm not sure really.
Ian.
PATCH 11/19 xen: arm: Annotate handlers for PCTR_EL2.Tx
In reply to Ian Campbell--- xen/arch/arm/traps.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
xen/arch/arm/traps.c +14 -0
@@ -1704,6 +1704,11 @@ static void do_cp15_32(struct cpu_user_regs *regs,* ARMv7 (DDI 0406C.b): B1.14.3 * ARMv8 (DDI 0487A.d): D1-1501 Table D1-43 * +* CPTR_EL2.T{0..9,12..13} +* +* ARMv7 (DDI 0406C.b): B1.14.12 +* ARMv8 (DDI 0487A.d): N/A +* * And all other unknown registers. */ default :
@@ -1735,6 +1740,15 @@ static void do_cp15_64(struct cpu_user_regs *regs,if ( !vtimer_emulate (regs, hsr) )return inject_undef_exception (regs, hsr);break ; + +/* +* CPTR_EL2.T{0..9,12..13} +* +* ARMv7 (DDI 0406C.b): B1.14.12 +* ARMv8 (DDI 0487A.d): N/A +* +* And all other unknown registers. +*/ default : {const struct hsr_cp64 cp64 = hsr.cp64;
Signature · 1 line
1.7.10.4
Re: [PATCH 11/19] xen: arm: Annotate handlers for PCTR_EL2.Tx
Hi Ian,
Subject: s/PCTR/CPTR/
Quoted 10 lines · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)xen/arch/arm/traps.c +0
-0
xen/arch/arm/traps.c static void do_cp15_32(struct cpu_user_regs *regs, · line 1710
Quoted 7 lines
xen/arch/arm/traps.c +4
-0@@ -1704,6 +1704,11 @@ static void do_cp15_32(struct cpu_user_regs *regs,* ARMv7 (DDI 0406C.b): B1.14.3 * ARMv8 (DDI 0487A.d): D1-1501 Table D1-43 * +* CPTR_EL2.T{0..9,12..13} +* +* ARMv7 (DDI 0406C.b): B1.14.12 +* ARMv8 (DDI 0487A.d): N/A
I would also update the comment on top of WRITE_SYSREG(..., CPTR_EL2) to make clear that CP0..CP9 & CP12..CP13 are only traps for ARMv7.
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH 11/19] xen: arm: Annotate handlers for PCTR_EL2.Tx
Quoted 10 lines · Julien Grall
Hi Ian,
Subject: s/PCTR/CPTR/
Quoted 5 lines · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
Quoted 15 lines
xen/arch/arm/traps.c static void do_cp15_32(struct cpu_user_regs *regs, · line 1710
Quoted 12 lines
xen/arch/arm/traps.c +4
-0@@ -1704,6 +1704,11 @@ static void do_cp15_32(struct cpu_user_regs *regs,* ARMv7 (DDI 0406C.b): B1.14.3 * ARMv8 (DDI 0487A.d): D1-1501 Table D1-43 * +* CPTR_EL2.T{0..9,12..13} +* +* ARMv7 (DDI 0406C.b): B1.14.12 +* ARMv8 (DDI 0487A.d): N/A I would also update the comment on top of
WRITE_SYSREG(..., CPTR_EL2)to make clear that CP0..CP9 & CP12..CP13 are only traps for ARMv7.
On v8 the corresponding bits are RES1, i.e. they always trap. I wrote:
/* Trap all coprocessor registers (0-13) except cp10 and
* cp11 for VFP.
*
* /!\ All coprocessors except cp10 and cp11 cannot be used in Xen. *
* On ARM64 the TCPx bits which we set here (0..9,12,13) are all
* RES1, i.e. they would trap whether we did this write or not. */
Ian.
Re: [PATCH 11/19] xen: arm: Annotate handlers for PCTR_EL2.Tx
Hi Ian,
Quoted 27 lines · Ian Campbell
Quoted 10 lines · Julien Grall
Hi Ian,
Subject: s/PCTR/CPTR/
Quoted 5 lines · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)Quoted 15 lines
xen/arch/arm/traps.c static void do_cp15_32(struct cpu_user_regs *regs, · line 1710
Quoted 12 lines
xen/arch/arm/traps.c +4
-0@@ -1704,6 +1704,11 @@ static void do_cp15_32(struct cpu_user_regs *regs,* ARMv7 (DDI 0406C.b): B1.14.3 * ARMv8 (DDI 0487A.d): D1-1501 Table D1-43 * +* CPTR_EL2.T{0..9,12..13} +* +* ARMv7 (DDI 0406C.b): B1.14.12 +* ARMv8 (DDI 0487A.d): N/A I would also update the comment on top of
WRITE_SYSREG(..., CPTR_EL2)to make clear that CP0..CP9 & CP12..CP13 are only traps for ARMv7.
Quoted 1 line
On v8 the corresponding bits are RES1, i.e. they always trap. I wrote:
Somehow I read RES0 in the spec.
Quoted 9 lines
/* Trap all coprocessor registers (0-13) except cp10 and
* cp11 for VFP.
*
* /!\ All coprocessors except cp10 and cp11 cannot be used in Xen. *
* On ARM64 the TCPx bits which we set here (0..9,12,13) are all
* RES1, i.e. they would trap whether we did this write or not. */
I'm fine with this change.
Regards,
Signature · 1 line
Julien Grall
PATCH 12/19 xen: arm: Annotate the handlers for HSTR_EL2.Tx
In reply to Ian Campbell--- xen/arch/arm/traps.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
xen/arch/arm/traps.c +10 -0
@@ -1709,6 +1709,11 @@ static void do_cp15_32(struct cpu_user_regs *regs,* ARMv7 (DDI 0406C.b): B1.14.12 * ARMv8 (DDI 0487A.d): N/A * +* HSTR_EL2.Tx +* +* ARMv7 (DDI 0406C.b): B1.14.14 +* ARMv8 (DDI 0487A.d): D1-1507 Table D1-55 +* * And all other unknown registers. */ default :
@@ -1747,6 +1752,11 @@ static void do_cp15_64(struct cpu_user_regs *regs,* ARMv7 (DDI 0406C.b): B1.14.12 * ARMv8 (DDI 0487A.d): N/A * +* HSTR_EL2.Tx +* +* ARMv7 (DDI 0406C.b): B1.14.14 +* ARMv8 (DDI 0487A.d): D1-1507 Table D1-55 +* * And all other unknown registers. */ default :
Signature · 1 line
1.7.10.4
Re: [PATCH 12/19] xen: arm: Annotate the handlers for HSTR_EL2.Tx
Hi Ian,
Quoted 10 lines · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 10 ++++++++++ 1 file changed, 10 insertions(+)xen/arch/arm/traps.c +0
-0
xen/arch/arm/traps.c static void do_cp15_32(struct cpu_user_regs *regs, · line 1712
Quoted 4 lines
xen/arch/arm/traps.c +1
-0@@ -1709,6 +1709,11 @@ static void do_cp15_32(struct cpu_user_regs *regs,* ARMv7 (DDI 0406C.b): B1.14.12 * ARMv8 (DDI 0487A.d): N/A * +* HSTR_EL2.Tx
I would prefer if you use T15 instead of Tx. This is less confusing as we only trap c15 and the bit T14 exists on ARMv8 (even though it's RES0).
Regards,
Signature · 1 line
Julien Grall
PATCH 13/19 xen: arm: Annotate registers trapped by MDCR_EL2.TDRA
In reply to Ian Campbell--- xen/arch/arm/traps.c | 32 ++++++++++++++++++++++++++++++++ xen/include/asm-arm/cpregs.h | 4 ++++ xen/include/asm-arm/sysregs.h | 1 + 3 files changed, 37 insertions(+)
xen/arch/arm/traps.c +32 -0
@@ -1790,6 +1790,17 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)switch ( hsr.bits &HSR_CP32_REGS_MASK ) { +/* +* MDCR_EL2.TDRA +* +* ARMv7 (DDI 0406C.b): B1.14.15 +* ARMv8 (DDI 0487A.d): D1-1508 Table D1-57 +* +* Unhandled: +* DBGDRAR +* DBGDSAR +*/ +case HSR_CPREG32 (DBGDIDR ):/* * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
@@ -1840,6 +1851,8 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)* * ARMv7 (DDI 0406C.b): B1.14.16 * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54 +* +* And all other unknown registers. */ default : gdprintk(XENLOG_ERR,
@@ -1870,6 +1883,17 @@ static void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr)* * ARMv7 (DDI 0406C.b): B1.14.16 * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54 +* +* MDCR_EL2.TDRA +* +* ARMv7 (DDI 0406C.b): B1.14.15 +* ARMv8 (DDI 0487A.d): D1-1508 Table D1-57 +* +* Unhandled: +* DBGDRAR64 +* DBGDSAR64 +* +* And all other unknown registers. */ gdprintk(XENLOG_ERR, "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
@@ -1936,6 +1960,14 @@ static void do_sysreg(struct cpu_user_regs *regs, *x = v->arch.actlr;break ; +/* +* MDCR_EL2.TDRA +* +* ARMv8 (DDI 0487A.d): D1-1508 Table D1-57 +*/ +case HSR_SYSREG_MDRAR_EL1 : +return handle_ro_raz (regs, x, hsr.sysreg.read, hsr,1 ); +/* RAZ/WI registers: */ /* - Debug */ case HSR_SYSREG_MDSCR_EL1 :
xen/include/asm-arm/cpregs.h +4 -0
@@ -89,10 +89,14 @@#define TEECR p14,6,c0,c0,0/* ThumbEE Configuration Register */ /* CP14 CR1: */ +#define DBGDRAR64 p14,0,c1/* Debug ROM Address Register (64-bit access) */ +#define DBGDRAR p14,0,c1,c0,0/* Debug ROM Address Register (32-bit access) */ #define TEEHBR p14,6,c1,c0,0/* ThumbEE Handler Base Register */ #define JOSCR p14,7,c1,c0,0/* Jazelle OS Control Register */ /* CP14 CR2: */ +#define DBGDSAR64 p14,0,c2/* Debug Self Address Offset Register (64-bit access) */ +#define DBGDSAR p14,0,c2,c0,0/* Debug Self Address Offset Register (32-bit access) */ #define JMCR p14,7,c2,c0,0/* Jazelle Main Configuration Register */
xen/include/asm-arm/sysregs.h +1 -0
@@ -45,6 +45,7 @@#define HSR_SYSREG_DCCISW HSR_SYSREG(1,0,c7,c14,2)#define HSR_SYSREG_MDSCR_EL1 HSR_SYSREG(2,0,c0,c2,2) +#define HSR_SYSREG_MDRAR_EL1 HSR_SYSREG(2,0,c1,c0,0)#define HSR_SYSREG_OSLAR_EL1 HSR_SYSREG(2,0,c1,c0,4)#define HSR_SYSREG_OSDLR_EL1 HSR_SYSREG(2,0,c1,c3,4)#define HSR_SYSREG_MDCCSR_EL0 HSR_SYSREG(2,3,c0,c1,0)
Signature · 1 line
1.7.10.4
Re: [PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA
Hi Ian,
Quoted 16 lines · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 32 ++++++++++++++++++++++++++++++++ xen/include/asm-arm/cpregs.h | 4 ++++ xen/include/asm-arm/sysregs.h | 1 + 3 files changed, 37 insertions(+)xen/arch/arm/traps.c +1
-0@@ -1790,6 +1790,17 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)switch ( hsr.bits &HSR_CP32_REGS_MASK ) { +/*
xen/arch/arm/traps.c static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr) · line 1803
Quoted 10 lines
xen/arch/arm/traps.c +10
-0+* MDCR_EL2.TDRA +* +* ARMv7 (DDI 0406C.b): B1.14.15 +* ARMv8 (DDI 0487A.d): D1-1508 Table D1-57 +* +* Unhandled: +* DBGDRAR +* DBGDSAR +*/ +
Why did you put the comment here? For AArch32, only DBGDRAR and DBGSAR are trapped with this bit.
I think this should be moved above the label default.
Quoted 15 lines
xen/arch/arm/traps.c +2
-0case HSR_CPREG32 (DBGDIDR ):/* * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis @@ -1840,6 +1851,8 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)* * ARMv7 (DDI 0406C.b): B1.14.16 * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54 +* +* And all other unknown registers. */ default : gdprintk(XENLOG_ERR,@@ -1870,6 +1883,17 @@ static void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr)* * ARMv7 (DDI 0406C.b): B1.14.16
xen/arch/arm/traps.c static void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr) · line 1894
Quoted 10 lines
xen/arch/arm/traps.c +9
-0* ARMv8 (DDI 0487A.d): D1-1507 Table D1-54 +* +* MDCR_EL2.TDRA +* +* ARMv7 (DDI 0406C.b): B1.14.15 +* ARMv8 (DDI 0487A.d): D1-1508 Table D1-57 +* +* Unhandled: +* DBGDRAR64 +* DBGDSAR64
This is confusing. The real name of the register is DBGDRAR. I would say "DBGDRAR 64-bit".
Furthermore, this is the only registers not handled on AArch32 for this bit. This is rather strange to list them while you didn't do it for the trace registers.
xen/arch/arm/traps.c static void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr) · line 1896
Quoted 2 lines
xen/arch/arm/traps.c +2
-0+* +* And all other unknown registers.
For consistency, I would have add this part of the comment in patch #10 (where the comment has been added).
Anyway, the patch is already written so I'm fine with it.
Quoted 4 lines
xen/arch/arm/traps.c +0
-0*/ gdprintk(XENLOG_ERR, "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
xen/arch/arm/traps.c static void do_sysreg(struct cpu_user_regs *regs, · line 1969
Quoted 10 lines
xen/arch/arm/traps.c +7
-0@@ -1936,6 +1960,14 @@ static void do_sysreg(struct cpu_user_regs *regs, *x = v->arch.actlr;break ; +/* +* MDCR_EL2.TDRA +* +* ARMv8 (DDI 0487A.d): D1-1508 Table D1-57 +*/ +case HSR_SYSREG_MDRAR_EL1 : +return handle_ro_raz (regs, x, hsr.sysreg.read, hsr,1 );
This change should be in a separate patch or mention in the commit message.
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA
Quoted 33 lines · Julien Grall
Hi Ian,
Quoted 16 lines · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 32 ++++++++++++++++++++++++++++++++ xen/include/asm-arm/cpregs.h | 4 ++++ xen/include/asm-arm/sysregs.h | 1 + 3 files changed, 37 insertions(+)xen/arch/arm/traps.c +1
-0@@ -1790,6 +1790,17 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)switch ( hsr.bits &HSR_CP32_REGS_MASK ) { +/* xen/arch/arm/traps.c static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr) · line 1803
Quoted 10 lines
xen/arch/arm/traps.c +10
-0+* MDCR_EL2.TDRA +* +* ARMv7 (DDI 0406C.b): B1.14.15 +* ARMv8 (DDI 0487A.d): D1-1508 Table D1-57 +* +* Unhandled: +* DBGDRAR +* DBGDSAR +*/ +Why did you put the comment here? For AArch32, only DBGDRAR and DBGSAR are trapped with this bit.
Quoted 1 line
I think this should be moved above the label default.
Yes, not sure how it got out of place here. Moved.
Quoted 21 lines
Quoted 21 lines
xen/arch/arm/traps.c +6
-0case HSR_CPREG32 (DBGDIDR ):/* * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis @@ -1840,6 +1851,8 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)* * ARMv7 (DDI 0406C.b): B1.14.16 * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54 +* +* And all other unknown registers. */ default : gdprintk(XENLOG_ERR,@@ -1870,6 +1883,17 @@ static void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr)* * ARMv7 (DDI 0406C.b): B1.14.16 * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54 +* +* MDCR_EL2.TDRA +* +* ARMv7 (DDI 0406C.b): B1.14.15
Quoted 8 lines
xen/arch/arm/traps.c static void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr) · line 1894
Quoted 5 lines
xen/arch/arm/traps.c +5
-0+* ARMv8 (DDI 0487A.d): D1-1508 Table D1-57 +* +* Unhandled: +* DBGDRAR64 +* DBGDSAR64 This is confusing. The real name of the register is DBGDRAR. I would say "DBGDRAR 64-bit".
Yes, this was a tricky one because the register is actually two (a 32 and 64 version), hence in the #define I had to add a suffix and I duplicated that here.
Instead I'll make both of them "DBGDRAR (NN-bit accesses)", similar to what I did for the #defines
Quoted 4 lines
Furthermore, this is the only registers not handled on AArch32 for this bit. This is rather strange to list them while you didn't do it for the trace registers.
My intention was that every register trapped by a bit which we set be listed somewhere, to make it easier to cross reference with the docs and check we haven't accidentally forgotten something (as opposed to deliberately ignoring as indicated by these comments).
You seem to be saying I've missed some trace registers, which ones?
Quoted 8 lines
xen/arch/arm/traps.c static void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr) · line 1896
Quoted 3 lines
xen/arch/arm/traps.c +2
-0+* +* And all other unknown registers. For consistency, I would have add this part of the comment in patch #10 (where the comment has been added).
Anyway, the patch is already written so I'm fine with it.
I was rebasing any way so I did this.
Quoted 7 lines
Quoted 7 lines
xen/arch/arm/traps.c +0
-0*/ gdprintk(XENLOG_ERR, "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",@@ -1936,6 +1960,14 @@ static void do_sysreg(struct cpu_user_regs *regs, *x = v->arch.actlr;break ;
Quoted 9 lines
xen/arch/arm/traps.c static void do_sysreg(struct cpu_user_regs *regs, · line 1969
Quoted 7 lines
xen/arch/arm/traps.c +7
-0+/* +* MDCR_EL2.TDRA +* +* ARMv8 (DDI 0487A.d): D1-1508 Table D1-57 +*/ +case HSR_SYSREG_MDRAR_EL1 : +return handle_ro_raz (regs, x, hsr.sysreg.read, hsr,1 );This change should be in a separate patch or mention in the commit message.
I just added a mention of it since it is the AArch64 version of DBGDRAR. I can't remember why I made this raz instead of trapping like with DBGDRAR, but since I already wrote it I left them as is.
Ian
Re: [PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA
Hi Ian,
Quoted 10 lines · Ian Campbell
Quoted 3 lines
Furthermore, this is the only registers not handled on AArch32 for this bit. This is rather strange to list them while you didn't do it for the trace registers.
My intention was that every register trapped by a bit which we set be listed somewhere, to make it easier to cross reference with the docs and check we haven't accidentally forgotten something (as opposed to deliberately ignoring as indicated by these comments).
You seem to be saying I've missed some trace registers, which ones?
I meant that you didn't list the trace registers trapped but unhandled. Although I wasn't able to find a list, is it trace module specific? If so maybe a comment would be good?
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA
Quoted 12 lines · Julien Grall
Hi Ian,
Quoted 9 lines · Ian Campbell
Quoted 3 lines
Furthermore, this is the only registers not handled on AArch32 for this bit. This is rather strange to list them while you didn't do it for the trace registers.
My intention was that every register trapped by a bit which we set be listed somewhere, to make it easier to cross reference with the docs and check we haven't accidentally forgotten something (as opposed to deliberately ignoring as indicated by these comments).
Quoted 5 lines
Quoted 1 line
You seem to be saying I've missed some trace registers, which ones?
I meant that you didn't list the trace registers trapped but unhandled. Although I wasn't able to find a list, is it trace module specific? If so maybe a comment would be good?
I think maybe you are talking about the things trapped by CPTR_EL2.TTA rather than MDCR_EL2.TDRA (the subject of this patch)?
The table referenced for CPTR_EL2.TTA just says "All implemented trace registers", rather than listing anything specific. I could add a similar comment to the relevant patch.
Looks like HCR_EL2.TIDCP is similarly lacking a comment for the unhandled ones. I'll add one.
Quoted 3 lines
Regards,
PATCH 14/19 xen: arm: Annotate registers trapped by MDCR_EL2.TDOSA
In reply to Ian CampbellGather the affected handlers in a single place per trap type.
Add some HSR_SYSREG and AArch32 defines for those registers (because I'd already typed them in when I realised I didn't need them).
--- xen/arch/arm/traps.c | 37 +++++++++++++++++++++++++++++-------- xen/include/asm-arm/cpregs.h | 2 ++ xen/include/asm-arm/sysregs.h | 2 ++ 3 files changed, 33 insertions(+), 8 deletions(-)
xen/arch/arm/traps.c +29 -8
@@ -1801,6 +1801,21 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)* DBGDSAR */ +/* +* MDCR_EL2.TDOSA +* +* ARMv7 (DDI 0406C.b): B1.14.15 +* ARMv8 (DDI 0487A.d): D1-1509 Table D1-58 +* +* Unhandled: +* DBGOSLSR +* DBGPRCR +*/ +case HSR_CPREG32 (DBGOSLAR ): +return handle_wo_wi (regs, r, cp32.read, hsr,1 ); +case HSR_CPREG32 (DBGOSDLR ): +return handle_raz_wi (regs, r, cp32.read, hsr,1 ); +case HSR_CPREG32 (DBGDIDR ):/* * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
@@ -1840,12 +1855,8 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)case HSR_CPREG32 (DBGWCR0 ):case HSR_CPREG32 (DBGBVR1 ):case HSR_CPREG32 (DBGBCR1 ): -case HSR_CPREG32 (DBGOSDLR ):return handle_raz_wi (regs, r, cp32.read, hsr,1 ); -case HSR_CPREG32 (DBGOSLAR ): -return handle_wo_wi (regs, r, cp32.read, hsr,1 ); -/* * CPTR_EL2.TTA *
@@ -1968,6 +1979,20 @@ static void do_sysreg(struct cpu_user_regs *regs,case HSR_SYSREG_MDRAR_EL1 :return handle_ro_raz (regs, x, hsr.sysreg.read, hsr,1 ); +/* +* MDCR_EL2.TDOSA +* +* ARMv8 (DDI 0487A.d): D1-1509 Table D1-58 +* +* Unhandled: +* OSLSR_EL1 +* DBGPRCR_EL1 +*/ +case HSR_SYSREG_OSLAR_EL1 : +return handle_wo_wi (regs, x, hsr.sysreg.read, hsr,1 ); +case HSR_SYSREG_OSDLR_EL1 : +return handle_raz_wi (regs, x, hsr.sysreg.read, hsr,1 ); +/* RAZ/WI registers: */ /* - Debug */ case HSR_SYSREG_MDSCR_EL1 :
@@ -1977,8 +2002,6 @@ static void do_sysreg(struct cpu_user_regs *regs,/* - Watchpoints */ HSR_SYSREG_DBG_CASES(DBGWVR): HSR_SYSREG_DBG_CASES(DBGWCR): -/* - Double Lock Register */ -case HSR_SYSREG_OSDLR_EL1 :/* - Perf monitors */ case HSR_SYSREG_PMINTENSET_EL1 :case HSR_SYSREG_PMINTENCLR_EL1 :
@@ -2021,8 +2044,6 @@ static void do_sysreg(struct cpu_user_regs *regs,return handle_raz_wi (regs, x, hsr.sysreg.read, hsr,1 );/* Write only, Write ignore registers: */ -case HSR_SYSREG_OSLAR_EL1 : -return handle_wo_wi (regs, x, hsr.sysreg.read, hsr,1 );case HSR_SYSREG_CNTP_CTL_EL0 :case HSR_SYSREG_CNTP_TVAL_EL0 :
xen/include/asm-arm/cpregs.h +2 -0
@@ -83,7 +83,9 @@#define DBGBVR1 p14,0,c0,c1,4/* Breakpoint Value 1 */ #define DBGBCR1 p14,0,c0,c1,5/* Breakpoint Control 1 */ #define DBGOSLAR p14,0,c1,c0,4/* OS Lock Access */ +#define DBGOSLSR p14,0,c1,c1,4/* OS Lock Status Register */ #define DBGOSDLR p14,0,c1,c3,4/* OS Double Lock */ +#define DBGPRCR p14,0,c1,c4,4/* Debug Power Control Register */ /* CP14 CR0: */ #define TEECR p14,6,c0,c0,0/* ThumbEE Configuration Register */
xen/include/asm-arm/sysregs.h +2 -0
@@ -47,7 +47,9 @@#define HSR_SYSREG_MDSCR_EL1 HSR_SYSREG(2,0,c0,c2,2)#define HSR_SYSREG_MDRAR_EL1 HSR_SYSREG(2,0,c1,c0,0)#define HSR_SYSREG_OSLAR_EL1 HSR_SYSREG(2,0,c1,c0,4) +#define HSR_SYSREG_OSLSR_EL1 HSR_SYSREG(2,0,c1,c1,4)#define HSR_SYSREG_OSDLR_EL1 HSR_SYSREG(2,0,c1,c3,4) +#define HSR_SYSREG_DBGPRCR_EL1 HSR_SYSREG(2,0,c1,c4,4)#define HSR_SYSREG_MDCCSR_EL0 HSR_SYSREG(2,3,c0,c1,0)#define HSR_SYSREG_DBGBVRn_EL1 (n) HSR_SYSREG(2,0,c0,c##n,4)
Signature · 1 line
1.7.10.4
PATCH 15/19 xen: arm: Annotate registers trapped by MDCR_EL2.TDA
In reply to Ian CampbellGather the affected handlers in a single place per trap type.
--- xen/arch/arm/traps.c | 71 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 60 insertions(+), 11 deletions(-)
xen/arch/arm/traps.c +60 -11
@@ -1816,6 +1816,28 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)case HSR_CPREG32 (DBGOSDLR ):return handle_raz_wi (regs, r, cp32.read, hsr,1 ); +/* +* MDCR_EL2.TDA +* +* ARMv7 (DDI 0406C.b): B1.14.15 +* ARMv8 (DDI 0487A.d): D1-1510 Table D1-59 +* +* Unhandled: +* DBGDCCINT +* DBGDTRRXint +* DBGDTRTXint +* DBGWFAR +* DBGDTRTXext +* DBGDTRRXext, +* DBGBXVR<n> +* DBGCLAIMSET +* DBGCLAIMCLR +* DBGAUTHSTATUS +* DBGDEVID +* DBGDEVID1 +* DBGDEVID2 +* DBGOSECCR +*/ case HSR_CPREG32 (DBGDIDR ):/* * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
@@ -1925,6 +1947,17 @@ static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)return ; } +/* +* MDCR_EL2.TDOSA +* +* ARMv7 (DDI 0406C.b): B1.14.15 +* ARMv8 (DDI 0487A.d): D1-1509 Table D1-58 +* +* Unhandled: +* DBGDTRTXint +* DBGDTRRXint +*/ + gdprintk(XENLOG_ERR, "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n", cp64.read ? "mrrc" : "mcrr",
@@ -1993,15 +2026,38 @@ static void do_sysreg(struct cpu_user_regs *regs,case HSR_SYSREG_OSDLR_EL1 :return handle_raz_wi (regs, x, hsr.sysreg.read, hsr,1 ); -/* RAZ/WI registers: */ -/* - Debug */ +/* +* MDCR_EL2.TDA +* +* ARMv8 (DDI 0487A.d): D1-1510 Table D1-59 +* +* Unhandled: +* MDCCINT_EL1 +* DBGDTR_EL0 +* DBGDTRRX_EL0 +* DBGDTRTX_EL0 +* OSDTRRX_EL1 +* OSDTRTX_EL1 +* OSECCR_EL1 +* DBGCLAIMSET_EL1 +* DBGCLAIMCLR_EL1 +* DBGAUTHSTATUS_EL1 +*/ case HSR_SYSREG_MDSCR_EL1 : -/* - Breakpoints */ +return handle_raz_wi (regs, x, hsr.sysreg.read, hsr,1 ); +case HSR_SYSREG_MDCCSR_EL0 : +/* +* Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate that +* register as RAZ/WI above. So RO at both EL0 and EL1. +*/ +return handle_ro_raz (regs, x, hsr.sysreg.read, hsr,0 ); HSR_SYSREG_DBG_CASES(DBGBVR): HSR_SYSREG_DBG_CASES(DBGBCR): - /* - Watchpoints */ HSR_SYSREG_DBG_CASES(DBGWVR): HSR_SYSREG_DBG_CASES(DBGWCR): + return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1); + +/* RAZ/WI registers: */ /* - Perf monitors */ case HSR_SYSREG_PMINTENSET_EL1 :case HSR_SYSREG_PMINTENCLR_EL1 :
@@ -2011,13 +2067,6 @@ static void do_sysreg(struct cpu_user_regs *regs,*/ return handle_raz_wi (regs, x, hsr.sysreg.read, hsr,1 ); -case HSR_SYSREG_MDCCSR_EL0 : -/* -* Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate that -* register as RAZ/WI above. So RO at both EL0 and EL1. -*/ -return handle_ro_raz (regs, x, hsr.sysreg.read, hsr,0 ); -/* - Perf monitors */ case HSR_SYSREG_PMUSERENR_EL0 :/* RO at EL0. RAZ/WI at EL1 */
Signature · 1 line
1.7.10.4
Re: [PATCH 15/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDA
Hi Ian,
Quoted 26 lines · Ian Campbell
Gather the affected handlers in a single place per trap type.
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 71 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 60 insertions(+), 11 deletions(-)xen/arch/arm/traps.c +11
-0@@ -1816,6 +1816,28 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)case HSR_CPREG32 (DBGOSDLR ):return handle_raz_wi (regs, r, cp32.read, hsr,1 ); +/* +* MDCR_EL2.TDA +* +* ARMv7 (DDI 0406C.b): B1.14.15 +* ARMv8 (DDI 0487A.d): D1-1510 Table D1-59 +* +* Unhandled: +* DBGDCCINT +* DBGDTRRXint +* DBGDTRTXint +* DBGWFAR
xen/arch/arm/traps.c static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr) · line 1839
Quoted 10 lines
xen/arch/arm/traps.c +10
-0+* DBGDTRTXext +* DBGDTRRXext, +* DBGBXVR<n> +* DBGCLAIMSET +* DBGCLAIMCLR +* DBGAUTHSTATUS +* DBGDEVID +* DBGDEVID1 +* DBGDEVID2 +* DBGOSECCR
Listing unhandled registers doesn't bring much information. It's more useful to explain that we choose to implement a dummy Debug support.
Also, it's very confusing that you are mixing AArch64 name with AArch32 name. The former is used for the traps register while the latter is used for register name.
xen/arch/arm/traps.c static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr) · line 1951
Quoted 10 lines
xen/arch/arm/traps.c +3
-0+*/ case HSR_CPREG32 (DBGDIDR ):/* * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis @@ -1925,6 +1947,17 @@ static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)return ; } +/* +* MDCR_EL2.TDOSA
Did you mean MDCR_EL2.TDA?
[..]
Quoted 2 lines
xen/arch/arm/traps.c +0
-1case HSR_SYSREG_MDSCR_EL1 : -/* - Breakpoints */
[..]
Quoted 1 line
- /* - Watchpoints */
I think this 2 comments was useful in order to split the list of registers.
regards,
Signature · 1 line
Julien Grall
Re: [PATCH 15/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDA
Quoted 33 lines · Julien Grall
Hi Ian,
Quoted 30 lines · Ian Campbell
Gather the affected handlers in a single place per trap type.
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 71 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 60 insertions(+), 11 deletions(-)xen/arch/arm/traps.c +15
-0@@ -1816,6 +1816,28 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)case HSR_CPREG32 (DBGOSDLR ):return handle_raz_wi (regs, r, cp32.read, hsr,1 ); +/* +* MDCR_EL2.TDA +* +* ARMv7 (DDI 0406C.b): B1.14.15 +* ARMv8 (DDI 0487A.d): D1-1510 Table D1-59 +* +* Unhandled: +* DBGDCCINT +* DBGDTRRXint +* DBGDTRTXint +* DBGWFAR +* DBGDTRTXext +* DBGDTRRXext, +* DBGBXVR<n> +* DBGCLAIMSET
Quoted 8 lines
xen/arch/arm/traps.c static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr) · line 1839
Quoted 6 lines
xen/arch/arm/traps.c +6
-0+* DBGCLAIMCLR +* DBGAUTHSTATUS +* DBGDEVID +* DBGDEVID1 +* DBGDEVID2 +* DBGOSECCR Listing unhandled registers doesn't bring much information.
Actually it does and it was quite deliberate and part of the purpose of the series. By listing these unhandled registers you can now look at the docs (in particular the ARMv8 table is very good for this) and see immediately that every register is either handled or listed as unhandled.
Without that you need to remember why certain things aren't handled in order to distinguish them from things which have been accidentally forgotten.
Quoted 3 lines
Also, it's very confusing that you are mixing AArch64 name with AArch32 name. The former is used for the traps register while the latter is used for register name.
For the traps register where it is similar enough I chose to only list the Aarch64 name, since that is what we use when we write it. I could write both I suppose but it didn't seem especially useful.
For the registers themselves it is easier to correlate with the docs if the name which is relevant to the context (e.g. an arm32 vs arm64 handler). Comparing two lists where one is Aarch64 names and the other Aarch32 is hard, especially in the dbg registers where the names don't always correlate especially obviously.
The only change I'd be willing to make here would be to list HDCR.TDA after MDCR_EL2.TDA.
Quoted 8 lines
Quoted 8 lines
xen/arch/arm/traps.c +1
-0+*/ case HSR_CPREG32 (DBGDIDR ):/* * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis @@ -1925,6 +1947,17 @@ static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)return ; }
Quoted 6 lines
xen/arch/arm/traps.c static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr) · line 1951
Quoted 2 lines
xen/arch/arm/traps.c +2
-0+/* +* MDCR_EL2.TDOSA Did you mean MDCR_EL2.TDA?
[..]
No, this hunk should have been in the previous patch. Moved.
Quoted 9 lines
Quoted 3 lines
xen/arch/arm/traps.c +0
-1case HSR_SYSREG_MDSCR_EL1 : -/* - Breakpoints */ [..]
Quoted 1 line
- /* - Watchpoints */
I think this 2 comments was useful in order to split the list of registers.
In the new code this is just:
HSR_SYSREG_DBG_CASES(DBGBVR):
HSR_SYSREG_DBG_CASES(DBGBCR):
HSR_SYSREG_DBG_CASES(DBGWVR):
HSR_SYSREG_DBG_CASES(DBGWCR):
return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);I don't think the comments were adding much any more, especially given we aren't actually doing anything with them.
Ian.
PATCH 16/19 xen: arm: Annotate registers trapped by MDCR_EL2.TPM and TPMCR
In reply to Ian Campbell--- xen/arch/arm/traps.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)
xen/arch/arm/traps.c +34 -5
@@ -1661,6 +1661,24 @@ static void do_cp15_32(struct cpu_user_regs *regs, *r = v->arch.actlr;break ; +/* +* MDCR_EL2.TPM +* +* ARMv7 (DDI 0406C.b): B1.14.17 +* ARMv8 (DDI 0487A.d): D1-1511 Table D1-61 +* +* Unhandled: +* PMEVCNTR<n> +* PMEVTYPER<n> +* PMCCFILTR +* +* MDCR_EL2.TPMCR +* +* ARMv7 (DDI 0406C.b): B1.14.17 +* ARMv8 (DDI 0487A.d): D1-1511 Table D1-62 +* +* NB: Both MDCR_EL2.TPM and MDCR_EL2.TPMCR cause trapping of PMCR. +*/ /* We could trap ID_DFR0 and tell the guest we don't support * performance monitoring, but Linux doesn't check the ID_DFR0. * Therefore it will read PMCR.
@@ -1675,7 +1693,6 @@ static void do_cp15_32(struct cpu_user_regs *regs,return handle_ro_raz (regs, r, cp32.read, hsr,0 ); else return handle_raz_wi(regs, r, cp32.read, hsr, 1); -case HSR_CPREG32 (PMINTENSET ):case HSR_CPREG32 (PMINTENCLR ):/* EL1 only, however MDCR_EL2.TPM==1 means EL0 may trap here also. */
@@ -2057,8 +2074,22 @@ static void do_sysreg(struct cpu_user_regs *regs, HSR_SYSREG_DBG_CASES(DBGWCR):return handle_raz_wi (regs, x, hsr.sysreg.read, hsr,1 ); -/* RAZ/WI registers: */ -/* - Perf monitors */ +/* +* MDCR_EL2.TPM +* +* ARMv8 (DDI 0487A.d): D1-1511 Table D1-61 +* +* Unhandled: +* PMEVCNTR<n>_EL0 +* PMEVTYPER<n>_EL0 +* PMCCFILTR_EL0 +* MDCR_EL2.TPMCR +* +* ARMv7 (DDI 0406C.b): B1.14.17 +* ARMv8 (DDI 0487A.d): D1-1511 Table D1-62 +* +* NB: Both MDCR_EL2.TPM and MDCR_EL2.TPMCR cause trapping of PMCR. +*/ case HSR_SYSREG_PMINTENSET_EL1 :case HSR_SYSREG_PMINTENCLR_EL1 :/*
@@ -2066,8 +2097,6 @@ static void do_sysreg(struct cpu_user_regs *regs,* undef. */ return handle_raz_wi (regs, x, hsr.sysreg.read, hsr,1 ); - -/* - Perf monitors */ case HSR_SYSREG_PMUSERENR_EL0 :/* RO at EL0. RAZ/WI at EL1 */ if (psr_mode_is_user (regs) )
Signature · 1 line
1.7.10.4
PATCH 17/19 xen: arm: Remove CNTPCT_EL0 trap handling.
In reply to Ian CampbellWe set CNTHCTL_EL2.EL1PCTEN and therefore according to ARMv8 (DDI 0487A.d) D1-1510 Table D1-60 we are not trapping this.
--- xen/arch/arm/traps.c | 1 - xen/arch/arm/vtimer.c | 30 ------------------------------ 2 files changed, 31 deletions(-)
xen/arch/arm/traps.c +0 -1
@@ -1757,7 +1757,6 @@ static void do_cp15_64(struct cpu_user_regs *regs,switch ( hsr.bits &HSR_CP64_REGS_MASK ) { -case HSR_CPREG64 (CNTPCT ):case HSR_CPREG64 (CNTP_CVAL ):if ( !vtimer_emulate (regs, hsr) )return inject_undef_exception (regs, hsr);
xen/arch/arm/vtimer.c +0 -30
@@ -243,28 +243,6 @@ static int vtimer_cntp_cval(struct cpu_user_regs *regs, uint64_t *r, int read) }return 1 ; } -static int vtimer_cntpct (struct cpu_user_regs *regs,uint64_t *r,int read) -{ -struct vcpu *v = current; -uint64_t ticks; -s_time_t now; - -if ( read ) - { -if ( !ACCESS_ALLOWED (regs,EL0PCTEN ) ) -return 0 ; - now =NOW () - v->domain->arch.phys_timer_base.offset; - ticks =ns_to_ticks (now); - *r = ticks; -return 1 ; - } -else - { -gprintk (XENLOG_DEBUG ,"WRITE to R/O CNTPCT\n" ); -return 0 ; - } -} -static int vtimer_emulate_cp32 (struct cpu_user_regs *regs,union hsr hsr) {
@@ -303,11 +281,6 @@ static int vtimer_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr)switch ( hsr.bits &HSR_CP64_REGS_MASK ) { -case HSR_CPREG64 (CNTPCT ): -if ( !vtimer_cntpct (regs, &x, cp64.read) ) -return 0 ; -break ; -case HSR_CPREG64 (CNTP_CVAL ):if ( !vtimer_cntp_cval (regs, &x, cp64.read) )return 0 ;
@@ -356,9 +329,6 @@ static int vtimer_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)case HSR_SYSREG_CNTP_CVAL_EL0 :return vtimer_cntp_cval (regs, x, sysreg.read); -case HSR_SYSREG_CNTPCT_EL0 : -return vtimer_cntpct (regs, x, sysreg.read); -default :return 0 ; }
Signature · 1 line
1.7.10.4
Re: [PATCH 17/19] xen: arm: Remove CNTPCT_EL0 trap handling.
Hi Ian,
Quoted 4 lines · Ian Campbell
We set CNTHCTL_EL2.EL1PCTEN and therefore according to ARMv8 (DDI 0487A.d) D1-1510 Table D1-60 we are not trapping this.
Signed-off-by Ian Campbell <[email protected]>
Regards,
Signature · 1 line
Julien Grall
PATCH 18/19 xen: arm: Annotate registers trapped when CNTHCTL_EL2.EL1PCEN == 0
In reply to Ian Campbell--- xen/arch/arm/traps.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
xen/arch/arm/traps.c +18 -2
@@ -1642,6 +1642,12 @@ static void do_cp15_32(struct cpu_user_regs *regs,switch ( hsr.bits &HSR_CP32_REGS_MASK ) { +/* +* !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN +* +* ARMv7 (DDI 0406C.b): B4.1.22 +* ARMv8 (DDI 0487A.d): D1-1510 Table D1-60 +*/ case HSR_CPREG32 (CNTP_CTL ):case HSR_CPREG32 (CNTP_TVAL ):if ( !vtimer_emulate (regs, hsr) )
@@ -1757,6 +1763,12 @@ static void do_cp15_64(struct cpu_user_regs *regs,switch ( hsr.bits &HSR_CP64_REGS_MASK ) { +/* +* !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN +* +* ARMv7 (DDI 0406C.b): B4.1.22 +* ARMv8 (DDI 0487A.d): D1-1510 Table D1-60 +*/ case HSR_CPREG64 (CNTP_CVAL ):if ( !vtimer_emulate (regs, hsr) )return inject_undef_exception (regs, hsr);
@@ -2120,14 +2132,18 @@ static void do_sysreg(struct cpu_user_regs *regs,*/ return handle_raz_wi (regs, x, hsr.sysreg.read, hsr,1 ); -/* Write only, Write ignore registers: */ - +/* +* !CNTHCTL_EL2.EL1PCEN +* +* ARMv8 (DDI 0487A.d): D1-1510 Table D1-60 +*/ case HSR_SYSREG_CNTP_CTL_EL0 :case HSR_SYSREG_CNTP_TVAL_EL0 :case HSR_SYSREG_CNTP_CVAL_EL0 :if ( !vtimer_emulate (regs, hsr) )return inject_undef_exception (regs, hsr);break ; +case HSR_SYSREG_ICC_SGI1R_EL1 :if ( !vgic_emulate (regs, hsr) ) {
Signature · 1 line
1.7.10.4
Re: [PATCH 18/19] xen: arm: Annotate registers trapped when CNTHCTL_EL2.EL1PCEN == 0
Hi Ian,
Quoted 10 lines · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)xen/arch/arm/traps.c +0
-0
xen/arch/arm/traps.c static void do_cp15_32(struct cpu_user_regs *regs, · line 1646
Quoted 5 lines
xen/arch/arm/traps.c +2
-0@@ -1642,6 +1642,12 @@ static void do_cp15_32(struct cpu_user_regs *regs,switch ( hsr.bits &HSR_CP32_REGS_MASK ) { +/* +* !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
I will be picky. The listing here is ARMv8 (AArch64) / ARMv7, but below it's ARMv7 / ARMv8.
Quoted 21 lines
xen/arch/arm/traps.c +10
-0+* +* ARMv7 (DDI 0406C.b): B4.1.22 +* ARMv8 (DDI 0487A.d): D1-1510 Table D1-60 +*/ case HSR_CPREG32 (CNTP_CTL ):case HSR_CPREG32 (CNTP_TVAL ):if ( !vtimer_emulate (regs, hsr) )@@ -1757,6 +1763,12 @@ static void do_cp15_64(struct cpu_user_regs *regs,switch ( hsr.bits &HSR_CP64_REGS_MASK ) { +/* +* !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN +* +* ARMv7 (DDI 0406C.b): B4.1.22 +* ARMv8 (DDI 0487A.d): D1-1510 Table D1-60 +*/ case HSR_CPREG64 (CNTP_CVAL ):if ( !vtimer_emulate (regs, hsr) )return inject_undef_exception (regs, hsr);
xen/arch/arm/traps.c static void do_sysreg(struct cpu_user_regs *regs, · line 2124
Quoted 5 lines
xen/arch/arm/traps.c +0
-2@@ -2120,14 +2132,18 @@ static void do_sysreg(struct cpu_user_regs *regs,*/ return handle_raz_wi (regs, x, hsr.sysreg.read, hsr,1 ); -/* Write only, Write ignore registers: */ -
This comment should have been dropped in patch #14.
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH 18/19] xen: arm: Annotate registers trapped when CNTHCTL_EL2.EL1PCEN == 0
Quoted 14 lines · Julien Grall
Hi Ian,
Quoted 11 lines · Ian Campbell
Signed-off-by Ian Campbell <[email protected]>--- xen/arch/arm/traps.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)xen/arch/arm/traps.c +0
-0@@ -1642,6 +1642,12 @@ static void do_cp15_32(struct cpu_user_regs *regs,
Quoted 7 lines
xen/arch/arm/traps.c static void do_cp15_32(struct cpu_user_regs *regs, · line 1646
Quoted 4 lines
xen/arch/arm/traps.c +2
-0switch ( hsr.bits &HSR_CP32_REGS_MASK ) { +/* +* !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN I will be picky. The listing here is ARMv8 (AArch64) / ARMv7, but below it's ARMv7 / ARMv8.
I'm not spotting the "below", which one did you mean?
I was trying to use the AArch64 names only when they were reasonably similar (i.e. just differing in H prefix vs _EL2 suffix or otherwise fairly obvious) and only use both names if there was some potentially confusing difference in the naming.
Quoted 25 lines
Quoted 25 lines
xen/arch/arm/traps.c +10
-0+* +* ARMv7 (DDI 0406C.b): B4.1.22 +* ARMv8 (DDI 0487A.d): D1-1510 Table D1-60 +*/ case HSR_CPREG32 (CNTP_CTL ):case HSR_CPREG32 (CNTP_TVAL ):if ( !vtimer_emulate (regs, hsr) )@@ -1757,6 +1763,12 @@ static void do_cp15_64(struct cpu_user_regs *regs,switch ( hsr.bits &HSR_CP64_REGS_MASK ) { +/* +* !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN +* +* ARMv7 (DDI 0406C.b): B4.1.22 +* ARMv8 (DDI 0487A.d): D1-1510 Table D1-60 +*/ case HSR_CPREG64 (CNTP_CVAL ):if ( !vtimer_emulate (regs, hsr) )return inject_undef_exception (regs, hsr);@@ -2120,14 +2132,18 @@ static void do_sysreg(struct cpu_user_regs *regs,*/ return handle_raz_wi (regs, x, hsr.sysreg.read, hsr,1 );
Quoted 4 lines
xen/arch/arm/traps.c static void do_sysreg(struct cpu_user_regs *regs, · line 2124
Quoted 2 lines
xen/arch/arm/traps.c +0
-2-/* Write only, Write ignore registers: */ -This comment should have been dropped in patch #14.
Moved.
Ian.
PATCH 19/19 xen: arm: Annotate source of ICC SGI register trapping
In reply to Ian CampbellI was unable to find an ARMv8 ARM reference to this, so refer to the GIC Architecture Specification instead.
ARMv8 ARM does cover other ways of trapping these accesses via ICH_HCR_EL2 but we don't use those and they trap additional registers as well.
--- xen/arch/arm/traps.c | 6 ++++++ 1 file changed, 6 insertions(+)
xen/arch/arm/traps.c +6 -0
@@ -2144,6 +2144,12 @@ static void do_sysreg(struct cpu_user_regs *regs,return inject_undef_exception (regs, hsr);break ; +/* +* HCR_EL2.FMO or HCR_EL2.IMO +* +* ARMv8: GIC Architecture Specification (PRD03-GENC-010745 24.0) +* Section 4.6.8. +*/ case HSR_SYSREG_ICC_SGI1R_EL1 :if ( !vgic_emulate (regs, hsr) ) {
Signature · 1 line
1.7.10.4
Re: [PATCH 19/19] xen: arm: Annotate source of ICC SGI register trapping
Hi Ian,
Quoted 8 lines · Ian Campbell
I was unable to find an ARMv8 ARM reference to this, so refer to the GIC Architecture Specification instead.
ARMv8 ARM does cover other ways of trapping these accesses via
ICH_HCR_EL2but we don't use those and they trap additional registers as well.Signed-off-by Ian Campbell <[email protected]>
Regards,
Signature · 1 line
Julien Grall