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

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

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.
Besides, that file will have more than 3 lines at the moment. Your 
current psci_init_smccc is longer right now :)
> > > index 5dda35cd7c..bc7b2260e8 100644
> > > --- a/xen/arch/arm/psci.c
> > > +++ b/xen/arch/arm/psci.c
> > > @@ -37,6 +37,7 @@
> > >   #endif
> > >   uint32_t psci_ver;
> > > +uint32_t smccc_ver;
> > And this variable actually is not related to PSCI.
> See my comment above. I am not going to create a file just for 3 lines.
See my comments above :)

--
Volodymyr Babchuk

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