| Message ID | 20190723213553.22300-4-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: Xen hardening for newer Armv8 | expand |
Julien Grall writes: > psr_mode_is_32bit() prototype does not match the rest of the helpers for > the process state. Looking at the callers, most of them will access > struct cpu_user_regs just for calling psr_mode_is_32bit(). > > The macro is now reworked to take a struct cpu_user_regs in parameter. > At the same time take the opportunity to switch to a static inline > helper. I'm a bit concerned about naming now. As psr_mode_is_32bit() is now have no psr parameter, and ARM ARM uses term "state" instead of "mode", maybe it is worth to rename this helper to something like "is_32bit_state"? > Lastly, when compiled for 32-bit, Xen will only support 32-bit guest. So > it is pointless to check whether the register state correspond to 64-bit > or not. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/traps.c | 28 ++++++++++++++-------------- > xen/include/asm-arm/regs.h | 9 ++++++++- > 2 files changed, 22 insertions(+), 15 deletions(-) > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index 111a2029e6..54e66a86d0 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -919,7 +919,7 @@ static void _show_registers(const struct cpu_user_regs *regs, > #ifdef CONFIG_ARM_64 > else if ( is_64bit_domain(v->domain) ) > { > - if ( psr_mode_is_32bit(regs->cpsr) ) > + if ( psr_mode_is_32bit(regs) ) > { > BUG_ON(!usr_mode(regs)); > show_registers_32(regs, ctxt, guest_mode, v); > @@ -1625,7 +1625,7 @@ int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr) > { > unsigned long it; > > - BUG_ON( !psr_mode_is_32bit(regs->cpsr) || !(cpsr&PSR_THUMB) ); > + BUG_ON( !psr_mode_is_32bit(regs) || !(cpsr & PSR_THUMB) ); > > it = ( (cpsr >> (10-2)) & 0xfc) | ((cpsr >> 25) & 0x3 ); > > @@ -1650,7 +1650,7 @@ int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr) > void advance_pc(struct cpu_user_regs *regs, const union hsr hsr) > { > unsigned long itbits, cond, cpsr = regs->cpsr; > - bool is_thumb = psr_mode_is_32bit(cpsr) && (cpsr & PSR_THUMB); > + bool is_thumb = psr_mode_is_32bit(regs) && (cpsr & PSR_THUMB); > > if ( is_thumb && (cpsr & PSR_IT_MASK) ) > { > @@ -2078,32 +2078,32 @@ void do_trap_guest_sync(struct cpu_user_regs *regs) > advance_pc(regs, hsr); > break; > case HSR_EC_CP15_32: > - GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); > + GUEST_BUG_ON(!psr_mode_is_32bit(regs)); > perfc_incr(trap_cp15_32); > do_cp15_32(regs, hsr); > break; > case HSR_EC_CP15_64: > - GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); > + GUEST_BUG_ON(!psr_mode_is_32bit(regs)); > perfc_incr(trap_cp15_64); > do_cp15_64(regs, hsr); > break; > case HSR_EC_CP14_32: > - GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); > + GUEST_BUG_ON(!psr_mode_is_32bit(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)); > + GUEST_BUG_ON(!psr_mode_is_32bit(regs)); > 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)); > + GUEST_BUG_ON(!psr_mode_is_32bit(regs)); > perfc_incr(trap_cp14_dbg); > do_cp14_dbg(regs, hsr); > break; > case HSR_EC_CP: > - GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); > + GUEST_BUG_ON(!psr_mode_is_32bit(regs)); > perfc_incr(trap_cp); > do_cp(regs, hsr); > break; > @@ -2114,7 +2114,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs) > * 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)); > + GUEST_BUG_ON(!psr_mode_is_32bit(regs)); > perfc_incr(trap_smc32); > do_trap_smc(regs, hsr); > break; > @@ -2122,7 +2122,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs) > { > register_t nr; > > - GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); > + GUEST_BUG_ON(!psr_mode_is_32bit(regs)); > perfc_incr(trap_hvc32); > #ifndef NDEBUG > if ( (hsr.iss & 0xff00) == 0xff00 ) > @@ -2137,7 +2137,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs) > } > #ifdef CONFIG_ARM_64 > case HSR_EC_HVC64: > - GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr)); > + GUEST_BUG_ON(psr_mode_is_32bit(regs)); > perfc_incr(trap_hvc64); > #ifndef NDEBUG > if ( (hsr.iss & 0xff00) == 0xff00 ) > @@ -2153,12 +2153,12 @@ void do_trap_guest_sync(struct cpu_user_regs *regs) > * > * ARMv8 (DDI 0487A.d): D1-1501 Table D1-44 > */ > - GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr)); > + GUEST_BUG_ON(psr_mode_is_32bit(regs)); > perfc_incr(trap_smc64); > do_trap_smc(regs, hsr); > break; > case HSR_EC_SYSREG: > - GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr)); > + GUEST_BUG_ON(psr_mode_is_32bit(regs)); > perfc_incr(trap_sysreg); > do_sysreg(regs, hsr); > break; > diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h > index ddc6eba9ce..0e3e56b452 100644 > --- a/xen/include/asm-arm/regs.h > +++ b/xen/include/asm-arm/regs.h > @@ -13,7 +13,14 @@ > > #define psr_mode(psr,m) (((psr) & PSR_MODE_MASK) == m) > > -#define psr_mode_is_32bit(psr) !!((psr) & PSR_MODE_BIT) > +static inline bool psr_mode_is_32bit(const struct cpu_user_regs *regs) > +{ > +#ifdef CONFIG_ARM_32 > + return true; > +#else > + return !!(regs->cpsr & PSR_MODE_BIT); > +#endif > +} > > #define usr_mode(r) psr_mode((r)->cpsr,PSR_MODE_USR) > #define fiq_mode(r) psr_mode((r)->cpsr,PSR_MODE_FIQ)
Hi, On 26/07/2019 13:31, Volodymyr Babchuk wrote: > > Julien Grall writes: > >> psr_mode_is_32bit() prototype does not match the rest of the helpers for >> the process state. Looking at the callers, most of them will access >> struct cpu_user_regs just for calling psr_mode_is_32bit(). >> >> The macro is now reworked to take a struct cpu_user_regs in parameter. >> At the same time take the opportunity to switch to a static inline >> helper. > I'm a bit concerned about naming now. As psr_mode_is_32bit() is now have > no psr parameter, and ARM ARM uses term "state" instead of "mode", maybe > it is worth to rename this helper to something like "is_32bit_state"? It really depends how you see it. The bit is part of the "mode" field, so technically we are checking whether the mode corresponds to a 32-bit one or not. This is also inline with the rest of the helpers within this header. I would be willing to consider renaming the helper to regs_mode_is_32bit(). On a side note, Linux is using exactly the same term (see vcpu_mode_is_32bit). Cheers,
Julien Grall writes: > Hi, > > On 26/07/2019 13:31, Volodymyr Babchuk wrote: >> >> Julien Grall writes: >> >>> psr_mode_is_32bit() prototype does not match the rest of the helpers for >>> the process state. Looking at the callers, most of them will access >>> struct cpu_user_regs just for calling psr_mode_is_32bit(). >>> >>> The macro is now reworked to take a struct cpu_user_regs in parameter. >>> At the same time take the opportunity to switch to a static inline >>> helper. >> I'm a bit concerned about naming now. As psr_mode_is_32bit() is now have >> no psr parameter, and ARM ARM uses term "state" instead of "mode", maybe >> it is worth to rename this helper to something like "is_32bit_state"? > > It really depends how you see it. The bit is part of the "mode" field, > so technically we are checking whether the mode corresponds to a > 32-bit one or not. This is also inline with the rest of the helpers > within this header. > > I would be willing to consider renaming the helper to regs_mode_is_32bit(). I'm fine with this name.
On Fri, 26 Jul 2019, Volodymyr Babchuk wrote: > Julien Grall writes: > > > Hi, > > > > On 26/07/2019 13:31, Volodymyr Babchuk wrote: > >> > >> Julien Grall writes: > >> > >>> psr_mode_is_32bit() prototype does not match the rest of the helpers for > >>> the process state. Looking at the callers, most of them will access > >>> struct cpu_user_regs just for calling psr_mode_is_32bit(). > >>> > >>> The macro is now reworked to take a struct cpu_user_regs in parameter. > >>> At the same time take the opportunity to switch to a static inline > >>> helper. > >> I'm a bit concerned about naming now. As psr_mode_is_32bit() is now have > >> no psr parameter, and ARM ARM uses term "state" instead of "mode", maybe > >> it is worth to rename this helper to something like "is_32bit_state"? > > > > It really depends how you see it. The bit is part of the "mode" field, > > so technically we are checking whether the mode corresponds to a > > 32-bit one or not. This is also inline with the rest of the helpers > > within this header. > > > > I would be willing to consider renaming the helper to regs_mode_is_32bit(). > I'm fine with this name. The patch is fine by me, as is, or with the name changed to regs_mode_is_32bit. (I didn't commit it.) Either way: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Hi Stefano, On 29/07/2019 22:52, Stefano Stabellini wrote: > On Fri, 26 Jul 2019, Volodymyr Babchuk wrote: >> Julien Grall writes: >> >>> Hi, >>> >>> On 26/07/2019 13:31, Volodymyr Babchuk wrote: >>>> >>>> Julien Grall writes: >>>> >>>>> psr_mode_is_32bit() prototype does not match the rest of the helpers for >>>>> the process state. Looking at the callers, most of them will access >>>>> struct cpu_user_regs just for calling psr_mode_is_32bit(). >>>>> >>>>> The macro is now reworked to take a struct cpu_user_regs in parameter. >>>>> At the same time take the opportunity to switch to a static inline >>>>> helper. >>>> I'm a bit concerned about naming now. As psr_mode_is_32bit() is now have >>>> no psr parameter, and ARM ARM uses term "state" instead of "mode", maybe >>>> it is worth to rename this helper to something like "is_32bit_state"? >>> >>> It really depends how you see it. The bit is part of the "mode" field, >>> so technically we are checking whether the mode corresponds to a >>> 32-bit one or not. This is also inline with the rest of the helpers >>> within this header. >>> >>> I would be willing to consider renaming the helper to regs_mode_is_32bit(). >> I'm fine with this name. > > The patch is fine by me, as is, or with the name changed to > regs_mode_is_32bit. (I didn't commit it.) I am thinking to get the renaming separately. So I can also rework the other helpers. > > Either way: > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> So I will commit as is and add in my todo list renaming. Cheers,
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 111a2029e6..54e66a86d0 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -919,7 +919,7 @@ static void _show_registers(const struct cpu_user_regs *regs, #ifdef CONFIG_ARM_64 else if ( is_64bit_domain(v->domain) ) { - if ( psr_mode_is_32bit(regs->cpsr) ) + if ( psr_mode_is_32bit(regs) ) { BUG_ON(!usr_mode(regs)); show_registers_32(regs, ctxt, guest_mode, v); @@ -1625,7 +1625,7 @@ int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr) { unsigned long it; - BUG_ON( !psr_mode_is_32bit(regs->cpsr) || !(cpsr&PSR_THUMB) ); + BUG_ON( !psr_mode_is_32bit(regs) || !(cpsr & PSR_THUMB) ); it = ( (cpsr >> (10-2)) & 0xfc) | ((cpsr >> 25) & 0x3 ); @@ -1650,7 +1650,7 @@ int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr) void advance_pc(struct cpu_user_regs *regs, const union hsr hsr) { unsigned long itbits, cond, cpsr = regs->cpsr; - bool is_thumb = psr_mode_is_32bit(cpsr) && (cpsr & PSR_THUMB); + bool is_thumb = psr_mode_is_32bit(regs) && (cpsr & PSR_THUMB); if ( is_thumb && (cpsr & PSR_IT_MASK) ) { @@ -2078,32 +2078,32 @@ void do_trap_guest_sync(struct cpu_user_regs *regs) advance_pc(regs, hsr); break; case HSR_EC_CP15_32: - GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); + GUEST_BUG_ON(!psr_mode_is_32bit(regs)); perfc_incr(trap_cp15_32); do_cp15_32(regs, hsr); break; case HSR_EC_CP15_64: - GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); + GUEST_BUG_ON(!psr_mode_is_32bit(regs)); perfc_incr(trap_cp15_64); do_cp15_64(regs, hsr); break; case HSR_EC_CP14_32: - GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); + GUEST_BUG_ON(!psr_mode_is_32bit(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)); + GUEST_BUG_ON(!psr_mode_is_32bit(regs)); 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)); + GUEST_BUG_ON(!psr_mode_is_32bit(regs)); perfc_incr(trap_cp14_dbg); do_cp14_dbg(regs, hsr); break; case HSR_EC_CP: - GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); + GUEST_BUG_ON(!psr_mode_is_32bit(regs)); perfc_incr(trap_cp); do_cp(regs, hsr); break; @@ -2114,7 +2114,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs) * 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)); + GUEST_BUG_ON(!psr_mode_is_32bit(regs)); perfc_incr(trap_smc32); do_trap_smc(regs, hsr); break; @@ -2122,7 +2122,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs) { register_t nr; - GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); + GUEST_BUG_ON(!psr_mode_is_32bit(regs)); perfc_incr(trap_hvc32); #ifndef NDEBUG if ( (hsr.iss & 0xff00) == 0xff00 ) @@ -2137,7 +2137,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs) } #ifdef CONFIG_ARM_64 case HSR_EC_HVC64: - GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr)); + GUEST_BUG_ON(psr_mode_is_32bit(regs)); perfc_incr(trap_hvc64); #ifndef NDEBUG if ( (hsr.iss & 0xff00) == 0xff00 ) @@ -2153,12 +2153,12 @@ void do_trap_guest_sync(struct cpu_user_regs *regs) * * ARMv8 (DDI 0487A.d): D1-1501 Table D1-44 */ - GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr)); + GUEST_BUG_ON(psr_mode_is_32bit(regs)); perfc_incr(trap_smc64); do_trap_smc(regs, hsr); break; case HSR_EC_SYSREG: - GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr)); + GUEST_BUG_ON(psr_mode_is_32bit(regs)); perfc_incr(trap_sysreg); do_sysreg(regs, hsr); break; diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h index ddc6eba9ce..0e3e56b452 100644 --- a/xen/include/asm-arm/regs.h +++ b/xen/include/asm-arm/regs.h @@ -13,7 +13,14 @@ #define psr_mode(psr,m) (((psr) & PSR_MODE_MASK) == m) -#define psr_mode_is_32bit(psr) !!((psr) & PSR_MODE_BIT) +static inline bool psr_mode_is_32bit(const struct cpu_user_regs *regs) +{ +#ifdef CONFIG_ARM_32 + return true; +#else + return !!(regs->cpsr & PSR_MODE_BIT); +#endif +} #define usr_mode(r) psr_mode((r)->cpsr,PSR_MODE_USR) #define fiq_mode(r) psr_mode((r)->cpsr,PSR_MODE_FIQ)
psr_mode_is_32bit() prototype does not match the rest of the helpers for the process state. Looking at the callers, most of them will access struct cpu_user_regs just for calling psr_mode_is_32bit(). The macro is now reworked to take a struct cpu_user_regs in parameter. At the same time take the opportunity to switch to a static inline helper. Lastly, when compiled for 32-bit, Xen will only support 32-bit guest. So it is pointless to check whether the register state correspond to 64-bit or not. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/traps.c | 28 ++++++++++++++-------------- xen/include/asm-arm/regs.h | 9 ++++++++- 2 files changed, 22 insertions(+), 15 deletions(-)