Re: [Xen-devel] [PATCH v2 09/15] xen/arm: psci: Detect SMCCC version

[Julien Grall <julien.grall@xxxxxxx>]

On 12/02/18 14:43, Volodymyr Babchuk wrote:
> Hi Julien,
>
> On 09.02.18 19:09, Julien Grall wrote:
> > On 02/09/2018 05:04 PM, Volodymyr Babchuk wrote:
> > > Julien,
> > >
> > >
> > > On 08.02.18 21:21, Julien Grall wrote:
> > > > PSCI 1.0 and later allows the SMCCC version to be (indirectly) probed
> > > > via PSCI_FEATURES. If the PSCI_FEATURES does not exist (PSCI 0.2 or
> > > > earlier) and the function return an error, then we considered SMCCC 1.0
> > > > is implemented.
> > > >
> > > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > > ---
> > > >      Changes in v2:
> > > >          - Patch added
> > > > ---
> > > >   xen/arch/arm/psci.c         | 34 +++++++++++++++++++++++++++++++++-
> > > >   xen/include/asm-arm/smccc.h |  5 ++++-
> > > >   2 files changed, 37 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> > > I find it strange to determine SMCCC version in PSCI code. psci.c is 
> > > not the first place, where I will look for SMCCC version discovery.
> > > I think it is better to add smccc.c, where such functions can reside.
> > SMCCC version discovery is based on PSCI, hence it is in the PSCI 
> > code. I can't see a good reason to create a file with 3 lines at the 
> > moment.
> SMCCC version discovery is a Arm Architecture Service function. PSCI 
> used to discover if this function is supported at all. Dubious 
> architectural solution from my point of view. But it is already done...
> We had similar discussions about introducing new files earlier, so you 
> know  my point. I would like to see clean codebase where one can 
> navigate without grep/cscope. I see no point, why function that calls 
> Arm architecture service to identify SMCCC version should reside in PSCI 
> code.
Good luck for navigating without grep/cscope in a such a big code base.

> Besides, that file will have more than 3 lines at the moment. Your 
> current psci_init_smccc is longer right now :)
You got my point with "3 lines"... It is one function in one file. There 
limited reason to create a file for that, more that I clearly don't want 
to expose psci_features() outside of psci.c for now.
We can revisit this in the future if we need to discover SMCCC in a 
different way.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel