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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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.

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

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

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

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