Re: [PATCH v3 2/6] firmware: smccc: Detect hypervisor via RSI host call in CCA Realms

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing compiler memory barrier causes dead-store elimination and constant-folding of the RSI host call buffer.
--

--- Patch [2]: [PATCH v3 2/6] firmware: smccc: Detect hypervisor via RSI host call in CCA Realms ---
commit 5f8398cb1bc3327a58d70463ef37d994ffacca02
Author: Kameron Carr <[email protected]>

firmware: smccc: Detect hypervisor via RSI host call in CCA Realms

This commit modifies arm_smccc_hypervisor_has_uuid() to use rsi_host_call()
when operating in a Realm. It introduces arm_smccc_realm_get_hypervisor_uuid()
which uses a spinlock-protected file-static buffer to make the call.

> diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
> index bdee057db2fd3..a876b7aa2dc99 100644
> --- a/drivers/firmware/smccc/smccc.c
> +++ b/drivers/firmware/smccc/smccc.c
[ ... ]
> @@ -67,12 +73,45 @@ s32 arm_smccc_get_soc_id_revision(void)
>  }
>  EXPORT_SYMBOL_GPL(arm_smccc_get_soc_id_revision);
>  
> +#ifdef CONFIG_ARM64
> +static struct rsi_host_call uuid_hc;
> +static DEFINE_SPINLOCK(uuid_hc_lock);
> +
> +/*
> + * Helper function to get the hypervisor UUID via an RsiHostCall.
> + */
> +static void arm_smccc_realm_get_hypervisor_uuid(struct arm_smccc_res *res)
> +{
> +	guard(spinlock_irqsave)(&uuid_hc_lock);
> +
> +	memset(&uuid_hc, 0, sizeof(uuid_hc));
> +	uuid_hc.gprs[0] = ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID;
> +
> +	if (rsi_host_call(__pa_symbol(&uuid_hc)) != RSI_SUCCESS) {
> +		res->a0 = SMCCC_RET_NOT_SUPPORTED;
> +		return;
> +	}
> +
> +	res->a0 = uuid_hc.gprs[0];

[Severity: High]
Does this code need a compiler memory barrier before reading back the results?

Because __pa_symbol() uses RELOC_HIDE() to explicitly hide the pointer's
provenance from the compiler's alias analysis, the compiler may not realize
that rsi_host_call() modifies uuid_hc.

Without a barrier(), could the compiler constant-fold the reads to their
statically-known previous values? If so, this could result in res->a0 being
optimized to ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID instead of the value
returned by the hypervisor, and potentially allow the stores during
initialization to be dead-store eliminated.

> +	res->a1 = uuid_hc.gprs[1];
> +	res->a2 = uuid_hc.gprs[2];
> +	res->a3 = uuid_hc.gprs[3];
> +}
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2