Re: [Xen-devel] [PATCH 3/7] xen/arm: Rework psr_mode_is_32bit()
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 So I will commit as is and add in my todo list renaming. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 3/7] xen/arm: Rework psr_mode_is_32bit()
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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 3/7] xen/arm: Rework psr_mode_is_32bit()
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. -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 3/7] xen/arm: Rework psr_mode_is_32bit()
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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 3/7] xen/arm: Rework psr_mode_is_32bit()
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
> ---
> 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_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
> */
> -
[Xen-devel] [PATCH 3/7] xen/arm: Rework psr_mode_is_32bit()
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
---
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_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
