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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 23 Apr 2024, at 17:26, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi Julien,
> 
> On Mon, Apr 22, 2024 at 1:40 PM 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?
> 
> Good catch, no this wasn't what I expected. I'll need to change this.
> 
>> 
>> [...]
>> 
>>> 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?
> 
> Yes, I'll move it.
> 
>> 
>>> + *
>>> + * 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?
> 
> There's one at the top of arch/arm/tee/ffa.c,
> https://developer.arm.com/documentation/den0077/e
> Do you want the link close to the defines when I've moved them to
> public/arch-arm.h?
> Or is it perhaps better to give a link to "Arm Base System
> Architecture v1.0C", https://developer.arm.com/documentation/den0094/
> instead?

I would say we need the link to Arm Base System Architecture in arch-arm.

Regards
Bertrand

> 
> Thanks,
> Jens