Re: [PATCH 12/16] arm64: drtm: Add macro definitions for DEN0113

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Sep 24, 2026 at 05:29:52PM -0700, Jonathan Cameron wrote:
> This one at least does it via base and sum.  But that then separates it from
> the spec?   That is sort of the case anyway because the spec has the full
> number.  0xC400_0110 so I guess not too bad.

I wasn't sure what to do with this since the spec had the full hex and
we have these helper macros.. I made it look more like something else
with the offset style instead of the function call style:

> > +#define ARM_DRTM_SMC_VERSION			(ARM_DRTM_SMC_FN_BASE + 0x00)

IDK, maybe ARM_DRTM_SMC_FN_BASE(0x110) is more connected to the spec,
but I'm used to doing it like this from ioctl land which is very
similar.


> > +#define ARM_DRTM_FEATURE_SELECTOR	BIT_U64(63)
> > +#define ARM_DRTM_FEATURE_TPM		0x01
> > +#define ARM_DRTM_FEATURE_MIN_MEMORY	0x02
> > +#define ARM_DRTM_FEATURE_DMA_PROTECTION	0x03
> > +#define ARM_DRTM_FEATURE_BOOT_PE	0x04
> > +#define ARM_DRTM_FEATURE_TCB_HASH	0x05
> > +#define ARM_DRTM_FEATURE_IMAGE_AUTH	0x06
>
> Nice to have a mask for the 8 bits of the feature field.

OK

> Also nice to keep order the same as the SMC defines which would put this after
> version.  That also puts it next to the values returned for each feature.

I reorganized everything

> > +#define ARM_DRTM_TPM_ALG_MASK		GENMASK_U64(15, 0)
> > +#define ARM_DRTM_TPM_HASHING		BIT_U64(32)
> > +#define ARM_DRTM_PCR_SCHEMA_MASK	GENMASK_U64(36, 33)
> > +#define ARM_DRTM_PCR_SCHEMA_DEFAULT	BIT_U64(33)
> > +#define ARM_DRTM_PCR_SCHEMA_AUTHORITIES	BIT_U64(34)
> 
> Ah.  Always love a field made up of a bunch of bits. Hard to define
> cleanly.   Do you actually need the PCR_SCHEMA_MASK?  

I did it with a field content which gets reused in the launch struct:

/*
 * PCR schema numbers. ARM_DRTM_PCR_SCHEMA_MASK is a bitmap indexed by these,
 * and the Launch Features ARM_DRTM_LAUNCH_PCR_SCHEMA_MASK field holds one.
 */
#define ARM_DRTM_PCR_SCHEMA_DEFAULT		0
#define ARM_DRTM_PCR_SCHEMA_AUTHORITIES		1

/*
 * DMA protection types. ARM_DRTM_DMA_PROTECTION_MASK is a bitmap indexed by
 * these, and the Launch Features ARM_DRTM_LAUNCH_DMA_PROTECTION_MASK field
 * holds one.
 */
#define ARM_DRTM_DMA_PROTECTION_COMPLETE	0
#define ARM_DRTM_DMA_PROTECTION_REGION		1

Have to use BIT() to get the bitmap encoding.

> Same here.  I vaguely wonder if it is worth adding something
> reflecting the relevant feature ID to each of these defines
> so we know what the are referring to?

I added comments and spec references for everything

> > +#define ARM_DRTM_LAUNCH_HASH_FIRMWARE	0
> > +#define ARM_DRTM_LAUNCH_HASH_TPM	BIT_U32(0)
> 
> I'd rather see them as fields and field value pairs but
> can see that is going to get a bit verbose.

I think that would be too verbose in this case, and the usage is very
limited.


> > +static inline s64 arm_drtm_features(u64 function_or_feature, u64 *value)
> > +{
> > +	struct arm_smccc_res res;
> > +	s64 status;
> > +
> > +	arm_smccc_1_1_smc(ARM_DRTM_SMC_FEATURES, function_or_feature,
> > +			  0, 0, 0, 0, 0, 0, &res);
> > +	status = res.a0;
> > +	if (status >= 0 && value)
> > +		*value = res.a1;
> If status == 0, is res.a1 useful?
> You have a helpful comment at the call site in the final patch but none
> the less I have read the spec section a couple of times and have no idea.

I read the spec as 0 is not allowed.

I changed this around to have two wrapper functions, which works a bit
better.

static inline s64 arm_drtm_query_function(u64 function_id)
{
	struct arm_smccc_res res;

	arm_smccc_1_1_smc(ARM_DRTM_SMC_FEATURES, function_id, 0, 0, 0, 0, 0, 0,
			  &res);
	return res.a0;
}

/*
 * v1.4b Section 3.3.1 says:
 *   A return value of greater than 0 means the feature ID is
 *   implemented, and there are other feature-specific
 *   feature or capability bits available in other return value
 *   registers.
 * And for FEATURE_SELECTOR all defined features return data in other
 * registers, so 0 is not an allowed return.
 */
static inline s64 arm_drtm_query_feature(u64 feature, u64 *value)
{
	struct arm_smccc_res res;

	arm_smccc_1_1_smc(ARM_DRTM_SMC_FEATURES,
			  ARM_DRTM_FEATURE_SELECTOR |
				  FIELD_PREP(ARM_DRTM_FEATURE_ID_MASK, feature),
			  0, 0, 0, 0, 0, 0, &res);
	*value = res.a1;
	return res.a0;
}

Thanks,
Jason

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv