Hi Jens,
> On 1 Mar 2023, at 10:30, Jens Wiklander <jens.wiklan...@linaro.org> wrote:
>
> Hi Bertrand,
>
> On Tue, Feb 28, 2023 at 1:57 PM Bertrand Marquis
> <bertrand.marq...@arm.com> wrote:
>>
>> Hi Jens,
>>
>>> On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklan...@linaro.org> 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.wiklan...@linaro.org>
>>> ---
>>> 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
