Re: [XEN PATCH v2 5/5] xen/arm: ffa: support notification

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 23 Apr 2024, at 17:16, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On 23/04/2024 16:12, Bertrand Marquis wrote:
>> Hi Julien,
>>> On 22 Apr 2024, at 13:40, Julien Grall <julien@xxxxxxx> 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