Re: [PATCH v1 03/12] xen/arm: ffa: Fix is_64bit init

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 9 Dec 2025, at 10:05, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On Fri, Dec 5, 2025 at 11:37 AM Bertrand Marquis
> <bertrand.marquis@xxxxxxx> wrote:
>> 
>> is_64bit_domain(d) is not set during domain_init as the domain field is
>> only set when loading the domain image which is done after executing
>> domain_init.
>> 
>> Fix the implementation to set is_64bit when version gets negotiated.
>> is_64bit is only used during partition_info_get once a domain is added
>> in the list of domains having ffa support. It must only be accessed when
>> the rwlock is taken (which is the case).
>> 
>> is_64bit must not be used without the rwlock taken and other places in
>> the code needing to test 64bit support of the current domain will have
>> to use calls to is_64bit_domain instead of the field from now on.
>> 
>> Fixes: 09a201605f99 ("xen/arm: ffa: Introduce VM to VM support")
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> ---
>> Changes in v1:
>> - patch introduced
>> ---
>> xen/arch/arm/tee/ffa.c         | 9 ++++++++-
>> xen/arch/arm/tee/ffa_private.h | 5 +++++
>> 2 files changed, 13 insertions(+), 1 deletion(-)
>> 
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index aadd6c21e7f2..0f6f837378cc 100644
>> --- a/xen/arch/arm/tee/ffa.c
>> +++ b/xen/arch/arm/tee/ffa.c
>> @@ -180,6 +180,14 @@ static bool ffa_negotiate_version(struct cpu_user_regs 
>> *regs)
>>             goto out_handled;
>>         }
>> 
>> +        /*
>> +         * We cannot set is_64bit during domain init because the field is 
>> not
>> +         * yet initialized.
>> +         * This field is only used during partinfo_get with the rwlock taken
>> +         * so there is no ordering issue with guest_vers.
>> +         */
>> +        ctx->is_64bit = is_64bit_domain(d);
> 
> This should only be assigned under the rwlock. But do we need the
> is_64bit field at all? Why can't we always use is_64bit_domain()
> instead?

As we take it after when needed, setting it here should be ok but i can move 
this
inside the rwlock section to be more coherent.

The field is needed when creating the list of partitions. To use 
is_64bit_domain, I
would to get access to the foreign domain description which i try to prevent to 
not
create a way for a guest to block other guests by doing partinfo_get. This is 
why
i store the information i need for foreign guests in the ctx instead of using 
RCU
to get access to the domain descriptor.

Cheers
Bertrand

> 
> Cheers,
> Jens
> 
>> +
>>         /*
>>          * A successful FFA_VERSION call does not freeze negotiation. Guests
>>          * are allowed to issue multiple FFA_VERSION attempts (e.g. probing
>> @@ -433,7 +441,6 @@ static int ffa_domain_init(struct domain *d)
>> 
>>     ctx->ffa_id = ffa_get_vm_id(d);
>>     ctx->num_vcpus = d->max_vcpus;
>> -    ctx->is_64bit = is_64bit_domain(d);
>> 
>>     /*
>>      * ffa_domain_teardown() will be called if ffa_domain_init() returns an
>> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
>> index 4e4ac7fd7bc4..2daa4589a930 100644
>> --- a/xen/arch/arm/tee/ffa_private.h
>> +++ b/xen/arch/arm/tee/ffa_private.h
>> @@ -344,6 +344,11 @@ struct ffa_ctx {
>>     /* FF-A Endpoint ID */
>>     uint16_t ffa_id;
>>     uint16_t num_vcpus;
>> +    /*
>> +     * Must only be accessed with the ffa_ctx_list_rwlock taken as it set
>> +     * when guest_vers is set and other accesses could see a partially set
>> +     * value.
>> +     */
>>     bool is_64bit;
>> 
>>     /*
>> --
>> 2.51.2