Re: [Xen-devel] [PATCH 1/7] arm: traps: psci: use generic register accessors

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andrew,

On 09/08/17 12:12, Andrew Cooper wrote:
> On 09/08/17 10:52, Julien Grall wrote:
> > On 08/08/17 21:37, Andrew Cooper wrote:
> > > On 08/08/2017 21:08, Volodymyr Babchuk wrote:
> > > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > > > index 6cf9ee7..ed78b36 100644
> > > > --- a/xen/arch/arm/traps.c
> > > > +++ b/xen/arch/arm/traps.c
> > > > @@ -1449,13 +1449,12 @@ static void do_debug_trap(struct
> > > > cpu_user_regs *regs, unsigned int code)
> > > >  }
> > > >  #endif
> > > >
> > > > +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
> > > > +#define PSCI_ARG(reg,n) get_user_reg(reg, n)
> > > > +
> > > >  #ifdef CONFIG_ARM_64
> > > > -#define PSCI_RESULT_REG(reg) (reg)->x0
> > > > -#define PSCI_ARG(reg,n) (reg)->x##n
> > > > -#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n &
> > > > 0x00000000FFFFFFFF )
> > > > +#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) &
> > > > 0x00000000FFFFFFFF)
> > > There is no need for the mask as well as the explicit (uint32_t) cast.
> > > I'd recommend dropping the mask entirely.
> > I want to avoid the implicit cast from 64-bit register to 32-bit that
> > Volodymyr introduced in his series.
> >
> > uint32_t pstate = get_user_reg(regs, 1);
> This is how we'd expect code to look on the x86 side, but you are the
> maintainer here, so have prerogative.
This is a bit error-prone. But less than passing directly the return as 
an argument. I.e
foo(get_user_reg(regs, 1))

And assuming foo will do the cast for you. My point of explicit size is 
avoid potential mis-usage in the code.
> > IHMO this is a call to mistake. Another solution is to provide 3 helpers
> >     - get_user_reg32(...)
> >     - get_user_reg64(...)
> >     - get_user_reg(...) -> Return the full register (32-bit on ARM32,
> > 64-bit on ARM64).
> >
> > This would at least document the return value of get_user_reg*.
> None of this is an explanation for having both an explicit uint32_t cast
> and mask hidden inside PSCI_ARG32().  They are redundant.
If you are using a macro, you need to keep at least the cast to prevent 
a user misusing the variable. The other solution is a static inline as I 
suggested above.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel