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

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Tue, Dec 9, 2025 at 11:41 AM Jens Wiklander
<jens.wiklander@xxxxxxxxxx> wrote:
>
> Hi Bertrand,
>
> On Tue, Dec 9, 2025 at 11:11 AM Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx> wrote:
> >
> > 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.

It's not needed since this field is initialized before it can be found
in the list. I'm OK with either way.

Reviewed-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>

Cheers,
Jens

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