Re: [PATCH v5 3/9] xen/arm: add a primitive FF-A mediator

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Julien,

On Tue, Sep 6, 2022 at 12:25 AM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi Jens,
>
> More remarks.
>
> On 18/08/2022 11:55, Jens Wiklander wrote:
> > +/* Negotiated FF-A version to use with the SPMC */
> > +static uint32_t ffa_version __read_mostly;
>
> NIT: if this is not meant to be modified after boot, then I would
> suggest to use __ro_after_init. This was introduced recently and will
> prevent the variable to be modified after boot.

Thanks, I'll update

> > +
> > +static bool ffa_get_version(uint32_t *vers)
>
> This is not __init. Is this going to be called at runtime by a domain?
> If yes...

Correct.

>
> > +{
> > +    const struct arm_smccc_1_2_regs arg = {
> > +        .a0 = FFA_VERSION,
> > +        .a1 = FFA_MY_VERSION,
> > +    };
> > +    struct arm_smccc_1_2_regs resp;
> > +
> > +    arm_smccc_1_2_smc(&arg, &resp);
> > +    if ( resp.a0 == FFA_RET_NOT_SUPPORTED )
> > +    {
> > +        printk(XENLOG_ERR "ffa: FFA_VERSION returned not supported\n");
>
> ... this wants to be a XENLOG_G_ERR to rate limited it. XENLOG_ERR is
> not by default and will allow a domain to spam Xen console.
>
> A rule of thumb is any code reachable for a domain (other than dom0)
> should use XENLOG_G_* when printing or gprintk(XENLOG_*, ) if you want
> to print the domain ID and ratelimit. Note that the latter doesn't
> require the G_* becauce it will add it automatically.

Thanks for the explanation, I'll update accordingly.

>
> > +        return false;
> > +    }
> > +
> > +    *vers = resp.a0;
> > +
> > +    return true;
> > +}
> > +
> > +static u16 get_vm_id(const struct domain *d)
> > +{
> > +    /* +1 since 0 is reserved for the hypervisor in FF-A */
> > +    return d->domain_id + 1;
> > +}
> > +
> > +static void set_regs(struct cpu_user_regs *regs, register_t v0, register_t 
> > v1,
> > +                     register_t v2, register_t v3, register_t v4, 
> > register_t v5,
> > +                     register_t v6, register_t v7)
> > +{
> > +        set_user_reg(regs, 0, v0);
> > +        set_user_reg(regs, 1, v1);
> > +        set_user_reg(regs, 2, v2);
> > +        set_user_reg(regs, 3, v3);
> > +        set_user_reg(regs, 4, v4);
> > +        set_user_reg(regs, 5, v5);
> > +        set_user_reg(regs, 6, v6);
> > +        set_user_reg(regs, 7, v7);
> > +}
> > +
> > +static void set_regs_success(struct cpu_user_regs *regs, uint32_t w2,
> > +                             uint32_t w3)
> > +{
> > +    set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, 0, 0, 0, 0);
> > +}
> > +
> > +static void handle_version(struct cpu_user_regs *regs)
> > +{
> > +    struct domain *d = current->domain;
> > +    struct ffa_ctx *ctx = d->arch.ffa;
> > +    uint32_t vers = get_user_reg(regs, 1);
> > +
> > +    if ( vers < FFA_VERSION_1_1 )
> > +        vers = FFA_VERSION_1_0;
> > +    else
> > +        vers = FFA_VERSION_1_1;
> > +
> > +    ctx->guest_vers = vers;
> > +    set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
> > +}
> > +
> > +bool ffa_handle_call(struct cpu_user_regs *regs, uint32_t fid)
> > +{
> > +    struct domain *d = current->domain;
> > +    struct ffa_ctx *ctx = d->arch.ffa;
> > +
> > +    if ( !ctx )
> > +        return false;
> > +
> > +    switch ( fid )
> > +    {
> > +    case FFA_VERSION:
> > +        handle_version(regs);
> > +        return true;
> > +    case FFA_ID_GET:
> > +        set_regs_success(regs, get_vm_id(d), 0);
> > +        return true;
> > +
> > +    default:
> > +        printk(XENLOG_ERR "ffa: unhandled fid 0x%x\n", fid);
>
> This one definitely want to be a XENLOG_G_ERR. But I would use
> gprintk(XENLOG_ERR, ).

I'll update.

Again, thanks for the review.

Cheers,
Jens

>
> Cheers,
>
> --
> Julien Grall