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

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi,

On Wed, Mar 1, 2023 at 10:56 AM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Jens,
>
> > On 1 Mar 2023, at 10:30, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > 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.
>
> Yes this is the best solution for now.
> And support.md patch is already saying already arm64.
>
> >
> >>
> >>>
> >>> 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.
>
> So it is a constant and the buffers are just mapped or not mapped.

Correct

>
> >
> >>
> >> 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?
>
> Why MIN ?

I was trying to prepare a bit for the future with a minimum value that
would be rounded up to a larger granule as needed depending on
PAGE_SIZE in Xen and what might be discovered about the secure world.

>
> How about just using ffa_rx or ffa_tx being NULL or not to check if the 
> buffers are
> mapped and remove the count.

Sure, I'll drop the _MIN_ part of the define then.

>
> >
> >>
> >> 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.
>
> Then passing FFA_MIN_RXTX_PAGE_COUNT should be enough.

OK.

Thanks,
Jens

>
> Cheers
> Bertrand
>
> >
> > 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
>
>