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@arm.com>
---
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);
+
         /*
          * 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

Hi Bertrand,

On Fri, Dec 5, 2025 at 11:37 AM Bertrand Marquis
<bertrand.marquis@arm.com> 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@arm.com>
> ---
> 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?

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
>

Hi Jens,

> On 9 Dec 2025, at 10:05, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> 
> Hi Bertrand,
> 
> On Fri, Dec 5, 2025 at 11:37 AM Bertrand Marquis
> <bertrand.marquis@arm.com> 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@arm.com>
>> ---
>> 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

Hi Bertrand,

On Tue, Dec 9, 2025 at 11:11 AM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi Jens,
>
> > On 9 Dec 2025, at 10:05, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi Bertrand,
> >
> > On Fri, Dec 5, 2025 at 11:37 AM Bertrand Marquis
> > <bertrand.marquis@arm.com> 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@arm.com>
> >> ---
> >> 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.

Got it, thanks for the explanation.

Cheers,
Jens

>
> 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
>
>