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