Re: [PATCH 03/10] xen/arm: ffa: fix version negotiation

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 22 Sep 2024, at 14:25, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> NIT Typo: s/fix/Fix/ to match the other title

Ack

> 
> On 19/09/2024 14:19, Bertrand Marquis wrote:
>> Fix FFA version negotiation with the firmware to follow the
>> specification guidance more closely.
> 
> To confirm, below is based on 13.2.1 in DEN0077A, is that correct? If so, can 
> you add a link in the commit message (and maybe code).

Yes it and i will add a link and description to the commit message.

> 
>> When the firmware returns OK we can have several cases:
>> - the version requested is accepted but the firmware supports a greater
>>   one in the same major.
>> - the firmware supports a greater major version. It could still return
>>   OK even if the version requested is not accepted. Reject it.
>> - the firmware supports a lower version. It will return OK and give that
>>   version. Check if we support it and use it or reject it if we do not.
>> Adapt the code to:
>> - reject any version lower than the one we support or not with the same
>>   major version
>> - use the version returned if in our supported range (currently 1.1
>>   only)
>> - use 1.1 if the version returned is greater.
>> Also adapt the handling of version requests from VM:
>> - return an error for a different major
>> - return 1.1 for a version >= 1.1
>> - return 1.0 if 1.0 was requested
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> ---
>>  xen/arch/arm/tee/ffa.c | 38 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 30 insertions(+), 8 deletions(-)
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index 7ff2529b2055..1f602f25d097 100644
>> --- a/xen/arch/arm/tee/ffa.c
>> +++ b/xen/arch/arm/tee/ffa.c
>> @@ -141,13 +141,24 @@ static void handle_version(struct cpu_user_regs *regs)
>>      struct ffa_ctx *ctx = d->arch.tee;
>>      uint32_t vers = get_user_reg(regs, 1);
>>  -    if ( vers < FFA_VERSION_1_1 )
>> -        vers = FFA_VERSION_1_0;
>> -    else
>> -        vers = FFA_VERSION_1_1;
>> +    /**
> 
> Coding style: We are use a single '*' to start comment.

Ack

> 
>> +     * As of now we only support 1.0 or 1.1.
>> +     * For any 1.x >= 1.1 return OK with 1.1
>> +     * For 1.0 return OK with 1.0
>> +     * For anything else return an error.
>> +     */
> > +    if ( (vers >> FFA_VERSION_MAJOR_SHIFT) == FFA_MY_VERSION_MAJOR )
> > +    {> +        if ( vers < FFA_VERSION_1_1 )
>> +            vers = FFA_VERSION_1_0;
>> +        else
>> +            vers = FFA_VERSION_1_1;
> 
> I feel the logic is fragile. The first ``if`` is generic and I think it would 
> be easy to update the major version without updating handle_version(). To 
> some extend, the same problem would happen with the minor version.

so something like:
if (MAJOR(vers) == MY_MAJOR)
{
   if (MINOR(vers) < MY_MIN || MINOR(vers)>MY_MIN)
        vers = MY_VERSION
   else
        keep requested version
}

> 
> AFAICT, this is not a new issue, but as you touch the code, we should 
> probably harden it. I could settle with a BUILD_BUG_ON() to catch any change 
> of the minor/major.

i could see a BUILD_BUG_ON(MAJOR(MIN_VERSION) != MAJOR(MAX_VERSION))
Is that what you have in mind ?

> 
>>  -    ctx->guest_vers = vers;
>> -    ffa_set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
>> +        ctx->guest_vers = vers;
>> +        ffa_set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
>> +    }
>> +    else
>> +        ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>  }
>>    static void handle_msg_send_direct_req(struct cpu_user_regs *regs, 
>> uint32_t fid)
>> @@ -530,7 +541,8 @@ static bool ffa_probe(void)
>>          goto err_no_fw;
>>      }
>>  -    if ( vers < FFA_MIN_SPMC_VERSION || vers > FFA_MY_VERSION )
>> +    if ( vers < FFA_MIN_SPMC_VERSION ||
>> +              (vers >> FFA_VERSION_MAJOR_SHIFT) != FFA_MY_VERSION_MAJOR )
> 
> Coding style: the second line should be aligned with 'vers' rather than 
> indented.

Ack

> 
>>      {
>>          printk(XENLOG_ERR "ffa: Incompatible version %#x found\n", vers);
>>          goto err_no_fw;
>> @@ -542,7 +554,17 @@ static bool ffa_probe(void)
>>      printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
>>             major_vers, minor_vers);
>>  -    ffa_fw_version = vers;
>> +    /**
> 
> Coding style: We start comment with /*.

Ack

> 
>> +     * If the call succeed and the version returned is higher or equal to
>> +     * the one Xen requested, the version requested by Xen will be the one
>> +     * used. If the version returned is lower but compatible with Xen, Xen
>> +     * will use that version instead.
>> +     * A version with a different major is rejected before.
>> +     */
>> +    if ( vers > FFA_MY_VERSION )
>> +        ffa_fw_version = FFA_MY_VERSION;
>> +    else
>> +        ffa_fw_version = vers;
> 
> Looking at the code after your series (didn't check before). We don't seem to 
> use ffa_fw_version for other than checking that FFA was detected. So wouldn't 
> it be better to stop storing the version?

We are only supporting a firmware version with 1.1 at the moment but when we 
will add support for FFA version 1.2 in the next weeks this will not be true 
anymore so if this is ok with you i would rather keep it.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall