Re: [XEN PATCH v7 11/20] xen/arm: ffa: map SPMC rx/tx buffers

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Tue, Feb 28, 2023 at 1:57 PM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Jens,
>
> > On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > When initializing the FF-A mediator map the RX and TX buffers shared with
> > the SPMC.
> >
> > These buffer are later used to to transmit data that cannot be passed in
> > registers only.
> >
> > Adds a check that the SP supports the needed FF-A features
> > FFA_RXTX_MAP_64 / FFA_RXTX_MAP_32 and FFA_RXTX_UNMAP. In 64-bit mode we
> > must use FFA_RXTX_MAP_64 since registers are used to transmit the
> > physical addresses of the RX/TX buffers.
>
> Right now, FFA on 32bit would only work correctly if LPAE is not used and 
> only addresses
> under 4G are used by Xen and by guests as addresses are transferred through a 
> single register.
>
> I think that we need for now to only enable FFA support on 64bit as the 
> limitations we
> would need to enforce on 32bit are complex and the use case for FFA on 32bit 
> platforms
> is not that obvious now.

OK, I'll drop the #ifdef CONFIG_ARM_64 and #ifdef CONFIG_ARM_32 and
instead depend on ARM_64 in Kconfig.
If we ever want to use this on ARM_32 we'll have to go through
everything anyway.

>
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> > ---
> > xen/arch/arm/tee/ffa.c | 57 +++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 56 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index a5d8a12635b6..07dd5c36d54b 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -148,6 +148,15 @@ struct ffa_ctx {
> > /* Negotiated FF-A version to use with the SPMC */
> > static uint32_t ffa_version __ro_after_init;
> >
> > +/*
> > + * Our rx/tx buffers shared with the SPMC.
> > + *
> > + * ffa_page_count is the number of pages used in each of these buffers.
> > + */
> > +static void *ffa_rx __read_mostly;
> > +static void *ffa_tx __read_mostly;
> > +static unsigned int ffa_page_count __read_mostly;
> > +
> > static bool ffa_get_version(uint32_t *vers)
> > {
> >     const struct arm_smccc_1_2_regs arg = {
> > @@ -217,6 +226,17 @@ static bool check_mandatory_feature(uint32_t id)
> >     return !ret;
> > }
> >
> > +static int32_t ffa_rxtx_map(register_t tx_addr, register_t rx_addr,
> > +                            uint32_t page_count)
>
> Using register_t type here is doing an implicit cast when called and on
> 32bit this might later remove part of the address.
> This function must take paddr_t as parameters.

I'll change to paddr_t for rx/tx.

>
> > +{
> > +    uint32_t fid = FFA_RXTX_MAP_32;
> > +
> > +    if ( IS_ENABLED(CONFIG_ARM_64) )
> > +        fid = FFA_RXTX_MAP_64;
> > +
> > +    return ffa_simple_call(fid, tx_addr, rx_addr, page_count, 0);
>
> simple call might not be suitable on 32bits due to the conversion.
> As said earlier, it would make more sense to disable FFA on 32bit and
> put some comments/build_bug_on in the code in places where there
> would be something to fix.

I'm dropping the 32-bit support.

>
> > +}
> > +
> > static uint16_t get_vm_id(const struct domain *d)
> > {
> >     /* +1 since 0 is reserved for the hypervisor in FF-A */
> > @@ -384,6 +404,7 @@ static int ffa_relinquish_resources(struct domain *d)
> > static bool ffa_probe(void)
> > {
> >     uint32_t vers;
> > +    int e;
> >     unsigned int major_vers;
> >     unsigned int minor_vers;
> >
> > @@ -426,12 +447,46 @@ static bool ffa_probe(void)
> >     printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
> >            major_vers, minor_vers);
> >
> > -    if ( !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
> > +    if (
> > +#ifdef CONFIG_ARM_64
> > +         !check_mandatory_feature(FFA_RXTX_MAP_64) ||
> > +#endif
> > +#ifdef CONFIG_ARM_32
> > +         !check_mandatory_feature(FFA_RXTX_MAP_32) ||
> > +#endif
> > +         !check_mandatory_feature(FFA_RXTX_UNMAP) ||
> > +         !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
> >         return false;
> >
> > +    ffa_rx = alloc_xenheap_pages(0, 0);
> > +    if ( !ffa_rx )
> > +        return false;
> > +
> > +    ffa_tx = alloc_xenheap_pages(0, 0);
> > +    if ( !ffa_tx )
> > +        goto err_free_ffa_rx;
> > +
> > +    e = ffa_rxtx_map(__pa(ffa_tx), __pa(ffa_rx), 1);
> > +    if ( e )
> > +    {
> > +        printk(XENLOG_ERR "ffa: Failed to map rxtx: error %d\n", e);
> > +        goto err_free_ffa_tx;
> > +    }
> > +    ffa_page_count = 1;
>
> ffa_page_count is a constant here and is not used to do the allocation or
> passed as parameter to ffa_rxtx_map.
>
> Do you expect this value to be modified ? how ?

I expect this value to match how many FFA_PAGE_SIZE pages have been
mapped for the RX/TX buffers. Currently, this is only 1 but will have
to be changed later if PAGE_SIZE in Xen or in the secure world is
larger than FFA_PAGE_SIZE. We may also later add support
configurations where RX/TX buffers aren't mapped.

>
> Please set it first and use it for allocation and as parameter to rxtx_map so
> that a modification of the value would only have to be done in one place.
>
> Please use a define if this is a constant.

How about adding a define FFA_MIN_RXTX_PAGE_COUNT and giving that to
ffa_rxtx_map() and later assigning it to ffa_page_count if the call
succeeds?

>
> As it is a global variable, does the parameter to rxtx_map make sense ?

Yes, ffa_rxtx_map() is a dumb wrapper so it should have all the needed
parameters for the SMC provided.

Cheers,
Jens

>
> Cheers
> Bertrand
>
> >     ffa_version = vers;
> >
> >     return true;
> > +
> > +err_free_ffa_tx:
> > +    free_xenheap_pages(ffa_tx, 0);
> > +    ffa_tx = NULL;
> > +err_free_ffa_rx:
> > +    free_xenheap_pages(ffa_rx, 0);
> > +    ffa_rx = NULL;
> > +    ffa_page_count = 0;
> > +    ffa_version = 0;
> > +
> > +    return false;
> > }
> >
> > static const struct tee_mediator_ops ffa_ops =
> > --
> > 2.34.1
> >
>