[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 03/10] xen/arm: ffa: fix version negotiation
On 24/09/2024 09:23, Bertrand Marquis wrote:
Hi Julien,
Hi Bertrand,
On 22 Sep 2024, at 14:25, Julien Grall <julien@xxxxxxx> wrote:
Hi Bertrand,
NIT Typo: s/fix/Fix/ to match the other title
Ack
On 19/09/2024 14:19, Bertrand Marquis wrote:
Fix FFA version negotiation with the firmware to follow the
specification guidance more closely.
To confirm, below is based on 13.2.1 in DEN0077A, is that correct? If so, can
you add a link in the commit message (and maybe code).
Yes it and i will add a link and description to the commit message.
When the firmware returns OK we can have several cases:
- the version requested is accepted but the firmware supports a greater
one in the same major.
- the firmware supports a greater major version. It could still return
OK even if the version requested is not accepted. Reject it.
- the firmware supports a lower version. It will return OK and give that
version. Check if we support it and use it or reject it if we do not.
Adapt the code to:
- reject any version lower than the one we support or not with the same
major version
- use the version returned if in our supported range (currently 1.1
only)
- use 1.1 if the version returned is greater.
Also adapt the handling of version requests from VM:
- return an error for a different major
- return 1.1 for a version >= 1.1
- return 1.0 if 1.0 was requested
Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
---
xen/arch/arm/tee/ffa.c | 38 ++++++++++++++++++++++++++++++--------
1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 7ff2529b2055..1f602f25d097 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -141,13 +141,24 @@ static void handle_version(struct cpu_user_regs *regs)
struct ffa_ctx *ctx = d->arch.tee;
uint32_t vers = get_user_reg(regs, 1);
- if ( vers < FFA_VERSION_1_1 )
- vers = FFA_VERSION_1_0;
- else
- vers = FFA_VERSION_1_1;
+ /**
Coding style: We are use a single '*' to start comment.
Ack
+ * As of now we only support 1.0 or 1.1.
+ * For any 1.x >= 1.1 return OK with 1.1
+ * For 1.0 return OK with 1.0
+ * For anything else return an error.
+ */
+ if ( (vers >> FFA_VERSION_MAJOR_SHIFT) == FFA_MY_VERSION_MAJOR )
+ {> + if ( vers < FFA_VERSION_1_1 )
+ vers = FFA_VERSION_1_0;
+ else
+ vers = FFA_VERSION_1_1;
I feel the logic is fragile. The first ``if`` is generic and I think it would
be easy to update the major version without updating handle_version(). To some
extend, the same problem would happen with the minor version.
so something like:
if (MAJOR(vers) == MY_MAJOR)
{
if (MINOR(vers) < MY_MIN || MINOR(vers)>MY_MIN)
vers = MY_VERSION
else
keep requested version
}
AFAICT, this is not a new issue, but as you touch the code, we should probably
harden it. I could settle with a BUILD_BUG_ON() to catch any change of the
minor/major.
i could see a BUILD_BUG_ON(MAJOR(MIN_VERSION) != MAJOR(MAX_VERSION))
Is that what you have in mind ?
I had in mind to check specifically that FFA_MY_VERSION_{MAJOR, MINOR}
were both 1. But I think your proposal is better.
- ctx->guest_vers = vers;
- ffa_set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
+ ctx->guest_vers = vers;
+ ffa_set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
+ }
+ else
+ ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
}
static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t
fid)
@@ -530,7 +541,8 @@ static bool ffa_probe(void)
goto err_no_fw;
}
- if ( vers < FFA_MIN_SPMC_VERSION || vers > FFA_MY_VERSION )
+ if ( vers < FFA_MIN_SPMC_VERSION ||
+ (vers >> FFA_VERSION_MAJOR_SHIFT) != FFA_MY_VERSION_MAJOR )
Coding style: the second line should be aligned with 'vers' rather than
indented.
Ack
{
printk(XENLOG_ERR "ffa: Incompatible version %#x found\n", vers);
goto err_no_fw;
@@ -542,7 +554,17 @@ static bool ffa_probe(void)
printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
major_vers, minor_vers);
- ffa_fw_version = vers;
+ /**
Coding style: We start comment with /*.
Ack
+ * If the call succeed and the version returned is higher or equal to
+ * the one Xen requested, the version requested by Xen will be the one
+ * used. If the version returned is lower but compatible with Xen, Xen
+ * will use that version instead.
+ * A version with a different major is rejected before.
+ */
+ if ( vers > FFA_MY_VERSION )
+ ffa_fw_version = FFA_MY_VERSION;
+ else
+ ffa_fw_version = vers;
Looking at the code after your series (didn't check before). We don't seem to
use ffa_fw_version for other than checking that FFA was detected. So wouldn't
it be better to stop storing the version?
We are only supporting a firmware version with 1.1 at the moment but when we
will add support for FFA version 1.2 in the next weeks this will not be true
anymore so if this is ok with you i would rather keep it.
I am fine to keep ffa_fw_version as-is given there is a future use.
Cheers,
--
Julien Grall
|
