Hi Bertrand,
On Tue, Dec 9, 2025 at 11:11 AM Bertrand Marquis
<[email protected]> wrote:
>
> 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.
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
>
>
