Re: [PATCH v6 3/4] firmware: smccc: arm-cca-guest: Bind the TSM provider to an SMCCC device

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Sudeep Holla <sudeep.holla@kernel.org> writes:

> On Thu, Jun 04, 2026 at 06:56:28PM +0530, Aneesh Kumar K.V wrote:
>> Sudeep Holla <sudeep.holla@kernel.org> writes:
>> 
>> ...
>> 
>> > +static const struct smccc_device_info smccc_devices[] __initconst = {
>> > +       {
>> > +               .func_id        = ARM_SMCCC_TRNG_VERSION,
>> > +               .requires_smc   = false,
>> > +               .min_return     = ARM_SMCCC_TRNG_MIN_VERSION,
>> > +               .device_name    = "arm-smccc-trng",
>> > +       },
>> > +};
>> > +
>> > +static bool __init
>> > +smccc_probe_smccc_device(const struct smccc_device_info *smccc_dev)
>> > +{
>> > +       struct arm_smccc_res res;
>> > +       unsigned long ret;
>> > +
>> > +       if (!IS_ENABLED(CONFIG_ARM64))
>> > +               return false;
>> > +
>> > +       if (smccc_conduit == SMCCC_CONDUIT_NONE)
>> > +               return false;
>> > +
>> > +       if (smccc_dev->requires_smc && smccc_conduit != SMCCC_CONDUIT_SMC)
>> > +               return false;
>> > +
>> > +       arm_smccc_1_1_invoke(smccc_dev->func_id, &res);
>> > +       ret = res.a0;
>> > +
>> > +       if ((s32)ret < 0)
>> > +               return false;
>> > +
>> > +       return ret >= smccc_dev->min_return;
>> > +}
>> > +
>> >
>> 
>> I am not sure we want the check to be as simple as ret < 0. Some
>> function IDs may return input errors based on the supplied arguments
>> (for example, RMI_ERROR_INPUT). In those cases, we would likely want
>> this to be handled via a callback.
>> 
>
> As I mentioned in response to Suzuki, we can defer that to probe of
> that device. If *_VERSION, succeeds SMCCC core can add that device and
> leave the rest to the core keeping the core and bus layer simple IMO.
>
>> We also want to use conditional compilation for some function IDs.
>> Given the callback approach and the #ifdefs, I wonder whether what we
>> currently have is actually simpler and more flexible.”
>> 
>
> I was trying to avoid conditional compilation altogether and hence the
> reason for keeping it as simple as possible. Also IS_ENABLED(CONFIG_ARM64)
> in above snippet must come as some condition to this generic probe.
>
> Adding any more logic or callback defeats the bus idea here if we need
> to rely/depend on multiple conditional compilation or callbacks IMO.
>
> Let's find see if it can work with what we are adding now and may add in
> near future and then decide.
>

If we move all the conditional checks to the driver probe path, then I
think this can work. Something like the below:

struct smccc_device_info {
	u32 func_id;
	bool requires_smc;
	const char *device_name;
};

static const struct smccc_device_info smccc_devices[] __initconst = {
	{
		.func_id        = ARM_SMCCC_TRNG_VERSION,
		.requires_smc   = false,
		.device_name    = "arm-smccc-trng",
	},

	{
		.func_id        = RSI_ABI_VERSION,
		.requires_smc   = true,
		.device_name    = RSI_DEV_NAME,
	},
};

static bool __init smccc_probe_smccc_device(const struct smccc_device_info *smccc_dev)
{
	unsigned long ret;
	struct arm_smccc_res res;

	if (smccc_conduit == SMCCC_CONDUIT_NONE)
		return false;

	if (smccc_dev->requires_smc && smccc_conduit != SMCCC_CONDUIT_SMC)
		return false;

	arm_smccc_1_1_invoke(smccc_dev->func_id, &res);
	ret = res.a0;

	if ((s32)ret == SMCCC_RET_NOT_SUPPORTED)
		return false;

	return true;
}

static int __init smccc_devices_init(void)
{
	struct arm_smccc_device *sdev;
	const struct smccc_device_info *smccc_dev;

	for (int i = 0; i < ARRAY_SIZE(smccc_devices); i++) {
		smccc_dev = &smccc_devices[i];

		if (!smccc_probe_smccc_device(smccc_dev))
			continue;

               sdev = arm_smccc_device_register(smccc_dev->device_name);
               if (IS_ERR(sdev))
                       pr_err("%s: could not register device: %ld\n",
                              smccc_dev->device_name, PTR_ERR(sdev));

	}

	return 0;
}
device_initcall(smccc_devices_init);

with the diff to hw_random/smccc_trng

modified   arch/arm64/include/asm/archrandom.h
@@ -12,7 +12,7 @@
 
 extern bool smccc_trng_available;
 
-static inline bool __init smccc_probe_trng(void)
+static inline bool smccc_probe_trng(void)
 {
 	struct arm_smccc_res res;
 
modified   drivers/char/hw_random/arm_smccc_trng.c
@@ -19,6 +19,8 @@
 #include <linux/arm-smccc.h>
 #include <linux/arm-smccc-bus.h>
 
+#include <asm/archrandom.h>
+
 #ifdef CONFIG_ARM64
 #define ARM_SMCCC_TRNG_RND	ARM_SMCCC_TRNG_RND64
 #define MAX_BITS_PER_CALL	(3 * 64UL)
@@ -98,6 +100,10 @@ static int smccc_trng_probe(struct arm_smccc_device *sdev)
 {
 	struct hwrng *trng;
 
+	/* validate the minimum version requirement */
+	if (!smccc_probe_trng())
+		return -ENODEV;
+
 	trng = devm_kzalloc(&sdev->dev, sizeof(*trng), GFP_KERNEL);
 	if (!trng)
 		return -ENOMEM;

We can also move arch/arm64/include/asm/rsi_smc.h to
include/linux/arm-rsi-smccc.h. There was a suggestion to move these
firmware interfaces out of architecture-specific code:

https://lore.kernel.org/all/agsNO9cc7H-b0H8L@willie-the-truck

This will also avoid the #ifdef CONFIG_ARM64

-aneesh