|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH v10 12/24] xen/arm: ffa: send guest events to Secure Partitions
Hi,
On Wed, Jul 19, 2023 at 11:19 AM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> 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.
You're right, I'll update the patch.
> 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.
Yes, I have unfortunately only one SP in my setup.
Thanks,
Jens
>
> 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
>
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |