Re: [XEN PATCH v10 12/24] xen/arm: ffa: send guest events to Secure Partitions

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 19 Jul 2023, at 10:27, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On Tue, Jul 18, 2023 at 12:05 PM Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx> wrote:
>> 
>> Hi Jens,
>> 
>>> On 17 Jul 2023, at 09:20, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
>>> 
>>> The FF-A specification defines framework messages sent as direct
>>> requests when certain events occurs. For instance when a VM (guest) is
>>> created or destroyed. Only SPs which have subscribed to these events
>>> will receive them. An SP can subscribe to these messages in its
>>> partition properties.
>>> 
>>> Adds a check that the SP supports the needed FF-A features
>>> FFA_PARTITION_INFO_GET and FFA_RX_RELEASE.
>>> 
>>> The partition properties of each SP is retrieved with
>>> FFA_PARTITION_INFO_GET which returns the information in our RX buffer.
>>> Using FFA_PARTITION_INFO_GET changes the owner of the RX buffer to the
>>> caller (us), so once we're done with the buffer it must be released
>>> using FFA_RX_RELEASE before another call can be made.
>>> 
>>> Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
>>> ---
>>> xen/arch/arm/tee/ffa.c | 233 ++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 231 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>>> index f8ccaabc568d..d755363de686 100644
>>> --- a/xen/arch/arm/tee/ffa.c
>>> +++ b/xen/arch/arm/tee/ffa.c
>>> @@ -160,14 +160,33 @@
>>> #define FFA_MSG_SEND                    0x8400006EU
>>> #define FFA_MSG_POLL                    0x8400006AU
>>> 
>>> +/* Partition information descriptor */
>>> +struct ffa_partition_info_1_1 {
>>> +    uint16_t id;
>>> +    uint16_t execution_context;
>>> +    uint32_t partition_properties;
>>> +    uint8_t uuid[16];
>>> +};
>>> +
>>> struct ffa_ctx {
>>>    /* FF-A version used by the guest */
>>>    uint32_t guest_vers;
>>> +    /*
>>> +     * Number of SPs that we have sent a VM created signal to, used in
>>> +     * ffa_domain_teardown() to know which SPs need to be signalled.
>>> +     */
>>> +    uint16_t create_signal_count;
>>> };
>>> 
>>> /* Negotiated FF-A version to use with the SPMC */
>>> static uint32_t __ro_after_init ffa_version;
>>> 
>>> +/* SPs subscribing to VM_CREATE and VM_DESTROYED events */
>>> +static uint16_t *subscr_vm_created __read_mostly;
>>> +static uint16_t subscr_vm_created_count __read_mostly;
>>> +static uint16_t *subscr_vm_destroyed __read_mostly;
>>> +static uint16_t subscr_vm_destroyed_count __read_mostly;
>>> +
>>> /*
>>> * Our rx/tx buffers shared with the SPMC. FFA_RXTX_PAGE_COUNT is the
>>> * number of pages used in each of these buffers.
>>> @@ -251,6 +270,87 @@ static int32_t ffa_rxtx_map(paddr_t tx_addr, paddr_t 
>>> rx_addr,
>>>    return ffa_simple_call(FFA_RXTX_MAP_64, tx_addr, rx_addr, page_count, 0);
>>> }
>>> 
>>> +static int32_t ffa_partition_info_get(uint32_t w1, uint32_t w2, uint32_t 
>>> w3,
>>> +                                      uint32_t w4, uint32_t w5,
>>> +                                      uint32_t *count)
>>> +{
>>> +    const struct arm_smccc_1_2_regs arg = {
>>> +        .a0 = FFA_PARTITION_INFO_GET,
>>> +        .a1 = w1,
>>> +        .a2 = w2,
>>> +        .a3 = w3,
>>> +        .a4 = w4,
>>> +        .a5 = w5,
>>> +    };
>>> +    struct arm_smccc_1_2_regs resp;
>>> +    uint32_t ret;
>>> +
>>> +    arm_smccc_1_2_smc(&arg, &resp);
>>> +
>>> +    ret = get_ffa_ret_code(&resp);
>>> +    if ( !ret )
>>> +        *count = resp.a2;
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int32_t ffa_rx_release(void)
>>> +{
>>> +    return ffa_simple_call(FFA_RX_RELEASE, 0, 0, 0, 0);
>>> +}
>>> +
>>> +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;
>>> +    unsigned int retry_count = 0;
>>> +    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;
>>> +        if ( ++retry_count > 10 )
>>> +        {
>>> +            /*
>>> +             * TODO
>>> +             * FFA_RET_INTERRUPTED means that the SPMC has a pending
>>> +             * non-secure interrupt, we need a way of delivering that
>>> +             * non-secure interrupt.
>>> +             * FFA_RET_RETRY is the SP telling us that it's temporarily
>>> +             * blocked from handling the direct request, we need a generic
>>> +             * way to deal with this.
>>> +             * For now in both cases, give up after a few retries.
>>> +             */
>>> +            return res;
>>> +        }
>>> +    } while ( res == FFA_RET_INTERRUPTED || res == FFA_RET_RETRY );
>>> +
>>> +    return res;
>>> +}
>>> +
>>> static uint16_t get_vm_id(const struct domain *d)
>>> {
>>>    /* +1 since 0 is reserved for the hypervisor in FF-A */
>>> @@ -374,6 +474,8 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>>> static int ffa_domain_init(struct domain *d)
>>> {
>>>    struct ffa_ctx *ctx;
>>> +    unsigned int n;
>>> +    int32_t res;
>>> 
>>>    if ( !ffa_version )
>>>        return -ENODEV;
>>> @@ -387,20 +489,68 @@ static int ffa_domain_init(struct domain *d)
>>>    ctx = xzalloc(struct ffa_ctx);
>>>    if ( !ctx )
>>>        return -ENOMEM;
>>> -
>> 
>> This line removal does not seem to be intended.
> 
> Right, I'll restore it.
> 
>> 
>>>    d->arch.tee = ctx;
>>> 
>>> +    for ( n = 0; n < subscr_vm_created_count; n++ )
>>> +    {
>>> +        res = ffa_direct_req_send_vm(subscr_vm_created[n], get_vm_id(d),
>>> +                                     FFA_MSG_SEND_VM_CREATED);
>>> +        if ( res )
>>> +        {
>>> +            printk(XENLOG_ERR "ffa: Failed to report creation of vm_id %u 
>>> to  %u: res %d\n",
>>> +                   get_vm_id(d), subscr_vm_created[n], res);
>>> +            ctx->create_signal_count = n;
>>> +            return -EIO;
>>> +        }
>>> +    }
>>> +    ctx->create_signal_count = n;
>> 
>> For clarity here, i would do:
>> signal_count = subscr_vm_created_count
> 
> OK, I'll update.
> 
>> 
>>> +
>>>    return 0;
>>> }
>>> 
>>> +static bool is_in_subscr_list(const uint16_t *subscr, uint16_t start,
>>> +                              uint16_t end, uint16_t vm_id)
>>> +{
>>> +    unsigned int n;
>>> +
>>> +    for (n = start; n < end; n++)
>> 
>> Coding style, missing spaces
>> 
>>> +    {
>>> +        if (subscr[n] == vm_id)
>> 
>> Coding style, missing spaces
> 
> I'm fixing the style issues.
> 
>> 
>>> +            return true;
>>> +    }
>>> +
>>> +    return false;
>>> +}
>>> +
>>> /* This function is supposed to undo what ffa_domain_init() has done */
>>> static int ffa_domain_teardown(struct domain *d)
>>> {
>>>    struct ffa_ctx *ctx = d->arch.tee;
>>> +    unsigned int n;
>>> +    int32_t res;
>>> 
>>>    if ( !ctx )
>>>        return 0;
>>> 
>>> +    for ( n = 0; n < subscr_vm_destroyed_count; n++ )
>>> +    {
>>> +        /*
>>> +         * Skip SPs subscribed to the VM created event that never was
>>> +         * notified of the VM creation due to an error during
>>> +         * ffa_domain_init().
>>> +         */
>>> +        if ( is_in_subscr_list(subscr_vm_created, ctx->create_signal_count,
>>> +                               subscr_vm_created_count, get_vm_id(d)) )
>>> +            continue;
>> 
>> I am not following the logic here and would need some explanations.
>> 
>> A SP is subscribing to be informed of any VM creation, but here you check
>> the list against the VM ID that is destroyed.
>> 
>> Also a SP could subscribe to be informed of VM destroyed but not VM created.
>> 
>> In my head the logic should be:
>> if (signal_count < subscr_vm_created_count)
>>        for each sp in subscr_vm_created
>>                if is_in_list subscr_vm_destroyed sp
>>                        send_destroyed
>> else
>>        for each subscr_vm_destroyed
>>                send_destroyed
> 
> The call to is_in_subscr_list() will only match SPs that have not yet
> received an expected VM creation event. If all SPs subscribed to the
> VM creation event have been notified, then is_in_subscr_list() will
> match nothing. The idea is that if an SP has subscribed to both create
> and destroy events then it might not expect a destroy event unless it
> has already received a create event. However, SPs only subscribed to
> destroy-events don't care if all create-events have been sent or not,
> they should be notified unconditionally.
> 
> Does it make sense to you?

The logic makes sense but the code is checking subscriber[n] againt the VM ID
being destroyed which does not make sense.
The subscriber list contains a list of SP ID, none of the entries will match 
the VM
ID being destroyed.
What needs to be tested is if someone in the created subscribers that was 
signaled
already is in the destroy subscribers so that it needs to be signaled.

Cheers
Bertrand

> 
>> 
>> 
>>> +
>>> +        res = ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d),
>>> +                                     FFA_MSG_SEND_VM_DESTROYED);
>>> +
>>> +        if ( res )
>>> +            printk(XENLOG_ERR "ffa: Failed to report destruction of vm_id 
>>> %u to  %u: res %d\n",
>>> +                   get_vm_id(d), subscr_vm_destroyed[n], res);
>>> +    }
>>> +
>>>    XFREE(d->arch.tee);
>>> 
>>>    return 0;
>>> @@ -411,6 +561,81 @@ static int ffa_relinquish_resources(struct domain *d)
>>>    return 0;
>>> }
>>> 
>>> +static void uninit_subscribers(void)
>>> +{
>>> +        subscr_vm_created_count = 0;
>>> +        subscr_vm_destroyed_count = 0;
>>> +        XFREE(subscr_vm_created);
>>> +        XFREE(subscr_vm_destroyed);
>>> +}
>>> +
>>> +static bool init_subscribers(struct ffa_partition_info_1_1 *fpi, uint16_t 
>>> count)
>>> +{
>>> +    uint16_t n;
>>> +    uint16_t c_pos;
>>> +    uint16_t d_pos;
>>> +
>>> +    subscr_vm_created_count = 0;
>>> +    subscr_vm_destroyed_count = 0;
>>> +    for ( n = 0; n < count; n++ )
>>> +    {
>>> +        if (fpi[n].partition_properties & FFA_PART_PROP_NOTIF_CREATED)
>> Coding style: spaces
>> 
>>> +            subscr_vm_created_count++;
>>> +        if (fpi[n].partition_properties & FFA_PART_PROP_NOTIF_DESTROYED)
>> Coding style: spaces
> 
> I'll fix the style issues.
> 
> Thanks,
> Jens
> 
>> 
>> Cheers
>> Bertrand
>> 
>>> +            subscr_vm_destroyed_count++;
>>> +    }
>>> +
>>> +    if ( subscr_vm_created_count )
>>> +        subscr_vm_created = xzalloc_array(uint16_t, 
>>> subscr_vm_created_count);
>>> +    if ( subscr_vm_destroyed_count )
>>> +        subscr_vm_destroyed = xzalloc_array(uint16_t,
>>> +                                            subscr_vm_destroyed_count);
>>> +    if ( (subscr_vm_created_count && !subscr_vm_created) ||
>>> +         (subscr_vm_destroyed_count && !subscr_vm_destroyed) )
>>> +    {
>>> +        printk(XENLOG_ERR "ffa: Failed to allocate subscription lists\n");
>>> +        uninit_subscribers();
>>> +        return false;
>>> +    }
>>> +
>>> +    for ( c_pos = 0, d_pos = 0, n = 0; n < count; n++ )
>>> +    {
>>> +        if ( fpi[n].partition_properties & FFA_PART_PROP_NOTIF_CREATED )
>>> +            subscr_vm_created[c_pos++] = fpi[n].id;
>>> +        if ( fpi[n].partition_properties & FFA_PART_PROP_NOTIF_DESTROYED )
>>> +            subscr_vm_destroyed[d_pos++] = fpi[n].id;
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>> +
>>> +static bool init_sps(void)
>>> +{
>>> +    bool ret = false;
>>> +    uint32_t count;
>>> +    int e;
>>> +
>>> +    e = ffa_partition_info_get(0, 0, 0, 0, 0, &count);
>>> +    if ( e )
>>> +    {
>>> +        printk(XENLOG_ERR "ffa: Failed to get list of SPs: %d\n", e);
>>> +        goto out;
>>> +    }
>>> +
>>> +    if ( count >= UINT16_MAX )
>>> +    {
>>> +        printk(XENLOG_ERR "ffa: Impossible number of SPs: %u\n", count);
>>> +        goto out;
>>> +    }
>>> +
>>> +    ret = init_subscribers(ffa_rx, count);
>>> +
>>> +out:
>>> +    ffa_rx_release();
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> static bool ffa_probe(void)
>>> {
>>>    uint32_t vers;
>>> @@ -462,7 +687,8 @@ static bool ffa_probe(void)
>>>     * TODO: Rework the code to allow domain to use a subset of the
>>>     * features supported.
>>>     */
>>> -    if (
>>> +    if ( !check_mandatory_feature(FFA_PARTITION_INFO_GET) ||
>>> +         !check_mandatory_feature(FFA_RX_RELEASE) ||
>>>         !check_mandatory_feature(FFA_RXTX_MAP_64) ||
>>>         !check_mandatory_feature(FFA_RXTX_UNMAP) ||
>>>         !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
>>> @@ -484,6 +710,9 @@ static bool ffa_probe(void)
>>>    }
>>>    ffa_version = vers;
>>> 
>>> +    if ( !init_sps() )
>>> +        goto err_free_ffa_tx;
>>> +
>>>    return true;
>>> 
>>> err_free_ffa_tx:
>>> --
>>> 2.34.1