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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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.

Cheers
Bertrand

> 
>> + *
>> + * SGI is the preferred delivery mechanism. SGIs 8-15 are normally not used
>> + * by a guest as they in a non-virtualized system typically are assigned to
>> + * the secure world. Here we're free to use SGI 8-15 since they are virtual
>> + * and have nothing to do with the secure world.
> 
> Do you have a pointer to the specification?
> 
> [...]
> 
> Cheers,
> 
> -- 
> Julien Grall