Hi Julien,
> On 23 Apr 2024, at 17:16, Julien Grall <jul...@xen.org> wrote:
>
> Hi Bertrand,
>
> On 23/04/2024 16:12, Bertrand Marquis wrote:
>> Hi Julien,
>>> On 22 Apr 2024, at 13:40, Julien Grall <jul...@xen.org> wrote:
>>>
>>> Hi Jens,
>>>
>>> This is not a full review of the code. I will let Bertrand doing it.
>>>
>>> On 22/04/2024 08:37, Jens Wiklander wrote:
>>>> +void ffa_notif_init(void)
>>>> +{
>>>> + const struct arm_smccc_1_2_regs arg = {
>>>> + .a0 = FFA_FEATURES,
>>>> + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
>>>> + };
>>>> + struct arm_smccc_1_2_regs resp;
>>>> + unsigned int irq;
>>>> + int ret;
>>>> +
>>>> + arm_smccc_1_2_smc(&arg, &resp);
>>>> + if ( resp.a0 != FFA_SUCCESS_32 )
>>>> + return;
>>>> +
>>>> + irq = resp.a2;
>>>> + if ( irq >= NR_GIC_SGI )
>>>> + irq_set_type(irq, IRQ_TYPE_EDGE_RISING);
>>>> + ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL);
>>>
>>> If I am not mistaken, ffa_notif_init() is only called once on the boot CPU.
>>> However, request_irq() needs to be called on every CPU so the callback is
>>> registered every where and the interrupt enabled.
>>>
>>> I know the name of the function is rather confusing. So can you confirm
>>> this is what you expected?
>>>
>>> [...]
>>>
>>>> diff --git a/xen/arch/arm/tee/ffa_private.h
>>>> b/xen/arch/arm/tee/ffa_private.h
>>>> index 98236cbf14a3..ef8ffd4526bd 100644
>>>> --- a/xen/arch/arm/tee/ffa_private.h
>>>> +++ b/xen/arch/arm/tee/ffa_private.h
>>>> @@ -25,6 +25,7 @@
>>>> #define FFA_RET_DENIED -6
>>>> #define FFA_RET_RETRY -7
>>>> #define FFA_RET_ABORTED -8
>>>> +#define FFA_RET_NO_DATA -9
>>>> /* FFA_VERSION helpers */
>>>> #define FFA_VERSION_MAJOR_SHIFT 16U
>>>> @@ -97,6 +98,18 @@
>>>> */
>>>> #define FFA_MAX_SHM_COUNT 32
>>>> +/*
>>>> + * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely
>>>> + * unused, but that may change.
>>>
>>> Are the value below intended for the guests? If so, can they be moved in
>>> public/arch-arm.h along with the others guest interrupts?
>> The values are to be used by the guest but they will discover them through
>> the FFA_FEATURES ABI so I do not think those
>> should belong the public headers.
>
> Sorry I should have been clearer. The values in the public headers are not
> meant for the guest. They are meant for a common understanding between the
> toolstack and Xen of the guest layout (memory + interrupts).
>
> I know that some of the guests started to rely on it. But this is against our
> policy:
>
> * These are defined for consistency between the tools and the
> * hypervisor. Guests must not rely on these hardcoded values but
> * should instead use the FDT.
>
> In this case, even if this is only used by Xen (today), I would argue they
> should defined at the same place because it is easier to understand the
> layout if it is in one place.
I see, that makes sense then to add them there.
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
