[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [PATCH 4/4] xen/arm64: Emulate correctly the register {w, x}zr
On AArch64, the encoding 31 could be used to represent {x,w}sp or {x,w}zr (See C1.2.4 in ARM DDI 0486A.d). The choice will depend how the register field is interpreted by the instruction. All the instructions trapped by Xen (either via a sysreg access or data abort) interprets the encoding 31 as zr. Therefore we don't have to worry about decoding the instruction. The register zr reprents the zero register, i.e it will always return 0 and write to it is ignored. To handle properly this properties, 2 new helpers have been introduced {get,set}_user_reg to read/write a value from/to a register. All the call to select_user_reg have been replaced by these 2 helpers. Furthermore, the code to emulate encoding 31 in select_user_reg has been dropped because it was invalid. For Aarch64 context, the encoding is used for sp or zr. For AArch32 context, the ISS won't be valid for data abort from AArch32 using r15 (i.e pc) as source/destination (See D7-1881 ARM DDI 0487A.d, note the validity is more restrictive than on ARMv7). It's also not possible to use r15 in co-processor instructions. This patch fixes setting MMIO register and sysreg to a random value (actually PC) instead of zero by something like: *((volatile int*)reg) = 0; compilers tend to generate "str wzr, [xx]" here. Reported-by: Marc Zyngier <Marc.Zyngier@xxxxxxx> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx> --- xen/arch/arm/io.c | 34 +++++++++---- xen/arch/arm/traps.c | 121 +++++++++++++++++++++++++++++---------------- xen/arch/arm/vgic-v3.c | 3 +- xen/arch/arm/vtimer.c | 59 +++++++++++++++++----- xen/include/asm-arm/regs.h | 7 +-- 5 files changed, 153 insertions(+), 71 deletions(-) diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c index 7e29943..0156755 100644 --- a/xen/arch/arm/io.c +++ b/xen/arch/arm/io.c @@ -24,12 +24,19 @@ #include <asm/mmio.h> static int handle_read(const struct mmio_handler *handler, struct vcpu *v, - mmio_info_t *info, register_t *r) + mmio_info_t *info) { const struct hsr_dabt dabt = info->dabt; + struct cpu_user_regs *regs = guest_cpu_user_regs(); + /* + * Initialize to zero to avoid leaking data if there is an + * implementation error in the emulation (such as not correctly + * setting r). + */ + register_t r = 0; uint8_t size = (1 << dabt.size) * 8; - if ( !handler->ops->read(v, info, r, handler->priv) ) + if ( !handler->ops->read(v, info, &r, handler->priv) ) return 0; /* @@ -37,7 +44,7 @@ static int handle_read(const struct mmio_handler *handler, struct vcpu *v, * Note that we expect the read handler to have zeroed the bits * outside the requested access size. */ - if ( dabt.sign && (*r & (1UL << (size - 1))) ) + if ( dabt.sign && (r & (1UL << (size - 1))) ) { /* * We are relying on register_t using the same as @@ -45,21 +52,30 @@ static int handle_read(const struct mmio_handler *handler, struct vcpu *v, * code smaller. */ BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long)); - *r |= (~0UL) << size; + r |= (~0UL) << size; } + set_user_reg(regs, dabt.reg, r); + return 1; } +static int handle_write(const struct mmio_handler *handler, struct vcpu *v, + mmio_info_t *info) +{ + const struct hsr_dabt dabt = info->dabt; + struct cpu_user_regs *regs = guest_cpu_user_regs(); + + return handler->ops->write(v, info, get_user_reg(regs, dabt.reg), + handler->priv); +} + int handle_mmio(mmio_info_t *info) { struct vcpu *v = current; int i; const struct mmio_handler *handler = NULL; const struct vmmio *vmmio = &v->domain->arch.vmmio; - struct hsr_dabt dabt = info->dabt; - struct cpu_user_regs *regs = guest_cpu_user_regs(); - register_t *r = select_user_reg(regs, dabt.reg); for ( i = 0; i < vmmio->num_entries; i++ ) { @@ -74,9 +90,9 @@ int handle_mmio(mmio_info_t *info) return 0; if ( info->dabt.write ) - return handler->ops->write(v, info, *r, handler->priv); + return handle_write(handler, v, info); else - return handle_read(handler, v, info, r); + return handle_read(handler, v, info); } void register_mmio_handler(struct domain *d, diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index c49bd3f..6e2e50e 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -149,7 +149,14 @@ static void print_xen_info(void) debug_build() ? 'y' : 'n', print_tainted(taint_str)); } -register_t *select_user_reg(struct cpu_user_regs *regs, int reg) +/* + * Returns a pointer to the given register value in regs, taking the + * processor mode (CPSR) into account. + * + * Note that this function should not be used directly but via + * {get,set}_user_reg. + */ +static register_t *select_user_reg(struct cpu_user_regs *regs, int reg) { BUG_ON( !guest_mode(regs) ); @@ -207,16 +214,42 @@ register_t *select_user_reg(struct cpu_user_regs *regs, int reg) } #undef REGOFFS #else - /* In 64 bit the syndrome register contains the AArch64 register - * number even if the trap was from AArch32 mode. Except that - * AArch32 R15 (PC) is encoded as 0b11111. + /* + * On 64-bit the syndrome register contains the register index as + * viewed in AArch64 state even if the trap was from AArch32 mode. */ - if ( reg == 0x1f /* && is aarch32 guest */) - return ®s->pc; return ®s->x0 + reg; #endif } +register_t get_user_reg(struct cpu_user_regs *regs, int reg) +{ +#ifdef CONFIG_ARM_64 + /* + * For store/load and sysreg instruction, the encoding 31 always + * correspond to {w,x}zr which is the zero register. + */ + if ( reg == 31 ) + return 0; +#endif + + return *select_user_reg(regs, reg); +} + +void set_user_reg(struct cpu_user_regs *regs, int reg, register_t value) +{ +#ifdef CONFIG_ARM_64 + /* + * For store/load and sysreg instruction, the encoding 31 always + * correspond to {w,x}zr which is the zero register. + */ + if ( reg == 31 ) + return; +#endif + + *select_user_reg(regs, reg) = value; +} + static const char *decode_fsc(uint32_t fsc, int *level) { const char *msg = NULL; @@ -1240,22 +1273,19 @@ static arm_hypercall_t arm_hypercall_table[] = { #ifndef NDEBUG static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code) { - register_t *r; uint32_t reg; uint32_t domid = current->domain->domain_id; switch ( code ) { case 0xe0 ... 0xef: reg = code - 0xe0; - r = select_user_reg(regs, reg); printk("DOM%d: R%d = 0x%"PRIregister" at 0x%"PRIvaddr"\n", - domid, reg, *r, regs->pc); + domid, reg, get_user_reg(regs, reg), regs->pc); break; case 0xfd: printk("DOM%d: Reached %"PRIvaddr"\n", domid, regs->pc); break; case 0xfe: - r = select_user_reg(regs, 0); - printk("%c", (char)(*r & 0xff)); + printk("%c", (char)(get_user_reg(regs, 0) & 0xff)); break; case 0xff: printk("DOM%d: DEBUG\n", domid); @@ -1613,7 +1643,7 @@ static void advance_pc(struct cpu_user_regs *regs, const union hsr hsr) /* Read as zero and write ignore */ static void handle_raz_wi(struct cpu_user_regs *regs, - register_t *reg, + int regidx, bool_t read, const union hsr hsr, int min_el) @@ -1624,7 +1654,7 @@ static void handle_raz_wi(struct cpu_user_regs *regs, return inject_undef_exception(regs, hsr); if ( read ) - *reg = 0; + set_user_reg(regs, regidx, 0); /* else: write ignored */ advance_pc(regs, hsr); @@ -1632,7 +1662,7 @@ static void handle_raz_wi(struct cpu_user_regs *regs, /* Write only as write ignore */ static void handle_wo_wi(struct cpu_user_regs *regs, - register_t *reg, + int regidx, bool_t read, const union hsr hsr, int min_el) @@ -1651,7 +1681,7 @@ static void handle_wo_wi(struct cpu_user_regs *regs, /* Read only as read as zero */ static void handle_ro_raz(struct cpu_user_regs *regs, - register_t *reg, + int regidx, bool_t read, const union hsr hsr, int min_el) @@ -1665,7 +1695,7 @@ static void handle_ro_raz(struct cpu_user_regs *regs, return inject_undef_exception(regs, hsr); /* else: raz */ - *reg = 0; + set_user_reg(regs, regidx, 0); advance_pc(regs, hsr); } @@ -1674,7 +1704,7 @@ static void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr) { const struct hsr_cp32 cp32 = hsr.cp32; - register_t *r = select_user_reg(regs, cp32.reg); + int regidx = cp32.reg; struct vcpu *v = current; if ( !check_conditional_instr(regs, hsr) ) @@ -1707,7 +1737,7 @@ static void do_cp15_32(struct cpu_user_regs *regs, if ( psr_mode_is_user(regs) ) return inject_undef_exception(regs, hsr); if ( cp32.read ) - *r = v->arch.actlr; + set_user_reg(regs, regidx, v->arch.actlr); break; /* @@ -1739,13 +1769,13 @@ 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) ) - return handle_ro_raz(regs, r, cp32.read, hsr, 0); + return handle_ro_raz(regs, regidx, cp32.read, hsr, 0); else - return handle_raz_wi(regs, r, cp32.read, hsr, 1); + return handle_raz_wi(regs, regidx, 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. */ - return handle_raz_wi(regs, r, cp32.read, hsr, 1); + return handle_raz_wi(regs, regidx, cp32.read, hsr, 1); case HSR_CPREG32(PMCR): case HSR_CPREG32(PMCNTENSET): case HSR_CPREG32(PMCNTENCLR): @@ -1762,7 +1792,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. */ - return handle_raz_wi(regs, r, cp32.read, hsr, 1); + return handle_raz_wi(regs, regidx, cp32.read, hsr, 1); /* * HCR_EL2.TIDCP @@ -1803,6 +1833,7 @@ static void do_cp15_32(struct cpu_user_regs *regs, inject_undef_exception(regs, hsr); return; } + advance_pc(regs, hsr); } @@ -1865,7 +1896,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; - register_t *r = select_user_reg(regs, cp32.reg); + int regidx = cp32.reg; struct domain *d = current->domain; if ( !check_conditional_instr(regs, hsr) ) @@ -1887,9 +1918,9 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr) * DBGPRCR */ case HSR_CPREG32(DBGOSLAR): - return handle_wo_wi(regs, r, cp32.read, hsr, 1); + return handle_wo_wi(regs, regidx, cp32.read, hsr, 1); case HSR_CPREG32(DBGOSDLR): - return handle_raz_wi(regs, r, cp32.read, hsr, 1); + return handle_raz_wi(regs, regidx, cp32.read, hsr, 1); /* * MDCR_EL2.TDA @@ -1914,6 +1945,9 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr) * DBGOSECCR */ case HSR_CPREG32(DBGDIDR): + { + uint32_t val; + /* * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis * is set to 0, which we emulated below. @@ -1927,23 +1961,26 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr) * - Version: ARMv7 v7.1 * - Variant and Revision bits match MDIR */ - *r = (1 << 24) | (5 << 16); - *r |= ((d->arch.vpidr >> 20) & 0xf) | (d->arch.vpidr & 0xf); + val = (1 << 24) | (5 << 16); + val |= ((d->arch.vpidr >> 20) & 0xf) | (d->arch.vpidr & 0xf); + set_user_reg(regs, regidx, val); + break; + } case HSR_CPREG32(DBGDSCRINT): /* * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis * is set to 0, which we emulated below. */ - return handle_ro_raz(regs, r, cp32.read, hsr, 1); + return handle_ro_raz(regs, regidx, cp32.read, hsr, 1); case HSR_CPREG32(DBGDSCREXT): /* * 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, 1); + return handle_raz_wi(regs, regidx, cp32.read, hsr, 1); case HSR_CPREG32(DBGVCR): case HSR_CPREG32(DBGBVR0): @@ -1952,7 +1989,7 @@ 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): - return handle_raz_wi(regs, r, cp32.read, hsr, 1); + return handle_raz_wi(regs, regidx, cp32.read, hsr, 1); /* * CPTR_EL2.TTA @@ -2076,7 +2113,7 @@ static void do_cp(struct cpu_user_regs *regs, const union hsr hsr) static void do_sysreg(struct cpu_user_regs *regs, const union hsr hsr) { - register_t *x = select_user_reg(regs, hsr.sysreg.reg); + int regidx = hsr.sysreg.reg; struct vcpu *v = current; switch ( hsr.bits & HSR_SYSREG_REGS_MASK ) @@ -2090,7 +2127,7 @@ static void do_sysreg(struct cpu_user_regs *regs, if ( psr_mode_is_user(regs) ) return inject_undef_exception(regs, hsr); if ( hsr.sysreg.read ) - *x = v->arch.actlr; + set_user_reg(regs, regidx, v->arch.actlr); break; /* @@ -2099,7 +2136,7 @@ static void do_sysreg(struct cpu_user_regs *regs, * 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); + return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 1); /* * MDCR_EL2.TDOSA @@ -2111,9 +2148,9 @@ static void do_sysreg(struct cpu_user_regs *regs, * DBGPRCR_EL1 */ case HSR_SYSREG_OSLAR_EL1: - return handle_wo_wi(regs, x, hsr.sysreg.read, hsr, 1); + return handle_wo_wi(regs, regidx, hsr.sysreg.read, hsr, 1); case HSR_SYSREG_OSDLR_EL1: - return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1); + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); /* * MDCR_EL2.TDA @@ -2133,18 +2170,18 @@ static void do_sysreg(struct cpu_user_regs *regs, * DBGAUTHSTATUS_EL1 */ case HSR_SYSREG_MDSCR_EL1: - return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1); + return handle_raz_wi(regs, regidx, 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); + return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0); 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); + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); /* * MDCR_EL2.TPM @@ -2168,13 +2205,13 @@ static void do_sysreg(struct cpu_user_regs *regs, * Accessible from EL1 only, but if EL0 trap happens handle as * undef. */ - return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1); + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); case HSR_SYSREG_PMUSERENR_EL0: /* RO at EL0. RAZ/WI at EL1 */ if ( psr_mode_is_user(regs) ) - return handle_ro_raz(regs, x, hsr.sysreg.read, hsr, 0); + return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0); else - return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1); + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); case HSR_SYSREG_PMCR_EL0: case HSR_SYSREG_PMCNTENSET_EL0: case HSR_SYSREG_PMCNTENCLR_EL0: @@ -2191,7 +2228,7 @@ 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. */ - return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1); + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); /* * !CNTHCTL_EL2.EL1PCEN diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index 985e866..56f98ef 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -1305,7 +1305,6 @@ static int vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr) { struct vcpu *v = current; struct hsr_sysreg sysreg = hsr.sysreg; - register_t *r = select_user_reg(regs, sysreg.reg); ASSERT (hsr.ec == HSR_EC_SYSREG); @@ -1319,7 +1318,7 @@ static int vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr) case HSR_SYSREG_ICC_SGI1R_EL1: /* WO */ if ( !sysreg.read ) - return vgic_v3_to_sgi(v, *r); + return vgic_v3_to_sgi(v, get_user_reg(regs, sysreg.reg)); else { gprintk(XENLOG_WARNING, "Reading SGI1R_EL1 - WO register\n"); diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c index b9c0b41..f636705 100644 --- a/xen/arch/arm/vtimer.c +++ b/xen/arch/arm/vtimer.c @@ -249,32 +249,49 @@ static int vtimer_cntp_cval(struct cpu_user_regs *regs, uint64_t *r, int read) static int vtimer_emulate_cp32(struct cpu_user_regs *regs, union hsr hsr) { struct hsr_cp32 cp32 = hsr.cp32; - uint32_t *r = (uint32_t *)select_user_reg(regs, cp32.reg); + /* + * Initialize to zero to avoid leaking data if there is an + * implementation error in the emulation (such as not correctly + * setting r). + */ + uint32_t r = 0; + int res; + if ( cp32.read ) perfc_incr(vtimer_cp32_reads); else perfc_incr(vtimer_cp32_writes); + if ( !cp32.read ) + r = get_user_reg(regs, cp32.reg); + switch ( hsr.bits & HSR_CP32_REGS_MASK ) { case HSR_CPREG32(CNTP_CTL): - return vtimer_cntp_ctl(regs, r, cp32.read); + res = vtimer_cntp_ctl(regs, &r, cp32.read); + break; case HSR_CPREG32(CNTP_TVAL): - return vtimer_cntp_tval(regs, r, cp32.read); + res = vtimer_cntp_tval(regs, &r, cp32.read); + break; default: return 0; } + + if ( res && cp32.read ) + set_user_reg(regs, cp32.reg, r); + + return res; } static int vtimer_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr) { struct hsr_cp64 cp64 = hsr.cp64; - uint32_t *r1 = (uint32_t *)select_user_reg(regs, cp64.reg1); - uint32_t *r2 = (uint32_t *)select_user_reg(regs, cp64.reg2); - uint64_t x = (uint64_t)(*r1) | ((uint64_t)(*r2) << 32); + uint32_t r1 = get_user_reg(regs, cp64.reg1); + uint32_t r2 = get_user_reg(regs, cp64.reg2); + uint64_t x = (uint64_t)r1 | ((uint64_t)r2 << 32); if ( cp64.read ) perfc_incr(vtimer_cp64_reads); @@ -294,8 +311,8 @@ static int vtimer_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr) if ( cp64.read ) { - *r1 = (uint32_t)(x & 0xffffffff); - *r2 = (uint32_t)(x >> 32); + set_user_reg(regs, cp64.reg1, x & 0xffffffff); + set_user_reg(regs, cp64.reg2, x >> 32); } return 1; @@ -311,14 +328,16 @@ static int vtimer_emulate_sysreg32(struct cpu_user_regs *regs, union hsr hsr, vtimer_sysreg32_fn_t fn) { struct hsr_sysreg sysreg = hsr.sysreg; - register_t *x = select_user_reg(regs, sysreg.reg); - uint32_t r = *x; + uint32_t r = 0; int ret; + if ( !sysreg.read ) + r = get_user_reg(regs, sysreg.reg); + ret = fn(regs, &r, sysreg.read); if ( ret && sysreg.read ) - *x = r; + set_user_reg(regs, sysreg.reg, r); return ret; } @@ -327,9 +346,23 @@ static int vtimer_emulate_sysreg64(struct cpu_user_regs *regs, union hsr hsr, vtimer_sysreg64_fn_t fn) { struct hsr_sysreg sysreg = hsr.sysreg; - uint64_t *x = select_user_reg(regs, sysreg.reg); + /* + * Initialize to zero to avoid leaking data if there is an + * implementation error in the emulation (such as not correctly + * setting x). + */ + uint64_t x = 0; + int ret; + + if ( !sysreg.read ) + x = get_user_reg(regs, sysreg.reg); - return fn(regs, x, sysreg.read); + ret = fn(regs, &x, sysreg.read); + + if ( ret && sysreg.read ) + set_user_reg(regs, sysreg.reg, x); + + return ret; } static int vtimer_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr) diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h index 56d53d6..2440edb 100644 --- a/xen/include/asm-arm/regs.h +++ b/xen/include/asm-arm/regs.h @@ -50,11 +50,8 @@ #define return_reg(v) ((v)->arch.cpu_info->guest_cpu_user_regs.r0) -/* - * Returns a pointer to the given register value in regs, taking the - * processor mode (CPSR) into account. - */ -extern register_t *select_user_reg(struct cpu_user_regs *regs, int reg); +register_t get_user_reg(struct cpu_user_regs *regs, int reg); +void set_user_reg(struct cpu_user_regs *regs, int reg, register_t val); #endif -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |