Re: [PATCH 2/4] xen/arm: ffa: Cache SP partition info at init

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Mon, Mar 2, 2026 at 10:58 AM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Jens,
>
> > On 2 Mar 2026, at 10:32, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > Hi Bertrand,
> >
> > On Mon, Mar 2, 2026 at 9:51 AM Bertrand Marquis
> > <Bertrand.Marquis@xxxxxxx> wrote:
> >>
> >> Hi Jens,
> >>
> >>> On 27 Feb 2026, at 11:39, Jens Wiklander <jens.wiklander@xxxxxxxxxx> 
> >>> wrote:
> >>>
> >>> Hi Bertrand,
> >>>
> >>> On Wed, Feb 25, 2026 at 11:02 AM Bertrand Marquis
> >>> <bertrand.marquis@xxxxxxx> wrote:
> >>>>
> >>>> FFA_PARTITION_INFO_GET currently queries the SPMC on each call and walks 
> >>>> the
> >>>> RX buffer every time. The SP list is expected to be static, so repeated
> >>>> firmware calls and validation are unnecessary.
> >>>>
> >>>> Cache the SPMC partition-info list at init time, keeping only secure
> >>>> endpoints, and reuse the cached entries for SP count and partition-info
> >>>> responses. Initialize the VM create/destroy subscriber lists from the 
> >>>> cached
> >>>> list and free the cache on init failure.
> >>>>
> >>>> SP partition info now reflects the init-time snapshot and will not 
> >>>> change.
> >>>>
> >>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> >>>> ---
> >>>> xen/arch/arm/tee/ffa_partinfo.c | 205 +++++++++++++++++++++-----------
> >>>> 1 file changed, 138 insertions(+), 67 deletions(-)
> >>>>
[snip]
> >>>> +static bool ffa_sp_list_cache_init(const void *buf, uint32_t count,
> >>>> +                                   uint32_t fpi_size)
> >>>> +{
> >>>> +    const uint8_t *src = buf;
> >>>> +    uint32_t secure_count = 0;
> >>>> +    uint32_t n, idx = 0;
> >>>> +    bool warned = false;
> >>>> +
> >>>> +    if ( fpi_size < sizeof(struct ffa_partition_info_1_0) ||
> >>>> +         fpi_size >= FFA_PAGE_SIZE )
> >>>> +        return false;
> >>>
> >>> Would it make sense to check that fpi_size is well aligned with struct
> >>> ffa_partition_info_1_0? If it's an odd size, we'll make unaligned
> >>> accesses below with memcpy(). But perhaps that's a bit much. The SPMC
> >>> isn't supposed to provide garbage.
> >>
> >> Memcpy should prevent issues even if accesses are not aligned.
> >> If we had this test, we cannot return an error to the SPMC so we would 
> >> have to
> >> generate one to the caller. It is simpler i think to handle non-aligned as 
> >> we do not
> >> expect the SPMC to generate such a case.
> >> Tell me if you agree.
> >
> > We dereference fpi below, and depending on compiler flags and pointer
> > types, memcpy() might not be safe with unaligned pointers.
> > From 6.3.2.3 Pointers, paragraph 7, in the C standard:
> > "A pointer to an object type may be converted to a pointer to a
> > different object type. If the
> > resulting pointer is not correctly aligned for the referenced type,
> > the behavior is
> > undefined."
> >
> > I've seen past examples where the compiler optimized memcpy() in a way
> > that breaks with unaligned pointers.
> >
> > We don't expect the test above to fail, but if it does we will not use
> > the secure firmware. I think refusing unexpected sizes is even
> > simpler. It should make finding eventual errors much easier.
>
> In the ffa spec, the size can grow and this is why there is a size field.
> FF-A expect that we either ignore or copy without looking the extra
> content.
> I think we should not be dependent on any alignment so we need to
> make sure we do the copy in a way that is robust to non alignments.
>
> >
> > So my question above is whether it's worth checking that fpi_size is
> > well-aligned, or if it's so unlikely that we don't need to consider
> > it.
>
> I think we need to find a solution where we handle properly things even
> if the size is not aligned.

I agree, that's the most robust.

> I though using memcpy would protect against
> that but maybe we need to use a stronger solution to ensure that works
> even if data is unaligned.

memcpy() can be used, but the pointer values must not pass through a struct
ffa_partition_info_1_0 pointer or such.

Cheers,
Jens