Hi Jens,
> On 9 Dec 2025, at 10:05, Jens Wiklander <[email protected]> wrote:
>
> Hi Bertrand,
>
> On Fri, Dec 5, 2025 at 11:37 AM Bertrand Marquis
> <[email protected]> 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 <[email protected]>
>> ---
>> 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
