Re: [PATCH hyperv-next v6 01/11] arm64: kvm, smccc: Introduce and use API for detecting hypervisor presence
From: Roman Kisel
Date: Mon Mar 17 2025 - 13:16:58 EST
On 3/17/2025 4:29 AM, Mark Rutland wrote:
On Fri, Mar 14, 2025 at 05:19:21PM -0700, Roman Kisel wrote:
[...]
+}
This use of a statement expression is bizarre, and the function would be
clearer without it, e.g.
I'll change that to what you're suggesting, thanks for your help!
| bool arm_smccc_hyp_present(const uuid_t *hyp_uuid)
| {
| struct arm_smccc_res res = {};
| uuid_t uuid;
|
| if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
| return false;
|
| arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
| if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
| return false;
|
| uuid_t = SMCCC_RES_TO_UUID(res.a0, res.a1, res.a2, res.a3);
| return uuid_equal(&uuid, hyp_uuid);
| }
As noted below, I'd prefer if this were renamed to something like
arm_smccc_hypervisor_has_uuid(), to more clearly indicate what is being
checked.
[...]
Will update, thanks for your help!
+/**
+ * arm_smccc_hyp_present(const uuid_t *hyp_uuid)
+ *
+ * Returns `true` if the hypervisor advertises its presence via SMCCC.
+ *
+ * When the function returns `false`, the caller shall not assume that
+ * there is no hypervisor running. Instead, the caller must fall back to
+ * other approaches if any are available.
+ */
+bool arm_smccc_hyp_present(const uuid_t *hyp_uuid);
I'd prefer if this were:
| /*
| * Returns whether a specific hypervisor UUID is advertised for the
| * Vendor Specific Hypervisor Service range.
| */
| bool arm_smccc_hypervisor_has_uuid(const uuid_t *uuid);
[...]
I think this'd be clearer if we did something similar to what we did for
the SMCCC SOC_ID name:
https://lore.kernel.org/linux-arm-kernel/20250219005932.3466-1-paul@xxxxxxxxxxxxxxxxxxxxxx/
... and pack/unpack the bytes explicitly, e.g.
That looks great, thanks for the suggestion!
| static inline uuid smccc_res_to_uuid(u32 r0, u32, r1, u32 r2, u32 r3)
| {
| uuid_t uuid = {
| .b = {
| [0] = (r0 >> 0) & 0xff,
| [1] = (r0 >> 8) & 0xff,
| [2] = (r0 >> 16) & 0xff,
| [3] = (r0 >> 24) & 0xff,
|
| [4] = (r1 >> 0) & 0xff,
| [5] = (r1 >> 8) & 0xff,
| [6] = (r1 >> 16) & 0xff,
| [7] = (r1 >> 24) & 0xff,
|
| [8] = (r2 >> 0) & 0xff,
| [9] = (r2 >> 8) & 0xff,
| [10] = (r2 >> 16) & 0xff,
| [11] = (r2 >> 24) & 0xff,
|
| [12] = (r3 >> 0) & 0xff,
| [13] = (r3 >> 8) & 0xff,
| [14] = (r3 >> 16) & 0xff,
| [15] = (r3 >> 24) & 0xff,
| },
| };
|
| return uuid;
| }
... which is a bit more verbose, but clearly aligns with what the SMCCC
spec says w.r.t. packing/unpacking, and should avoid warnings about
endianness conversions.
I believe what you're proposing hits a better trade-off, thanks again!
+
+#define UUID_TO_SMCCC_RES(uuid_init, regs) do { \
+ const uuid_t uuid = uuid_init; \
+ (regs)[0] = le32_to_cpu((u32)uuid.b[0] | (uuid.b[1] << 8) | \
+ ((uuid.b[2]) << 16) | ((uuid.b[3]) << 24)); \
+ (regs)[1] = le32_to_cpu((u32)uuid.b[4] | (uuid.b[5] << 8) | \
+ ((uuid.b[6]) << 16) | ((uuid.b[7]) << 24)); \
+ (regs)[2] = le32_to_cpu((u32)uuid.b[8] | (uuid.b[9] << 8) | \
+ ((uuid.b[10]) << 16) | ((uuid.b[11]) << 24)); \
+ (regs)[3] = le32_to_cpu((u32)uuid.b[12] | (uuid.b[13] << 8) | \
+ ((uuid.b[14]) << 16) | ((uuid.b[15]) << 24)); \
+ } while (0)
+
+#endif /* !__ASSEMBLER__ */
IMO it'd be clearer to initialise a uuid_t beforehand, and then allow
the helper to unpack the bytes, e.g.
static inline u32 smccc_uuid_to_reg(const uuid_t uuid, int reg)
{
u32 val = 0;
val |= (u32)(uuid.b[4 * reg + 0] << 0)
val |= (u32)(uuid.b[4 * reg + 1] << 8)
val |= (u32)(uuid.b[4 * reg + 2] << 16)
val |= (u32)(uuid.b[4 * reg + 3] << 24)
return val:
}
#define UUID_TO_SMCCC_RES(uuid, regs) \
do { \
(regs)[0] = smccc_uuid_to_reg(uuid, 0); \
(regs)[1] = smccc_uuid_to_reg(uuid, 1); \
(regs)[2] = smccc_uuid_to_reg(uuid, 2); \
(regs)[3] = smccc_uuid_to_reg(uuid, 3); \
} while (0)
... though arguably at that point you can get rid of the
UUID_TO_SMCCC_RES() macro and just expand that directly at the callsite.
I'll work on that, thanks!!
Mark.
--
Thank you,
Roman