Re: [Xen-devel] [PATCH v4 08/11] arm: PSCI: use definitions provided by asm/smccc.h

[Julien Grall <julien.grall@xxxxxxx>]

On 25/08/17 12:00, Volodymyr Babchuk wrote:
> Hi,
>
> On 24.08.17 20:22, Julien Grall wrote:
> > Hi Volodymyr,
> >
> > On 21/08/17 21:27, Volodymyr Babchuk wrote:
> > > smccc.h provides definitions to construct SMC call function number 
> > > according
> > > to SMCCC. We don't need multiple definitions for one thing, and 
> > > definitions
> > > in smccc.h are more generic than ones used in psci.h.
> > >
> > > So psci.h will only provide function codes, while whole SMC function
> > > identifier will be constructed using generic macros from smccc.h.
> > >
> > > PSCI_0_2_FN_xxx was deliberately renamed to PSCI_0_2_FUNC_xxx, 
> > > because this
> > > is a new entity. It can lead to problems, if we'll just change value of
> > > PSCI_0_2_FN_xxx without renaming it.
> > I don't think "new entity" is a good reason to rename them. And the 
> > previous naming was kind of nice to read. More that you still use 
> > PSCI_0_2_FN{32,64}. We should definitely stay consistent in naming.
> > So what is the exact problem? Is it because you are worry to miss some 
> > of them?
> Actually yes. That helped me to find references to PSCI code in seattle.c
But now you have inconsistent naming which is not better. I would prefer 
much prefer to keep the same name as before.
> > > diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> > > index 956d4ef..46a2fde 100644
> > > --- a/xen/arch/arm/vsmc.c
> > > +++ b/xen/arch/arm/vsmc.c
> > > @@ -102,7 +102,7 @@ static bool handle_psci_0_x(struct cpu_user_regs 
> > > *regs)
> > >  /* helper function for checking arm mode 32/64 bit */
> > >  static inline int psci_mode_check(struct domain *d, register_t fid)
> > >  {
> > > -    return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
> > > +    return is_64bit_domain(d) || !ARM_SMCCC_IS_64(fid);
> > I don't see any reason to do the renaming in psci_mode_check given 
> > that you will remove this function in the next patch.
> Yep, but in comments for the previous series you asked me to make this code
> easier to read :)
> PSCI_0_2_64BIT is dropped now, so I had to use long definitions from 
> smccc.h
If you remove the code in this series, then there are no need to review 
it. You can just mention in the commit message why you didn't clean-up 
that function.
Cheers,

--
Julien Grall

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