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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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.

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

> +{
> +    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.

> +}
> +
> 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 ?

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.

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

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
>