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

[Julien Grall <julien@xxxxxxx>]

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.
> +
> +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...
> +{
> +    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.
> +        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, ).
Cheers,

--
Julien Grall