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