| Message ID | 20230929-ffa_v1-1_notif-v3-3-c8e4f15190c8@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: arm_ffa: Add FF-A v1.1 support(notification + new memory descriptor format) | expand |
On Fri, Sep 29, 2023 at 04:02:52PM +0100, Sudeep Holla wrote: > A receiver endpoint must bind a notification to any sender endpoint > before the latter can signal the notification to the former. The receiver > assigns one or more doorbells to a specific sender. Only the sender can > ring these doorbells. > > A receiver uses the FFA_NOTIFICATION_BIND interface to bind one or more > notifications to the sender. A receiver un-binds a notification from a > sender endpoint to stop the notification from being signaled. It uses > the FFA_NOTIFICATION_UNBIND interface to do this. > > Allow the FF-A driver to be able to bind and unbind a given notification > ID to a specific partition ID. This will be used to register and > unregister notification callbacks from the FF-A client drivers. > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/arm_ffa/driver.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c > index efa4e7fb15e3..26bf9c4e3b5f 100644 > --- a/drivers/firmware/arm_ffa/driver.c > +++ b/drivers/firmware/arm_ffa/driver.c > @@ -587,6 +587,35 @@ static int ffa_notification_bitmap_destroy(void) > return 0; > } > > +#define NOTIFICATION_LOW_MASK GENMASK(31, 0) > +#define NOTIFICATION_HIGH_MASK GENMASK(63, 32) > +#define NOTIFICATION_BITMAP_HIGH(x) \ > + ((u32)(FIELD_GET(NOTIFICATION_HIGH_MASK, (x)))) > +#define NOTIFICATION_BITMAP_LOW(x) \ > + ((u32)(FIELD_GET(NOTIFICATION_LOW_MASK, (x)))) > + > +static int ffa_notification_bind_common(u16 dst_id, u64 bitmap, > + u32 flags, bool is_bind) > +{ > + ffa_value_t ret; > + u32 func, src_dst_ids = PACK_TARGET_INFO(dst_id, drv_info->vm_id); dst_id and drv_info->vm_id should be swapped. Thanks, Jens > + > + func = is_bind ? FFA_NOTIFICATION_BIND : FFA_NOTIFICATION_UNBIND; > + > + invoke_ffa_fn((ffa_value_t){ > + .a0 = func, .a1 = src_dst_ids, .a2 = flags, > + .a3 = NOTIFICATION_BITMAP_LOW(bitmap), > + .a4 = NOTIFICATION_BITMAP_HIGH(bitmap), > + }, &ret); > + > + if (ret.a0 == FFA_ERROR) > + return ffa_to_linux_errno((int)ret.a2); > + else if (ret.a0 != FFA_SUCCESS) > + return -EINVAL; > + > + return 0; > +} > + > static void ffa_set_up_mem_ops_native_flag(void) > { > if (!ffa_features(FFA_FN_NATIVE(MEM_LEND), 0, NULL, NULL) || > > -- > 2.42.0 >
Hi Jens,
> dst_id and drv_info->vm_id should be swapped.
I'm curious about this because swapping like this actually makes hafnium fail. Need to check from the spec.
Regards,
Olivier.
On Wed, Oct 04, 2023 at 10:50:26AM +0100, Olivier Deprez wrote: > Hi Jens, > > > dst_id and drv_info->vm_id should be swapped. > > I'm curious about this because swapping like this actually makes hafnium > fail. Need to check from the spec. I did check after I had swapped this in v2(because I was convinced Jens) was correct and you reported the failure. Reading the spec again the other day, I got corrected myself and agreed with Olivier and my original implementation(v1) which matches this patch(v3).
Hi Sudeep, On Wed, Oct 4, 2023 at 5:32 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, Oct 04, 2023 at 10:50:26AM +0100, Olivier Deprez wrote: > > Hi Jens, > > > > > dst_id and drv_info->vm_id should be swapped. > > > > I'm curious about this because swapping like this actually makes hafnium > > fail. Need to check from the spec. > > I did check after I had swapped this in v2(because I was convinced Jens) was > correct and you reported the failure. Reading the spec again the other day, > I got corrected myself and agreed with Olivier and my original > implementation(v1) which matches this patch(v3). I don't get it. The spec says for FFA_NOTIFICATION_BIND: Sender and Receiver endpoint IDs. – Bit[31:16]: Sender endpoint ID. – Bit[15:0]: Receiver endpoint ID. This is exactly the same as for instance FFA_MSG_SEND_DIRECT_REQ. In ffa_msg_send_direct_req() you assign src_dst_ids = PACK_TARGET_INFO(src_id, dst_id); but here in ffa_notification_bind_common() you assign src_dst_ids = PACK_TARGET_INFO(dst_id, drv_info->vm_id); Thanks, Jens
On Thu, Oct 05, 2023 at 08:57:26AM +0200, Jens Wiklander wrote: > Hi Sudeep, > > On Wed, Oct 4, 2023 at 5:32 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Wed, Oct 04, 2023 at 10:50:26AM +0100, Olivier Deprez wrote: > > > Hi Jens, > > > > > > > dst_id and drv_info->vm_id should be swapped. > > > > > > I'm curious about this because swapping like this actually makes hafnium > > > fail. Need to check from the spec. > > > > I did check after I had swapped this in v2(because I was convinced Jens) was > > correct and you reported the failure. Reading the spec again the other day, > > I got corrected myself and agreed with Olivier and my original > > implementation(v1) which matches this patch(v3). Well, I am not exactly sure what is the root cause for the confusion here: My poor choice of variable names and their usage with this macro, or the macro definition itself(I am not sure) OR The wordings in the specification > > I don't get it. The spec says for FFA_NOTIFICATION_BIND: > Sender and Receiver endpoint IDs. > – Bit[31:16]: Sender endpoint ID. > – Bit[15:0]: Receiver endpoint ID. > This is exactly the same as for instance FFA_MSG_SEND_DIRECT_REQ. > Not really as per my understanding of the specification. > In ffa_msg_send_direct_req() you assign > src_dst_ids = PACK_TARGET_INFO(src_id, dst_id); > Correct and if you look at the callsite, it is ffa_msg_send_direct_req(drv_info->vm_id, dev->vm_id,...) So the driver is the sender and the partition is the receiver. Probably this is simpler. > but here in ffa_notification_bind_common() you assign > src_dst_ids = PACK_TARGET_INFO(dst_id, drv_info->vm_id); > A receiver(FF-A driver) must bind a non-framework notification to a sender(SP) before the latter can signal the notification to the former. Only the sender can ring these doorbells. A receiver uses the FFA_NOTIFICATION_BIND interface to bind one or more notifications to the sender. So, based on this text(modified to refer sender and receiver in the driver context) from the spec, my understanding is the driver is the receiver and the SP is the sender of the notification. Do you think I am missing someting here ? Sorry for agreeing with you in v2 and silently changing it back without this actual discussion. Olivier raised the issue and then when I went back and looked at the spec, I realised why I had it this way from the beginning.
On Thu, Oct 5, 2023 at 10:51 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Thu, Oct 05, 2023 at 08:57:26AM +0200, Jens Wiklander wrote: > > Hi Sudeep, > > > > On Wed, Oct 4, 2023 at 5:32 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > On Wed, Oct 04, 2023 at 10:50:26AM +0100, Olivier Deprez wrote: > > > > Hi Jens, > > > > > > > > > dst_id and drv_info->vm_id should be swapped. > > > > > > > > I'm curious about this because swapping like this actually makes hafnium > > > > fail. Need to check from the spec. > > > > > > I did check after I had swapped this in v2(because I was convinced Jens) was > > > correct and you reported the failure. Reading the spec again the other day, > > > I got corrected myself and agreed with Olivier and my original > > > implementation(v1) which matches this patch(v3). > > Well, I am not exactly sure what is the root cause for the confusion here: > My poor choice of variable names and their usage with this macro, or the > macro definition itself(I am not sure) > > OR > > The wordings in the specification > > > > > I don't get it. The spec says for FFA_NOTIFICATION_BIND: > > Sender and Receiver endpoint IDs. > > – Bit[31:16]: Sender endpoint ID. > > – Bit[15:0]: Receiver endpoint ID. > > This is exactly the same as for instance FFA_MSG_SEND_DIRECT_REQ. > > > > Not really as per my understanding of the specification. > > > In ffa_msg_send_direct_req() you assign > > src_dst_ids = PACK_TARGET_INFO(src_id, dst_id); > > > > Correct and if you look at the callsite, it is > ffa_msg_send_direct_req(drv_info->vm_id, dev->vm_id,...) > > So the driver is the sender and the partition is the receiver. Probably > this is simpler. > > > but here in ffa_notification_bind_common() you assign > > src_dst_ids = PACK_TARGET_INFO(dst_id, drv_info->vm_id); > > > > A receiver(FF-A driver) must bind a non-framework notification to a > sender(SP) before the latter can signal the notification to the former. > Only the sender can ring these doorbells. A receiver uses the > FFA_NOTIFICATION_BIND interface to bind one or more notifications to the > sender. > > So, based on this text(modified to refer sender and receiver in the driver > context) from the spec, my understanding is the driver is the receiver > and the SP is the sender of the notification. > > Do you think I am missing someting here ? Sorry for agreeing with you > in v2 and silently changing it back without this actual discussion. > Olivier raised the issue and then when I went back and looked at the > spec, I realised why I had it this way from the beginning. Thanks for the explanation, now I get it. My mistake was that I thought that sender and receiver meant the sender and receiver of the actual message being sent like with a direct request, it is using the same register and the same wording after all. Instead, it means the sender and receiver of an eventual notification, which of course is the exact opposite. Thanks, Jens
diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c index efa4e7fb15e3..26bf9c4e3b5f 100644 --- a/drivers/firmware/arm_ffa/driver.c +++ b/drivers/firmware/arm_ffa/driver.c @@ -587,6 +587,35 @@ static int ffa_notification_bitmap_destroy(void) return 0; } +#define NOTIFICATION_LOW_MASK GENMASK(31, 0) +#define NOTIFICATION_HIGH_MASK GENMASK(63, 32) +#define NOTIFICATION_BITMAP_HIGH(x) \ + ((u32)(FIELD_GET(NOTIFICATION_HIGH_MASK, (x)))) +#define NOTIFICATION_BITMAP_LOW(x) \ + ((u32)(FIELD_GET(NOTIFICATION_LOW_MASK, (x)))) + +static int ffa_notification_bind_common(u16 dst_id, u64 bitmap, + u32 flags, bool is_bind) +{ + ffa_value_t ret; + u32 func, src_dst_ids = PACK_TARGET_INFO(dst_id, drv_info->vm_id); + + func = is_bind ? FFA_NOTIFICATION_BIND : FFA_NOTIFICATION_UNBIND; + + invoke_ffa_fn((ffa_value_t){ + .a0 = func, .a1 = src_dst_ids, .a2 = flags, + .a3 = NOTIFICATION_BITMAP_LOW(bitmap), + .a4 = NOTIFICATION_BITMAP_HIGH(bitmap), + }, &ret); + + if (ret.a0 == FFA_ERROR) + return ffa_to_linux_errno((int)ret.a2); + else if (ret.a0 != FFA_SUCCESS) + return -EINVAL; + + return 0; +} + static void ffa_set_up_mem_ops_native_flag(void) { if (!ffa_features(FFA_FN_NATIVE(MEM_LEND), 0, NULL, NULL) ||
A receiver endpoint must bind a notification to any sender endpoint before the latter can signal the notification to the former. The receiver assigns one or more doorbells to a specific sender. Only the sender can ring these doorbells. A receiver uses the FFA_NOTIFICATION_BIND interface to bind one or more notifications to the sender. A receiver un-binds a notification from a sender endpoint to stop the notification from being signaled. It uses the FFA_NOTIFICATION_UNBIND interface to do this. Allow the FF-A driver to be able to bind and unbind a given notification ID to a specific partition ID. This will be used to register and unregister notification callbacks from the FF-A client drivers. Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/firmware/arm_ffa/driver.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)