Re: [XEN PATCH v8 11/22] xen/arm: ffa: send guest events to Secure Partitions

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi,

On Thu, Apr 13, 2023 at 3:24 PM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi,
>
> On 13/04/2023 08:14, Jens Wiklander wrote:
> > +static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
> > +                                      uint8_t msg)
> > +{
> > +    uint32_t exp_resp = FFA_MSG_FLAG_FRAMEWORK;
> > +    int32_t res;
> > +
> > +    if ( msg == FFA_MSG_SEND_VM_CREATED )
> > +        exp_resp |= FFA_MSG_RESP_VM_CREATED;
> > +    else if ( msg == FFA_MSG_SEND_VM_DESTROYED )
> > +        exp_resp |= FFA_MSG_RESP_VM_DESTROYED;
> > +    else
> > +        return FFA_RET_INVALID_PARAMETERS;
> > +
> > +    do {
> > +        const struct arm_smccc_1_2_regs arg = {
> > +            .a0 = FFA_MSG_SEND_DIRECT_REQ_32,
> > +            .a1 = sp_id,
> > +            .a2 = FFA_MSG_FLAG_FRAMEWORK | msg,
> > +            .a5 = vm_id,
> > +        };
> > +        struct arm_smccc_1_2_regs resp;
> > +
> > +        arm_smccc_1_2_smc(&arg, &resp);
> > +        if ( resp.a0 != FFA_MSG_SEND_DIRECT_RESP_32 || resp.a2 != exp_resp 
> > )
> > +        {
> > +            /*
> > +             * This is an invalid response, likely due to some error in the
> > +             * implementation of the ABI.
> > +             */
> > +            return FFA_RET_INVALID_PARAMETERS;
> > +        }
> > +        res = resp.a3;
> > +    } while ( res == FFA_RET_INTERRUPTED || res == FFA_RET_RETRY );
>
> This loop seems potentially unbounded to me. Can you add a comment
> explaining why this is fine?

In the FF-A 1.1 specification
(https://developer.arm.com/documentation/den0077/e/?lang=en) Table
18.26 at page 330 it says that FFA_RET_INTERRUPTED and FFA_RET_RETRY
should be handled in this way. When looking at this from the
hypervisor's point of view it is troublesome since there isn't any
guarantee that we're progressing.

We should be able to rule out FFA_RET_INTERRUPTED since non-secure
interrupts should be masked at this point. I'm not sure if
FFA_RET_RETRY can be avoided entirely, but we should be able to put a
limit on how many times we're prepared to retry.

How about setting a limit of max 10 retries and treating
FFA_RET_INTERRUPTED as an error? Or is it better to not loop at all
and treat all but FFA_RET_OK as errors? What do others think?

Thanks,
Jens

>
> > +
> > +    return res;
> > +}
> > +
>
> Cheers,
>
> --
> Julien Grall