Hi Julien,
On Mon, Apr 22, 2024 at 1:40 PM 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?
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?
Thanks,
Jens
