Re: [PATCH 02/12] xen/arm: ffa: Fix MEM_SHARE NS attribute handling

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

On Fri, Feb 6, 2026 at 5:18 PM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
>
>
> > On 6 Feb 2026, at 10:28, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > Hi Bertrand,
> >
> > On Tue, Feb 3, 2026 at 6:38 PM Bertrand Marquis
> > <bertrand.marquis@xxxxxxx> wrote:
> >>
> >> The FF-A memory attribute encoding is currently a literal value (0x2f),
> >> which makes reviews and validation harder. In addition, MEM_SHARE
> >> accepts the NS (non-secure) attribute bit even though the normal world
> >> must not set it according to FF-A specification.
> >>
> >> Introduce named attribute bit masks and express FFA_NORMAL_MEM_REG_ATTR
> >> in terms of them for clarity.
> >>
> >> Reject MEM_SHARE descriptors with the NS bit set, returning
> >> INVALID_PARAMETERS to match FF-A v1.1 rules that prohibit normal world
> >> from setting this bit.
> >>
> >> Functional impact: MEM_SHARE now rejects descriptors with NS bit set,
> >> which were previously accepted but violate FF-A specification.
> >
> > To be fair, it was also rejected earlier, but with a different error code.
>
> True, will adapt the impact comment to say:
>
> Functional impact: MEM_SHARE now rejects descriptors with NS bit set
> with the right error code, INVALID_PARAMETER.
>
> Tell me if that would be ok for you and if it could be fixed on commit with
> your R-b if it is the case.

Sounds good. With that, please add:
Reviewed-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>

Cheers,
Jens

>
> >
> >>
> >> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> >> ---
> >> xen/arch/arm/tee/ffa_private.h | 17 ++++++++++++++++-
> >> xen/arch/arm/tee/ffa_shm.c     |  6 ++++++
> >> 2 files changed, 22 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/arm/tee/ffa_private.h 
> >> b/xen/arch/arm/tee/ffa_private.h
> >> index cd7ecabc7eff..b625f1c72914 100644
> >> --- a/xen/arch/arm/tee/ffa_private.h
> >> +++ b/xen/arch/arm/tee/ffa_private.h
> >> @@ -129,11 +129,26 @@
> >> #define FFA_HANDLE_HYP_FLAG             BIT(63, ULL)
> >> #define FFA_HANDLE_INVALID              0xffffffffffffffffULL
> >>
> >> +/* NS attribute was introduced in v1.1 */
> >> +#define FFA_MEM_ATTR_NS                 BIT(6, U)
> >> +
> >> +#define FFA_MEM_ATTR_TYPE_DEV           (1U << 3)
> >> +#define FFA_MEM_ATTR_TYPE_MEM           (2U << 4)
> >> +
> >> +#define FFA_MEM_ATTR_NC                 (1U << 2)
> >> +#define FFA_MEM_ATTR_WB                 (3U << 2)
> >> +
> >> +#define FFA_MEM_ATTR_NON_SHARE          (0U)
> >> +#define FFA_MEM_ATTR_OUT_SHARE          (2U)
> >> +#define FFA_MEM_ATTR_INN_SHARE          (3U)
> >> +
> >> /*
> >>  * Memory attributes: Normal memory, Write-Back cacheable, Inner shareable
> >>  * Defined in FF-A-1.1-REL0 Table 10.18 at page 175.
> >>  */
> >> -#define FFA_NORMAL_MEM_REG_ATTR         0x2fU
> >> +#define FFA_NORMAL_MEM_REG_ATTR         (FFA_MEM_ATTR_TYPE_MEM | \
> >> +                                         FFA_MEM_ATTR_WB | \
> >> +                                         FFA_MEM_ATTR_INN_SHARE)
> >> /*
> >>  * Memory access permissions: Read-write
> >>  * Defined in FF-A-1.1-REL0 Table 10.15 at page 168.
> >> diff --git a/xen/arch/arm/tee/ffa_shm.c b/xen/arch/arm/tee/ffa_shm.c
> >> index 8282bacf85d3..90800e44a86a 100644
> >> --- a/xen/arch/arm/tee/ffa_shm.c
> >> +++ b/xen/arch/arm/tee/ffa_shm.c
> >> @@ -512,6 +512,12 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
> >>     if ( ret )
> >>         goto out_unlock;
> >>
> >> +    if ( trans.mem_reg_attr & FFA_MEM_ATTR_NS )
> >> +    {
> >> +        ret = FFA_RET_INVALID_PARAMETERS;
> >> +        goto out_unlock;
> >> +    }
> >> +
> >>     if ( trans.mem_reg_attr != FFA_NORMAL_MEM_REG_ATTR )
> >>     {
> >>         ret = FFA_RET_NOT_SUPPORTED;
> >> --
> >> 2.50.1 (Apple Git-155)
> >>
> >
> > Looks good, but I think the commit message needs a small update or
> > clarification.
>
> Thanks.
>
> Cheers
> Bertrand
>
> >
> > Cheers,
> > Jens
>
>