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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 14 Apr 2023, at 10:19, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> 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.

The fact that interrupts are masked in Xen does not mean they will be in secure.
In fact what we should do when INTERRUPTED is received is something we have
to clear up but we should unmask interrupt to process them in Xen before 
retrying I think.

> 
> 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?

I would suggest to do a max retry for both cases and add a TODO in the code.
We will need to define a generic way to handle those cases but at this stage
INTERRUPTED should be considered TODO.
RETRY will probably stay with a limit here, in the case of a guest message both
of those possibilities could just be returned to the guest.


Do you agree ?

Cheers
Bertrand

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