Re: [PATCH 02/10] xen/arm: ffa: Rework feature discovery

[Julien Grall <julien@xxxxxxx>]

Hi Bertrand,

On 24/09/2024 09:16, Bertrand Marquis wrote:
> > On 22 Sep 2024, at 11:07, Julien Grall <julien@xxxxxxx> wrote:
> >
> > Hi,
> >
> > On 19/09/2024 14:19, Bertrand Marquis wrote:
> > > Store the list of ABI we need in a list and go through the list instead
> > > of having a list of conditions inside the code.
> > > No functional change.
> > > Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> > > ---
> > >   xen/arch/arm/tee/ffa.c | 61 +++++++++++++++++++++---------------------
> > >   1 file changed, 30 insertions(+), 31 deletions(-)
> > > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > > index 7c84aa6aa43d..7ff2529b2055 100644
> > > --- a/xen/arch/arm/tee/ffa.c
> > > +++ b/xen/arch/arm/tee/ffa.c
> > > @@ -74,6 +74,24 @@
> > >   /* Negotiated FF-A version to use with the SPMC, 0 if not there or supported 
> > > */
> > >   static uint32_t __ro_after_init ffa_fw_version;
> > >   +/* List of ABI we use from the firmware */
> > > +static const uint32_t ffa_fw_feat_needed[] = {
> > > +    FFA_VERSION,
> > > +    FFA_FEATURES,
> > > +    FFA_NOTIFICATION_BITMAP_CREATE,
> > > +    FFA_NOTIFICATION_BITMAP_DESTROY,
> > > +    FFA_PARTITION_INFO_GET,
> > > +    FFA_NOTIFICATION_INFO_GET_64,
> > > +    FFA_NOTIFICATION_GET,
> > > +    FFA_RX_RELEASE,
> > > +    FFA_RXTX_MAP_64,
> > > +    FFA_RXTX_UNMAP,
> > > +    FFA_MEM_SHARE_32,
> > > +    FFA_MEM_SHARE_64,
> > > +    FFA_MEM_RECLAIM,
> > > +    FFA_MSG_SEND_DIRECT_REQ_32,
> > > +    FFA_MSG_SEND_DIRECT_REQ_64,
> > > +};
> > NIT: As we are creating an array, could be take the opportunity to provide a 
> > name for each feature (we could use macro for that)? This would make easier for 
> > the user to know which feature is missing.
> In fact those are not "features" per say but ABI we need to use with the 
> firmware so maybe i should first rename the variable to say abi instead of feat.
Thanks for the clarification! With that in mind...

> Now we could create some features out of those as in practice several ABIs are 
> needed to be able to use one feature (for example notifications support require 
> the INFO_GET and the GET).
>
> In your mind you wanted to have something like:
> "Version", FFA_VERSION
> "Direct Messages", FFA_MSG_SEND_DIRECT_REQ_32,
> "Direct Messages", FFA_MSG_SEND_DIRECT_REQ_64
>
> So that we could print a more meaningfull name or would you be ok with just printing 
> "FFA_MSG_SEND_DIRECT_REQ_32" ?
... I was more thinking about printing which ABI is missing. This is 
more helpful to the user than knowning which features will be missing.
This has also the advantage that we can use macro to generate the names.

Cheers,

--
Julien Grall