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

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Fri, Apr 14, 2023 at 10:29 AM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> 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 ?

Sounds good to me, I'll do that.

Thanks,
Jens

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