| Message ID | cfd9ae67bdf24bee796b418937dd1486018fd188.1677079672.git.jens.wiklander@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Xen FF-A mediator | expand |
Hi Jens, > On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@linaro.org> 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@linaro.org> > --- > xen/arch/arm/tee/ffa.c | 191 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 190 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > index 07dd5c36d54b..f1b014b6c7f4 100644 > --- a/xen/arch/arm/tee/ffa.c > +++ b/xen/arch/arm/tee/ffa.c > @@ -140,6 +140,14 @@ > #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 { > uint32_t guest_vers; > bool interrupted; > @@ -148,6 +156,12 @@ struct ffa_ctx { > /* Negotiated FF-A version to use with the SPMC */ > static uint32_t ffa_version __ro_after_init; > > +/* SPs subscribing to VM_CREATE and VM_DESTROYED events */ > +static uint16_t *subscr_vm_created __read_mostly; > +static unsigned int subscr_vm_created_count __read_mostly; In the spec the number is returned in w2 so you should utse uint32_t here. > +static uint16_t *subscr_vm_destroyed __read_mostly; > +static unsigned int subscr_vm_destroyed_count __read_mostly; Same here > + > /* > * Our rx/tx buffers shared with the SPMC. > * > @@ -237,6 +251,72 @@ static int32_t ffa_rxtx_map(register_t tx_addr, register_t rx_addr, > return ffa_simple_call(fid, 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) This function is actually only useable to send framework created/destroyed messages so the function name should be adapted to be less generic. ffa_send_vm_events ? unless you want to modify it later to send more framework messages ? > +{ > + 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 ); We might end up in an infinite loop here or increase interrupt response time. In the general case we could return that code directly to the VM but here we are in the VM creation/destroy path so we cannot do that. Maybe in debug mode at least we should have a retry counter here for now with a print ? > + > + return res; > +} > + > static uint16_t get_vm_id(const struct domain *d) > { > /* +1 since 0 is reserved for the hypervisor in FF-A */ > @@ -371,6 +451,10 @@ 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; > + unsigned int m; > + unsigned int c_pos; > + int32_t res; > > /* > * We can't use that last possible domain ID or get_vm_id() would cause > @@ -383,24 +467,121 @@ static int ffa_domain_init(struct domain *d) > if ( !ctx ) > return -ENOMEM; > > + 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); in this function, get_vm_id(d) will not change so i would suggest to store it in a local variable instead of calling get_vm_id each time. > + c_pos = n; > + goto err; > + } > + } > + > d->arch.tee = ctx; > > return 0; > + > +err: > + /* Undo any already sent vm created messaged */ > + for ( n = 0; n < c_pos; n++ ) > + for ( m = 0; m < subscr_vm_destroyed_count; m++ ) > + if ( subscr_vm_destroyed[m] == subscr_vm_created[n] ) > + ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d), > + FFA_MSG_SEND_VM_DESTROYED); > + > + return -ENOMEM; The VM creation is not failing due to missing memory here. We need to find a better error code. Maybe ENOTCONN ? I am open to ideas here :-) > } > > /* This function is supposed to undo what ffa_domain_init() has done */ > static int ffa_relinquish_resources(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++ ) > + { > + 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; > } > > +static bool init_subscribers(void) > +{ > + struct ffa_partition_info_1_1 *fpi; > + bool ret = false; > + uint32_t count; > + int e; > + uint32_t n; > + uint32_t c_pos; > + uint32_t d_pos; > + > + if ( ffa_version < FFA_VERSION_1_1 ) > + return true; Correct me if i am wrong but on 1.0 version we cannot retrieve the count but we could do the slow path and do a first loop on info_get until we get an error ? > + > + 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; > + } > + > + fpi = ffa_rx; > + 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) > + subscr_vm_created_count++; > + if (fpi[n].partition_properties & FFA_PART_PROP_NOTIF_DESTROYED) > + 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"); > + subscr_vm_created_count = 0; > + subscr_vm_destroyed_count = 0; > + XFREE(subscr_vm_created); > + XFREE(subscr_vm_destroyed); > + goto out; > + } > + > + 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; > + } > + > + ret = true; > +out: > + ffa_rx_release(); > + > + return ret; > +} > + > static bool ffa_probe(void) > { > uint32_t vers; > @@ -447,7 +628,8 @@ static bool ffa_probe(void) > printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", > major_vers, minor_vers); > > - if ( > + if ( !check_mandatory_feature(FFA_PARTITION_INFO_GET) || > + !check_mandatory_feature(FFA_RX_RELEASE) || > #ifdef CONFIG_ARM_64 > !check_mandatory_feature(FFA_RXTX_MAP_64) || > #endif > @@ -475,6 +657,9 @@ static bool ffa_probe(void) > ffa_page_count = 1; > ffa_version = vers; > > + if ( !init_subscribers() ) > + goto err_free_ffa_tx; > + > return true; > > err_free_ffa_tx: > @@ -485,6 +670,10 @@ err_free_ffa_rx: > ffa_rx = NULL; > ffa_page_count = 0; > ffa_version = 0; > + XFREE(subscr_vm_created); > + subscr_vm_created_count = 0; > + XFREE(subscr_vm_destroyed); > + subscr_vm_destroyed_count = 0; > > return false; > } > -- > 2.34.1 > Cheers Bertrand
Hi Bertrand, On Tue, Feb 28, 2023 at 5:49 PM Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: > > Hi Jens, > > > On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@linaro.org> 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@linaro.org> > > --- > > xen/arch/arm/tee/ffa.c | 191 ++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 190 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > > index 07dd5c36d54b..f1b014b6c7f4 100644 > > --- a/xen/arch/arm/tee/ffa.c > > +++ b/xen/arch/arm/tee/ffa.c > > @@ -140,6 +140,14 @@ > > #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 { > > uint32_t guest_vers; > > bool interrupted; > > @@ -148,6 +156,12 @@ struct ffa_ctx { > > /* Negotiated FF-A version to use with the SPMC */ > > static uint32_t ffa_version __ro_after_init; > > > > +/* SPs subscribing to VM_CREATE and VM_DESTROYED events */ > > +static uint16_t *subscr_vm_created __read_mostly; > > +static unsigned int subscr_vm_created_count __read_mostly; > > In the spec the number is returned in w2 so you should utse uint32_t here. I don't understand. This value is increased for each SP which has the property set in the Partition information descriptor. > > > +static uint16_t *subscr_vm_destroyed __read_mostly; > > +static unsigned int subscr_vm_destroyed_count __read_mostly; > > Same here > > > + > > /* > > * Our rx/tx buffers shared with the SPMC. > > * > > @@ -237,6 +251,72 @@ static int32_t ffa_rxtx_map(register_t tx_addr, register_t rx_addr, > > return ffa_simple_call(fid, 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) > > This function is actually only useable to send framework created/destroyed > messages so the function name should be adapted to be less generic. > > ffa_send_vm_events ? > > unless you want to modify it later to send more framework messages ? That was the plan, but that may never happen. I'll rename it to ffa_send_vm_event() since we're only sending one event at a time. > > > +{ > > + 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 ); > > We might end up in an infinite loop here or increase interrupt response time. > In the general case we could return that code directly to the VM but here we > are in the VM creation/destroy path so we cannot do that. > > Maybe in debug mode at least we should have a retry counter here for now > with a print ? OK, I'll add something. > > > + > > + return res; > > +} > > + > > static uint16_t get_vm_id(const struct domain *d) > > { > > /* +1 since 0 is reserved for the hypervisor in FF-A */ > > @@ -371,6 +451,10 @@ 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; > > + unsigned int m; > > + unsigned int c_pos; > > + int32_t res; > > > > /* > > * We can't use that last possible domain ID or get_vm_id() would cause > > @@ -383,24 +467,121 @@ static int ffa_domain_init(struct domain *d) > > if ( !ctx ) > > return -ENOMEM; > > > > + 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); > > in this function, get_vm_id(d) will not change so i would suggest to store it in a local variable > instead of calling get_vm_id each time. OK > > > + c_pos = n; > > + goto err; > > + } > > + } > > + > > d->arch.tee = ctx; > > > > return 0; > > + > > +err: > > + /* Undo any already sent vm created messaged */ > > + for ( n = 0; n < c_pos; n++ ) > > + for ( m = 0; m < subscr_vm_destroyed_count; m++ ) > > + if ( subscr_vm_destroyed[m] == subscr_vm_created[n] ) > > + ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d), > > + FFA_MSG_SEND_VM_DESTROYED); > > + > > + return -ENOMEM; > > The VM creation is not failing due to missing memory here. > We need to find a better error code. > Maybe ENOTCONN ? > I am open to ideas here :-) That makes sense, I'll change it to ENOTCONN. > > > } > > > > /* This function is supposed to undo what ffa_domain_init() has done */ > > static int ffa_relinquish_resources(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++ ) > > + { > > + 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; > > } > > > > +static bool init_subscribers(void) > > +{ > > + struct ffa_partition_info_1_1 *fpi; > > + bool ret = false; > > + uint32_t count; > > + int e; > > + uint32_t n; > > + uint32_t c_pos; > > + uint32_t d_pos; > > + > > + if ( ffa_version < FFA_VERSION_1_1 ) > > + return true; > > Correct me if i am wrong but on 1.0 version we cannot retrieve the count but > we could do the slow path and do a first loop on info_get until we get an error ? Sending the events is not supported in 1.0 so there's nothing to record in that case. Thanks, Jens > > > + > > + 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; > > + } > > + > > + fpi = ffa_rx; > > + 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) > > + subscr_vm_created_count++; > > + if (fpi[n].partition_properties & FFA_PART_PROP_NOTIF_DESTROYED) > > + 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"); > > + subscr_vm_created_count = 0; > > + subscr_vm_destroyed_count = 0; > > + XFREE(subscr_vm_created); > > + XFREE(subscr_vm_destroyed); > > + goto out; > > + } > > + > > + 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; > > + } > > + > > + ret = true; > > +out: > > + ffa_rx_release(); > > + > > + return ret; > > +} > > + > > static bool ffa_probe(void) > > { > > uint32_t vers; > > @@ -447,7 +628,8 @@ static bool ffa_probe(void) > > printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", > > major_vers, minor_vers); > > > > - if ( > > + if ( !check_mandatory_feature(FFA_PARTITION_INFO_GET) || > > + !check_mandatory_feature(FFA_RX_RELEASE) || > > #ifdef CONFIG_ARM_64 > > !check_mandatory_feature(FFA_RXTX_MAP_64) || > > #endif > > @@ -475,6 +657,9 @@ static bool ffa_probe(void) > > ffa_page_count = 1; > > ffa_version = vers; > > > > + if ( !init_subscribers() ) > > + goto err_free_ffa_tx; > > + > > return true; > > > > err_free_ffa_tx: > > @@ -485,6 +670,10 @@ err_free_ffa_rx: > > ffa_rx = NULL; > > ffa_page_count = 0; > > ffa_version = 0; > > + XFREE(subscr_vm_created); > > + subscr_vm_created_count = 0; > > + XFREE(subscr_vm_destroyed); > > + subscr_vm_destroyed_count = 0; > > > > return false; > > } > > -- > > 2.34.1 > > > > Cheers > Bertrand > >
Hi Jens, > On 1 Mar 2023, at 11:16, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > Hi Bertrand, > > On Tue, Feb 28, 2023 at 5:49 PM Bertrand Marquis > <Bertrand.Marquis@arm.com> wrote: >> >> Hi Jens, >> >>> On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@linaro.org> 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@linaro.org> >>> --- >>> xen/arch/arm/tee/ffa.c | 191 ++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 190 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c >>> index 07dd5c36d54b..f1b014b6c7f4 100644 >>> --- a/xen/arch/arm/tee/ffa.c >>> +++ b/xen/arch/arm/tee/ffa.c >>> @@ -140,6 +140,14 @@ >>> #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 { >>> uint32_t guest_vers; >>> bool interrupted; >>> @@ -148,6 +156,12 @@ struct ffa_ctx { >>> /* Negotiated FF-A version to use with the SPMC */ >>> static uint32_t ffa_version __ro_after_init; >>> >>> +/* SPs subscribing to VM_CREATE and VM_DESTROYED events */ >>> +static uint16_t *subscr_vm_created __read_mostly; >>> +static unsigned int subscr_vm_created_count __read_mostly; >> >> In the spec the number is returned in w2 so you should utse uint32_t here. > > I don't understand. This value is increased for each SP which has the > property set in the Partition information descriptor. Using generic types should be prevented when possible. Here this is a subset of the number of partition which is uint32_t (wX reg) so i think this would be the logical type for this. > >> >>> +static uint16_t *subscr_vm_destroyed __read_mostly; >>> +static unsigned int subscr_vm_destroyed_count __read_mostly; >> >> Same here >> >>> + >>> /* >>> * Our rx/tx buffers shared with the SPMC. >>> * >>> @@ -237,6 +251,72 @@ static int32_t ffa_rxtx_map(register_t tx_addr, register_t rx_addr, >>> return ffa_simple_call(fid, 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) >> >> This function is actually only useable to send framework created/destroyed >> messages so the function name should be adapted to be less generic. >> >> ffa_send_vm_events ? >> >> unless you want to modify it later to send more framework messages ? > > That was the plan, but that may never happen. I'll rename it to > ffa_send_vm_event() since we're only sending one event at a time. > >> >>> +{ >>> + 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 ); >> >> We might end up in an infinite loop here or increase interrupt response time. >> In the general case we could return that code directly to the VM but here we >> are in the VM creation/destroy path so we cannot do that. >> >> Maybe in debug mode at least we should have a retry counter here for now >> with a print ? > > OK, I'll add something. > >> >>> + >>> + return res; >>> +} >>> + >>> static uint16_t get_vm_id(const struct domain *d) >>> { >>> /* +1 since 0 is reserved for the hypervisor in FF-A */ >>> @@ -371,6 +451,10 @@ 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; >>> + unsigned int m; >>> + unsigned int c_pos; >>> + int32_t res; >>> >>> /* >>> * We can't use that last possible domain ID or get_vm_id() would cause >>> @@ -383,24 +467,121 @@ static int ffa_domain_init(struct domain *d) >>> if ( !ctx ) >>> return -ENOMEM; >>> >>> + 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); >> >> in this function, get_vm_id(d) will not change so i would suggest to store it in a local variable >> instead of calling get_vm_id each time. > > OK > >> >>> + c_pos = n; >>> + goto err; >>> + } >>> + } >>> + >>> d->arch.tee = ctx; >>> >>> return 0; >>> + >>> +err: >>> + /* Undo any already sent vm created messaged */ >>> + for ( n = 0; n < c_pos; n++ ) >>> + for ( m = 0; m < subscr_vm_destroyed_count; m++ ) >>> + if ( subscr_vm_destroyed[m] == subscr_vm_created[n] ) >>> + ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d), >>> + FFA_MSG_SEND_VM_DESTROYED); >>> + >>> + return -ENOMEM; >> >> The VM creation is not failing due to missing memory here. >> We need to find a better error code. >> Maybe ENOTCONN ? >> I am open to ideas here :-) > > That makes sense, I'll change it to ENOTCONN. > >> >>> } >>> >>> /* This function is supposed to undo what ffa_domain_init() has done */ >>> static int ffa_relinquish_resources(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++ ) >>> + { >>> + 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; >>> } >>> >>> +static bool init_subscribers(void) >>> +{ >>> + struct ffa_partition_info_1_1 *fpi; >>> + bool ret = false; >>> + uint32_t count; >>> + int e; >>> + uint32_t n; >>> + uint32_t c_pos; >>> + uint32_t d_pos; >>> + >>> + if ( ffa_version < FFA_VERSION_1_1 ) >>> + return true; >> >> Correct me if i am wrong but on 1.0 version we cannot retrieve the count but >> we could do the slow path and do a first loop on info_get until we get an error ? > > Sending the events is not supported in 1.0 so there's nothing to > record in that case. Please add a comment here to say that subscribers are only supported after 1.1 and also mention it in the commit message. Cheers Bertrand > > Thanks, > Jens > >> >>> + >>> + 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; >>> + } >>> + >>> + fpi = ffa_rx; >>> + 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) >>> + subscr_vm_created_count++; >>> + if (fpi[n].partition_properties & FFA_PART_PROP_NOTIF_DESTROYED) >>> + 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"); >>> + subscr_vm_created_count = 0; >>> + subscr_vm_destroyed_count = 0; >>> + XFREE(subscr_vm_created); >>> + XFREE(subscr_vm_destroyed); >>> + goto out; >>> + } >>> + >>> + 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; >>> + } >>> + >>> + ret = true; >>> +out: >>> + ffa_rx_release(); >>> + >>> + return ret; >>> +} >>> + >>> static bool ffa_probe(void) >>> { >>> uint32_t vers; >>> @@ -447,7 +628,8 @@ static bool ffa_probe(void) >>> printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", >>> major_vers, minor_vers); >>> >>> - if ( >>> + if ( !check_mandatory_feature(FFA_PARTITION_INFO_GET) || >>> + !check_mandatory_feature(FFA_RX_RELEASE) || >>> #ifdef CONFIG_ARM_64 >>> !check_mandatory_feature(FFA_RXTX_MAP_64) || >>> #endif >>> @@ -475,6 +657,9 @@ static bool ffa_probe(void) >>> ffa_page_count = 1; >>> ffa_version = vers; >>> >>> + if ( !init_subscribers() ) >>> + goto err_free_ffa_tx; >>> + >>> return true; >>> >>> err_free_ffa_tx: >>> @@ -485,6 +670,10 @@ err_free_ffa_rx: >>> ffa_rx = NULL; >>> ffa_page_count = 0; >>> ffa_version = 0; >>> + XFREE(subscr_vm_created); >>> + subscr_vm_created_count = 0; >>> + XFREE(subscr_vm_destroyed); >>> + subscr_vm_destroyed_count = 0; >>> >>> return false; >>> } >>> -- >>> 2.34.1 >>> >> >> Cheers >> Bertrand
Hi Jens, > On 1 Mar 2023, at 11:16, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > Hi Bertrand, > > On Tue, Feb 28, 2023 at 5:49 PM Bertrand Marquis > <Bertrand.Marquis@arm.com> wrote: >> >> Hi Jens, >> >>> On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@linaro.org> 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@linaro.org> >>> --- >>> xen/arch/arm/tee/ffa.c | 191 ++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 190 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c >>> index 07dd5c36d54b..f1b014b6c7f4 100644 >>> --- a/xen/arch/arm/tee/ffa.c >>> +++ b/xen/arch/arm/tee/ffa.c >>> @@ -140,6 +140,14 @@ >>> #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 { >>> uint32_t guest_vers; >>> bool interrupted; >>> @@ -148,6 +156,12 @@ struct ffa_ctx { >>> /* Negotiated FF-A version to use with the SPMC */ >>> static uint32_t ffa_version __ro_after_init; >>> >>> +/* SPs subscribing to VM_CREATE and VM_DESTROYED events */ >>> +static uint16_t *subscr_vm_created __read_mostly; >>> +static unsigned int subscr_vm_created_count __read_mostly; >> >> In the spec the number is returned in w2 so you should utse uint32_t here. > > I don't understand. This value is increased for each SP which has the > property set in the Partition information descriptor. > >> >>> +static uint16_t *subscr_vm_destroyed __read_mostly; >>> +static unsigned int subscr_vm_destroyed_count __read_mostly; >> >> Same here >> >>> + >>> /* >>> * Our rx/tx buffers shared with the SPMC. >>> * >>> @@ -237,6 +251,72 @@ static int32_t ffa_rxtx_map(register_t tx_addr, register_t rx_addr, >>> return ffa_simple_call(fid, 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) >> >> This function is actually only useable to send framework created/destroyed >> messages so the function name should be adapted to be less generic. >> >> ffa_send_vm_events ? >> >> unless you want to modify it later to send more framework messages ? > > That was the plan, but that may never happen. I'll rename it to > ffa_send_vm_event() since we're only sending one event at a time. > >> >>> +{ >>> + 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 ); >> >> We might end up in an infinite loop here or increase interrupt response time. >> In the general case we could return that code directly to the VM but here we >> are in the VM creation/destroy path so we cannot do that. >> >> Maybe in debug mode at least we should have a retry counter here for now >> with a print ? > > OK, I'll add something. In fact here we should always count and fail if we do it more than once or twice. In a normal case, we would return the INTERRUPTED or RETRY to the caller so that he would do the call again. Here this is something done by Xen internally so we cannot do that (we could make the VM creation fail but destroying we cannot) so we must limit the number of retry and continue if this fails. Cheers Bertrand > >> >>> + >>> + return res; >>> +} >>> + >>> static uint16_t get_vm_id(const struct domain *d) >>> { >>> /* +1 since 0 is reserved for the hypervisor in FF-A */ >>> @@ -371,6 +451,10 @@ 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; >>> + unsigned int m; >>> + unsigned int c_pos; >>> + int32_t res; >>> >>> /* >>> * We can't use that last possible domain ID or get_vm_id() would cause >>> @@ -383,24 +467,121 @@ static int ffa_domain_init(struct domain *d) >>> if ( !ctx ) >>> return -ENOMEM; >>> >>> + 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); >> >> in this function, get_vm_id(d) will not change so i would suggest to store it in a local variable >> instead of calling get_vm_id each time. > > OK > >> >>> + c_pos = n; >>> + goto err; >>> + } >>> + } >>> + >>> d->arch.tee = ctx; >>> >>> return 0; >>> + >>> +err: >>> + /* Undo any already sent vm created messaged */ >>> + for ( n = 0; n < c_pos; n++ ) >>> + for ( m = 0; m < subscr_vm_destroyed_count; m++ ) >>> + if ( subscr_vm_destroyed[m] == subscr_vm_created[n] ) >>> + ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d), >>> + FFA_MSG_SEND_VM_DESTROYED); >>> + >>> + return -ENOMEM; >> >> The VM creation is not failing due to missing memory here. >> We need to find a better error code. >> Maybe ENOTCONN ? >> I am open to ideas here :-) > > That makes sense, I'll change it to ENOTCONN. > >> >>> } >>> >>> /* This function is supposed to undo what ffa_domain_init() has done */ >>> static int ffa_relinquish_resources(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++ ) >>> + { >>> + 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; >>> } >>> >>> +static bool init_subscribers(void) >>> +{ >>> + struct ffa_partition_info_1_1 *fpi; >>> + bool ret = false; >>> + uint32_t count; >>> + int e; >>> + uint32_t n; >>> + uint32_t c_pos; >>> + uint32_t d_pos; >>> + >>> + if ( ffa_version < FFA_VERSION_1_1 ) >>> + return true; >> >> Correct me if i am wrong but on 1.0 version we cannot retrieve the count but >> we could do the slow path and do a first loop on info_get until we get an error ? > > Sending the events is not supported in 1.0 so there's nothing to > record in that case. > > Thanks, > Jens > >> >>> + >>> + 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; >>> + } >>> + >>> + fpi = ffa_rx; >>> + 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) >>> + subscr_vm_created_count++; >>> + if (fpi[n].partition_properties & FFA_PART_PROP_NOTIF_DESTROYED) >>> + 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"); >>> + subscr_vm_created_count = 0; >>> + subscr_vm_destroyed_count = 0; >>> + XFREE(subscr_vm_created); >>> + XFREE(subscr_vm_destroyed); >>> + goto out; >>> + } >>> + >>> + 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; >>> + } >>> + >>> + ret = true; >>> +out: >>> + ffa_rx_release(); >>> + >>> + return ret; >>> +} >>> + >>> static bool ffa_probe(void) >>> { >>> uint32_t vers; >>> @@ -447,7 +628,8 @@ static bool ffa_probe(void) >>> printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", >>> major_vers, minor_vers); >>> >>> - if ( >>> + if ( !check_mandatory_feature(FFA_PARTITION_INFO_GET) || >>> + !check_mandatory_feature(FFA_RX_RELEASE) || >>> #ifdef CONFIG_ARM_64 >>> !check_mandatory_feature(FFA_RXTX_MAP_64) || >>> #endif >>> @@ -475,6 +657,9 @@ static bool ffa_probe(void) >>> ffa_page_count = 1; >>> ffa_version = vers; >>> >>> + if ( !init_subscribers() ) >>> + goto err_free_ffa_tx; >>> + >>> return true; >>> >>> err_free_ffa_tx: >>> @@ -485,6 +670,10 @@ err_free_ffa_rx: >>> ffa_rx = NULL; >>> ffa_page_count = 0; >>> ffa_version = 0; >>> + XFREE(subscr_vm_created); >>> + subscr_vm_created_count = 0; >>> + XFREE(subscr_vm_destroyed); >>> + subscr_vm_destroyed_count = 0; >>> >>> return false; >>> } >>> -- >>> 2.34.1 >>> >> >> Cheers >> Bertrand
Hi, On Wed, Mar 1, 2023 at 1:58 PM Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: > > Hi Jens, > > > On 1 Mar 2023, at 11:16, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > > > Hi Bertrand, > > > > On Tue, Feb 28, 2023 at 5:49 PM Bertrand Marquis > > <Bertrand.Marquis@arm.com> wrote: > >> > >> Hi Jens, > >> > >>> On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@linaro.org> 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@linaro.org> > >>> --- > >>> xen/arch/arm/tee/ffa.c | 191 ++++++++++++++++++++++++++++++++++++++++- > >>> 1 file changed, 190 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > >>> index 07dd5c36d54b..f1b014b6c7f4 100644 > >>> --- a/xen/arch/arm/tee/ffa.c > >>> +++ b/xen/arch/arm/tee/ffa.c > >>> @@ -140,6 +140,14 @@ > >>> #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 { > >>> uint32_t guest_vers; > >>> bool interrupted; > >>> @@ -148,6 +156,12 @@ struct ffa_ctx { > >>> /* Negotiated FF-A version to use with the SPMC */ > >>> static uint32_t ffa_version __ro_after_init; > >>> > >>> +/* SPs subscribing to VM_CREATE and VM_DESTROYED events */ > >>> +static uint16_t *subscr_vm_created __read_mostly; > >>> +static unsigned int subscr_vm_created_count __read_mostly; > >> > >> In the spec the number is returned in w2 so you should utse uint32_t here. > > > > I don't understand. This value is increased for each SP which has the > > property set in the Partition information descriptor. > > Using generic types should be prevented when possible. I'm usually of the opposite opinion, fixed width integers should only be used when there's a good reason to do so. However, if this is the Xen philosophy I can replace all those unsigned int with uint32_t if that's preferable. > Here this is a subset of the number of partition which is uint32_t (wX reg) so > i think this would be the logical type for this. The maximum number is actually UINT16_MAX so an observant reader might wonder why the uint32_t type was used here. > > > > >> > >>> +static uint16_t *subscr_vm_destroyed __read_mostly; > >>> +static unsigned int subscr_vm_destroyed_count __read_mostly; > >> > >> Same here > >> > >>> + > >>> /* > >>> * Our rx/tx buffers shared with the SPMC. > >>> * > >>> @@ -237,6 +251,72 @@ static int32_t ffa_rxtx_map(register_t tx_addr, register_t rx_addr, > >>> return ffa_simple_call(fid, 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) > >> > >> This function is actually only useable to send framework created/destroyed > >> messages so the function name should be adapted to be less generic. > >> > >> ffa_send_vm_events ? > >> > >> unless you want to modify it later to send more framework messages ? > > > > That was the plan, but that may never happen. I'll rename it to > > ffa_send_vm_event() since we're only sending one event at a time. > > > >> > >>> +{ > >>> + 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 ); > >> > >> We might end up in an infinite loop here or increase interrupt response time. > >> In the general case we could return that code directly to the VM but here we > >> are in the VM creation/destroy path so we cannot do that. > >> > >> Maybe in debug mode at least we should have a retry counter here for now > >> with a print ? > > > > OK, I'll add something. > > > >> > >>> + > >>> + return res; > >>> +} > >>> + > >>> static uint16_t get_vm_id(const struct domain *d) > >>> { > >>> /* +1 since 0 is reserved for the hypervisor in FF-A */ > >>> @@ -371,6 +451,10 @@ 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; > >>> + unsigned int m; > >>> + unsigned int c_pos; > >>> + int32_t res; > >>> > >>> /* > >>> * We can't use that last possible domain ID or get_vm_id() would cause > >>> @@ -383,24 +467,121 @@ static int ffa_domain_init(struct domain *d) > >>> if ( !ctx ) > >>> return -ENOMEM; > >>> > >>> + 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); > >> > >> in this function, get_vm_id(d) will not change so i would suggest to store it in a local variable > >> instead of calling get_vm_id each time. > > > > OK > > > >> > >>> + c_pos = n; > >>> + goto err; > >>> + } > >>> + } > >>> + > >>> d->arch.tee = ctx; > >>> > >>> return 0; > >>> + > >>> +err: > >>> + /* Undo any already sent vm created messaged */ > >>> + for ( n = 0; n < c_pos; n++ ) > >>> + for ( m = 0; m < subscr_vm_destroyed_count; m++ ) > >>> + if ( subscr_vm_destroyed[m] == subscr_vm_created[n] ) > >>> + ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d), > >>> + FFA_MSG_SEND_VM_DESTROYED); > >>> + > >>> + return -ENOMEM; > >> > >> The VM creation is not failing due to missing memory here. > >> We need to find a better error code. > >> Maybe ENOTCONN ? > >> I am open to ideas here :-) > > > > That makes sense, I'll change it to ENOTCONN. > > > >> > >>> } > >>> > >>> /* This function is supposed to undo what ffa_domain_init() has done */ > >>> static int ffa_relinquish_resources(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++ ) > >>> + { > >>> + 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; > >>> } > >>> > >>> +static bool init_subscribers(void) > >>> +{ > >>> + struct ffa_partition_info_1_1 *fpi; > >>> + bool ret = false; > >>> + uint32_t count; > >>> + int e; > >>> + uint32_t n; > >>> + uint32_t c_pos; > >>> + uint32_t d_pos; > >>> + > >>> + if ( ffa_version < FFA_VERSION_1_1 ) > >>> + return true; > >> > >> Correct me if i am wrong but on 1.0 version we cannot retrieve the count but > >> we could do the slow path and do a first loop on info_get until we get an error ? > > > > Sending the events is not supported in 1.0 so there's nothing to > > record in that case. > > Please add a comment here to say that subscribers are only supported after 1.1 > and also mention it in the commit message. OK. Thanks, Jens
Hi Jens, > On 1 Mar 2023, at 17:45, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > Hi, > > On Wed, Mar 1, 2023 at 1:58 PM Bertrand Marquis > <Bertrand.Marquis@arm.com> wrote: >> >> Hi Jens, >> >>> On 1 Mar 2023, at 11:16, Jens Wiklander <jens.wiklander@linaro.org> wrote: >>> >>> Hi Bertrand, >>> >>> On Tue, Feb 28, 2023 at 5:49 PM Bertrand Marquis >>> <Bertrand.Marquis@arm.com> wrote: >>>> >>>> Hi Jens, >>>> >>>>> On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@linaro.org> 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@linaro.org> >>>>> --- >>>>> xen/arch/arm/tee/ffa.c | 191 ++++++++++++++++++++++++++++++++++++++++- >>>>> 1 file changed, 190 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c >>>>> index 07dd5c36d54b..f1b014b6c7f4 100644 >>>>> --- a/xen/arch/arm/tee/ffa.c >>>>> +++ b/xen/arch/arm/tee/ffa.c >>>>> @@ -140,6 +140,14 @@ >>>>> #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 { >>>>> uint32_t guest_vers; >>>>> bool interrupted; >>>>> @@ -148,6 +156,12 @@ struct ffa_ctx { >>>>> /* Negotiated FF-A version to use with the SPMC */ >>>>> static uint32_t ffa_version __ro_after_init; >>>>> >>>>> +/* SPs subscribing to VM_CREATE and VM_DESTROYED events */ >>>>> +static uint16_t *subscr_vm_created __read_mostly; >>>>> +static unsigned int subscr_vm_created_count __read_mostly; >>>> >>>> In the spec the number is returned in w2 so you should utse uint32_t here. >>> >>> I don't understand. This value is increased for each SP which has the >>> property set in the Partition information descriptor. >> >> Using generic types should be prevented when possible. > > I'm usually of the opposite opinion, fixed width integers should only > be used when there's a good reason to do so. However, if this is the > Xen philosophy I can replace all those unsigned int with uint32_t if > that's preferable. Safety standards usually enforce to use explicit size types to prevent compiler dependencies. > >> Here this is a subset of the number of partition which is uint32_t (wX reg) so >> i think this would be the logical type for this. > > The maximum number is actually UINT16_MAX so an observant reader might > wonder why the uint32_t type was used here. Switching to uint16_t might make sense then but you will have to check that you are not going over UINT16_MAX in the code as you get a uint32_t back from the call. Cheers Bertrand > >> >>> >>>> >>>>> +static uint16_t *subscr_vm_destroyed __read_mostly; >>>>> +static unsigned int subscr_vm_destroyed_count __read_mostly; >>>> >>>> Same here >>>> >>>>> + >>>>> /* >>>>> * Our rx/tx buffers shared with the SPMC. >>>>> * >>>>> @@ -237,6 +251,72 @@ static int32_t ffa_rxtx_map(register_t tx_addr, register_t rx_addr, >>>>> return ffa_simple_call(fid, 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) >>>> >>>> This function is actually only useable to send framework created/destroyed >>>> messages so the function name should be adapted to be less generic. >>>> >>>> ffa_send_vm_events ? >>>> >>>> unless you want to modify it later to send more framework messages ? >>> >>> That was the plan, but that may never happen. I'll rename it to >>> ffa_send_vm_event() since we're only sending one event at a time. >>> >>>> >>>>> +{ >>>>> + 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 ); >>>> >>>> We might end up in an infinite loop here or increase interrupt response time. >>>> In the general case we could return that code directly to the VM but here we >>>> are in the VM creation/destroy path so we cannot do that. >>>> >>>> Maybe in debug mode at least we should have a retry counter here for now >>>> with a print ? >>> >>> OK, I'll add something. >>> >>>> >>>>> + >>>>> + return res; >>>>> +} >>>>> + >>>>> static uint16_t get_vm_id(const struct domain *d) >>>>> { >>>>> /* +1 since 0 is reserved for the hypervisor in FF-A */ >>>>> @@ -371,6 +451,10 @@ 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; >>>>> + unsigned int m; >>>>> + unsigned int c_pos; >>>>> + int32_t res; >>>>> >>>>> /* >>>>> * We can't use that last possible domain ID or get_vm_id() would cause >>>>> @@ -383,24 +467,121 @@ static int ffa_domain_init(struct domain *d) >>>>> if ( !ctx ) >>>>> return -ENOMEM; >>>>> >>>>> + 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); >>>> >>>> in this function, get_vm_id(d) will not change so i would suggest to store it in a local variable >>>> instead of calling get_vm_id each time. >>> >>> OK >>> >>>> >>>>> + c_pos = n; >>>>> + goto err; >>>>> + } >>>>> + } >>>>> + >>>>> d->arch.tee = ctx; >>>>> >>>>> return 0; >>>>> + >>>>> +err: >>>>> + /* Undo any already sent vm created messaged */ >>>>> + for ( n = 0; n < c_pos; n++ ) >>>>> + for ( m = 0; m < subscr_vm_destroyed_count; m++ ) >>>>> + if ( subscr_vm_destroyed[m] == subscr_vm_created[n] ) >>>>> + ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d), >>>>> + FFA_MSG_SEND_VM_DESTROYED); >>>>> + >>>>> + return -ENOMEM; >>>> >>>> The VM creation is not failing due to missing memory here. >>>> We need to find a better error code. >>>> Maybe ENOTCONN ? >>>> I am open to ideas here :-) >>> >>> That makes sense, I'll change it to ENOTCONN. >>> >>>> >>>>> } >>>>> >>>>> /* This function is supposed to undo what ffa_domain_init() has done */ >>>>> static int ffa_relinquish_resources(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++ ) >>>>> + { >>>>> + 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; >>>>> } >>>>> >>>>> +static bool init_subscribers(void) >>>>> +{ >>>>> + struct ffa_partition_info_1_1 *fpi; >>>>> + bool ret = false; >>>>> + uint32_t count; >>>>> + int e; >>>>> + uint32_t n; >>>>> + uint32_t c_pos; >>>>> + uint32_t d_pos; >>>>> + >>>>> + if ( ffa_version < FFA_VERSION_1_1 ) >>>>> + return true; >>>> >>>> Correct me if i am wrong but on 1.0 version we cannot retrieve the count but >>>> we could do the slow path and do a first loop on info_get until we get an error ? >>> >>> Sending the events is not supported in 1.0 so there's nothing to >>> record in that case. >> >> Please add a comment here to say that subscribers are only supported after 1.1 >> and also mention it in the commit message. > > OK. > > Thanks, > Jens
diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c index 07dd5c36d54b..f1b014b6c7f4 100644 --- a/xen/arch/arm/tee/ffa.c +++ b/xen/arch/arm/tee/ffa.c @@ -140,6 +140,14 @@ #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 { uint32_t guest_vers; bool interrupted; @@ -148,6 +156,12 @@ struct ffa_ctx { /* Negotiated FF-A version to use with the SPMC */ static uint32_t ffa_version __ro_after_init; +/* SPs subscribing to VM_CREATE and VM_DESTROYED events */ +static uint16_t *subscr_vm_created __read_mostly; +static unsigned int subscr_vm_created_count __read_mostly; +static uint16_t *subscr_vm_destroyed __read_mostly; +static unsigned int subscr_vm_destroyed_count __read_mostly; + /* * Our rx/tx buffers shared with the SPMC. * @@ -237,6 +251,72 @@ static int32_t ffa_rxtx_map(register_t tx_addr, register_t rx_addr, return ffa_simple_call(fid, 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; + 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 ); + + return res; +} + static uint16_t get_vm_id(const struct domain *d) { /* +1 since 0 is reserved for the hypervisor in FF-A */ @@ -371,6 +451,10 @@ 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; + unsigned int m; + unsigned int c_pos; + int32_t res; /* * We can't use that last possible domain ID or get_vm_id() would cause @@ -383,24 +467,121 @@ static int ffa_domain_init(struct domain *d) if ( !ctx ) return -ENOMEM; + 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); + c_pos = n; + goto err; + } + } + d->arch.tee = ctx; return 0; + +err: + /* Undo any already sent vm created messaged */ + for ( n = 0; n < c_pos; n++ ) + for ( m = 0; m < subscr_vm_destroyed_count; m++ ) + if ( subscr_vm_destroyed[m] == subscr_vm_created[n] ) + ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d), + FFA_MSG_SEND_VM_DESTROYED); + + return -ENOMEM; } /* This function is supposed to undo what ffa_domain_init() has done */ static int ffa_relinquish_resources(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++ ) + { + 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; } +static bool init_subscribers(void) +{ + struct ffa_partition_info_1_1 *fpi; + bool ret = false; + uint32_t count; + int e; + uint32_t n; + uint32_t c_pos; + uint32_t d_pos; + + if ( ffa_version < FFA_VERSION_1_1 ) + return true; + + 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; + } + + fpi = ffa_rx; + 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) + subscr_vm_created_count++; + if (fpi[n].partition_properties & FFA_PART_PROP_NOTIF_DESTROYED) + 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"); + subscr_vm_created_count = 0; + subscr_vm_destroyed_count = 0; + XFREE(subscr_vm_created); + XFREE(subscr_vm_destroyed); + goto out; + } + + 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; + } + + ret = true; +out: + ffa_rx_release(); + + return ret; +} + static bool ffa_probe(void) { uint32_t vers; @@ -447,7 +628,8 @@ static bool ffa_probe(void) printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", major_vers, minor_vers); - if ( + if ( !check_mandatory_feature(FFA_PARTITION_INFO_GET) || + !check_mandatory_feature(FFA_RX_RELEASE) || #ifdef CONFIG_ARM_64 !check_mandatory_feature(FFA_RXTX_MAP_64) || #endif @@ -475,6 +657,9 @@ static bool ffa_probe(void) ffa_page_count = 1; ffa_version = vers; + if ( !init_subscribers() ) + goto err_free_ffa_tx; + return true; err_free_ffa_tx: @@ -485,6 +670,10 @@ err_free_ffa_rx: ffa_rx = NULL; ffa_page_count = 0; ffa_version = 0; + XFREE(subscr_vm_created); + subscr_vm_created_count = 0; + XFREE(subscr_vm_destroyed); + subscr_vm_destroyed_count = 0; return false; }
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@linaro.org> --- xen/arch/arm/tee/ffa.c | 191 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 190 insertions(+), 1 deletion(-)