Re: [PATCH v2 1/2] firmware: stratix10-svc: add FCS crypto-service commands for Agilex 5

[Hang Suan Wang]

Response based on Sashiko review:
https://sashiko.dev/#/patchset/cover.1783966717.git.hang.suan.wang%40altera.com


On 14/7/2026 2:35 am, hang.suan.wang@xxxxxxxxxx wrote:
> From: Hang Suan Wang <hang.suan.wang@xxxxxxxxxx>
> 
> The Agilex 5 Secure Device Manager (SDM 1.5) exposes an FPGA Crypto
> Service (FCS) over the existing SIP SMC mailbox: a session-based
> interface for crypto primitives such as SDOS (Secure Data Object
> Service) encrypt/decrypt. The service layer has no command to drive it
> yet.
> 
> Teach stratix10-svc about this interface so an in-kernel FCS client can
> use it:
> 
>   - add the client command codes COMMAND_FCS_CRYPTO_OPEN_SESSION,
>     COMMAND_FCS_CRYPTO_CLOSE_SESSION and COMMAND_FCS_SDOS_DATA_EXT (all
>     asynchronous), and grow stratix10_svc_client_msg::arg[] from three
>     to four entries so the SDOS command can carry its session, context,
>     mode and owner arguments;
> 
>   - add the matching asynchronous SIP SMC function IDs
>     (INTEL_SIP_SMC_ASYNC_FCS_OPEN_CS_SESSION,
>     INTEL_SIP_SMC_ASYNC_FCS_CLOSE_CS_SESSION and
>     INTEL_SIP_SMC_ASYNC_FCS_CRYPTION_EXT) with their register-usage
>     documentation;
> 
>   - match "intel,agilex5-svc" and register a "stratix10-fcs" child
>     platform device, mirroring the existing RSU child, so an FCS client
>     driver can bind without a dedicated device-tree node;
> 
>   - dispatch the new commands in the asynchronous send and response
>     paths; for the SDOS data command, translate the source and
>     destination buffers (allocated from the service-layer gen_pool) to
>     physical addresses and pass them, together with the session/context
>     IDs and owner ID, to the SDM.
> 
> The transport is unchanged: Agilex 5 reuses the SIP SMC calling
> convention and async mailbox ABI the driver already implements, so no
> new transport mechanism is required.
> 
> The SDOS SMMU-remapped address slots currently carry the buffer
> physical addresses; SMMU remapping support is added in a follow-up
> series.
> 
> This is a prerequisite for the SoCFPGA FCS driver, the first in-tree
> consumer of these commands.
> 
> Signed-off-by: Hang Suan Wang <hang.suan.wang@xxxxxxxxxx>
> Reviewed-by: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> ---
>  drivers/firmware/stratix10-svc.c              | 58 +++++++++++++++--
>  include/linux/firmware/intel/stratix10-smc.h  | 64 +++++++++++++++++++
>  .../firmware/intel/stratix10-svc-client.h     | 18 +++++-
>  3 files changed, 135 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index c24ca5823078..09f709a2f28e 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
> @@ -45,6 +45,7 @@
>  
>  /* stratix10 service layer clients */
>  #define STRATIX10_RSU				"stratix10-rsu"
> +#define STRATIX10_FCS				"stratix10-fcs"
>  
>  /* Maximum number of SDM client IDs. */
>  #define MAX_SDM_CLIENT_IDS			16
> @@ -104,9 +105,11 @@ struct stratix10_svc_chan;
>  /**
>   * struct stratix10_svc - svc private data
>   * @stratix10_svc_rsu: pointer to stratix10 RSU device
> + * @stratix10_svc_fcs: pointer to stratix10 FCS device
>   */
>  struct stratix10_svc {
>  	struct platform_device *stratix10_svc_rsu;
> +	struct platform_device *stratix10_svc_fcs;
>  };
>  
>  /**
> @@ -1319,6 +1322,30 @@ int stratix10_svc_async_send(struct stratix10_svc_chan *chan, void *msg,
>  		STRATIX10_SIP_SMC_SET_TRANSACTIONID_X1(handle->transaction_id);
>  
>  	switch (p_msg->command) {
> +	case COMMAND_FCS_CRYPTO_OPEN_SESSION:
> +		args.a0 = INTEL_SIP_SMC_ASYNC_FCS_OPEN_CS_SESSION;
> +		break;
> +	case COMMAND_FCS_CRYPTO_CLOSE_SESSION:
> +		args.a0 = INTEL_SIP_SMC_ASYNC_FCS_CLOSE_CS_SESSION;
> +		args.a2 = p_msg->arg[0];
> +		break;
> +	case COMMAND_FCS_SDOS_DATA_EXT:
> +		args.a0 = INTEL_SIP_SMC_ASYNC_FCS_CRYPTION_EXT;
> +		args.a2 = p_msg->arg[0];
> +		args.a3 = p_msg->arg[1];
> +		args.a4 = p_msg->arg[2];
> +		/* payloads are allocated from the svc gen_pool; pass phys addr */
> +		args.a5 = gen_pool_virt_to_phys(ctrl->genpool,
> +						(unsigned long)p_msg->payload);

> If a client driver passes a NULL or invalid buffer (for example, if a
> decryption operation only requires an output buffer or an encryption
> operation has no payload yet), gen_pool_virt_to_phys() will return -1.
> Since this is cast to unsigned long and passed directly into the SMC
> arguments args.a5, args.a7, args.a10, and args.a11, could this cause
> secure-world crashes or undefined behavior in the firmware?
> Should the driver explicitly check for valid buffer addresses or sizes
> and handle absent buffers gracefully by passing 0?
> This isn't a bug introduced by this patch, but does stratix10_svc_async_send()
> need a memory barrier before invoking the SMC call?
> The memory pool used for SMC payloads is mapped as Write-Combine (MEMREMAP_WC).
> On ARM64, writes to WC memory are placed in write-combine buffers.
> Since context synchronization events like an SMC instruction do not guarantee
> the visibility of memory accesses to other observers, is it possible for the
> EL3 firmware to read stale or partial data from the payload buffer?
> Should there be a dma_wmb() or wmb() issued before actrl->invoke_fn() is
> called later in this function?

The buffer-validity concern does not actually apply because by the time
stratix10_svc_async_send() runs, the payload is already fully populated
and both pointers are guaranteed valid. The missing-barrier gap is 
confirmed pre-existing, I think I shall not introduced in this patch.


> +		args.a6 = p_msg->payload_length;
> +		args.a7 = gen_pool_virt_to_phys(ctrl->genpool,
> +						(unsigned long)p_msg->payload_output);
> +		args.a8 = p_msg->payload_length_output;
> +		args.a9 = p_msg->arg[3];
> +		/* SMMU remapping is added later; pass phys addr for now */
> +		args.a10 = args.a5;
> +		args.a11 = args.a7;
> +		break;
>  	case COMMAND_RSU_GET_SPT_TABLE:
>  		args.a0 = INTEL_SIP_SMC_ASYNC_RSU_GET_SPT;
>  		break;
> @@ -1408,8 +1435,13 @@ static int stratix10_svc_async_prepare_response(struct stratix10_svc_chan *chan,
>  	data->status = STRATIX10_GET_SDM_STATUS_CODE(handle->res.a1);
>  
>  	switch (p_msg->command) {
> +	case COMMAND_FCS_CRYPTO_CLOSE_SESSION:
>  	case COMMAND_RSU_NOTIFY:
>  		break;
> +	case COMMAND_FCS_CRYPTO_OPEN_SESSION:
> +	case COMMAND_FCS_SDOS_DATA_EXT:
> +		data->kaddr1 = (void *)&handle->res.a2;
> +		break;
>  	case COMMAND_RSU_GET_SPT_TABLE:
>  		data->kaddr1 = (void *)&handle->res.a2;
>  		data->kaddr2 = (void *)&handle->res.a3;
> @@ -1908,6 +1940,7 @@ EXPORT_SYMBOL_GPL(stratix10_svc_free_memory);
>  static const struct of_device_id stratix10_svc_drv_match[] = {
>  	{.compatible = "intel,stratix10-svc"},
>  	{.compatible = "intel,agilex-svc"},
> +	{.compatible = "intel,agilex5-svc"},
>  	{},
>  };
>  
> @@ -2011,20 +2044,36 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
>  
>  	ret = platform_device_add(svc->stratix10_svc_rsu);
>  	if (ret)
> -		goto err_put_device;
> +		goto err_put_rsu;
> +
> +	svc->stratix10_svc_fcs = platform_device_alloc(STRATIX10_FCS, 0);
> +	if (!svc->stratix10_svc_fcs) {
> +		dev_err(dev, "failed to allocate %s device\n", STRATIX10_FCS);
> +		ret = -ENOMEM;
> +		goto err_unregister_rsu;
> +	}
> +
> +	ret = platform_device_add(svc->stratix10_svc_fcs);
> +	if (ret)
> +		goto err_put_fcs;
>  
>  	ret = of_platform_default_populate(dev_of_node(dev), NULL, dev);
>  	if (ret)
> -		goto err_unregister_rsu_dev;
> +		goto err_unregister_fcs;
>  
>  	pr_info("Intel Service Layer Driver Initialized\n");
>  
>  	return 0;
>  
> -err_unregister_rsu_dev:
> +err_unregister_fcs:
> +	platform_device_unregister(svc->stratix10_svc_fcs);
> +	goto err_unregister_rsu;
> +err_put_fcs:
> +	platform_device_put(svc->stratix10_svc_fcs);
> +err_unregister_rsu:
>  	platform_device_unregister(svc->stratix10_svc_rsu);
>  	goto err_free_fifos;
> -err_put_device:
> +err_put_rsu:
>  	platform_device_put(svc->stratix10_svc_rsu);
>  err_free_fifos:
>  	/* only remove from list if list_add_tail() was reached */

> This isn't a bug introduced by this patch, but in the probe error path for
> stratix10_svc_drv_probe(), if adding the fcs child device fails, the code
> jumps to err_unregister_rsu.
> This triggers the RSU driver's remove callback, which drops a reference but
> does not stop the service channel's svc_smc_hvc_thread kthread. Execution
> then continues to err_free_fifos, where the parent driver frees all channel
> FIFOs using kfifo_free().
> Could the still-running kthread continue to access the freed FIFO memory via
> kfifo_out_spinlocked() and cause a use-after-free?
> Does the parent probe error path need to explicitly stop any active channel
> tasks before freeing the FIFOs, similar to how it is handled in
> stratix10_svc_drv_remove()?

This is not a real concern, by the time stratix10_rsu_probe() returns,
chan->task is already NULL. When the FCS allocation then fails and we jump
err_unregister_rsu → stratix10_rsu_remove() → stratix10_svc_free_channel() 
and on into err_free_fifos's kfifo_free(),
there's no live kthread left to race with the FIFO teardown.


> @@ -2051,6 +2100,7 @@ static void stratix10_svc_drv_remove(struct platform_device *pdev)
>  	of_platform_depopulate(ctrl->dev);
>  
>  	platform_device_unregister(svc->stratix10_svc_rsu);
> +	platform_device_unregister(svc->stratix10_svc_fcs);
>  
>  	for (i = 0; i < SVC_NUM_CHANNEL; i++) {
>  		if (ctrl->chans[i].task) {

> This isn't a bug introduced by this patch, but looking at
> stratix10_svc_drv_remove(), the parent driver calls stratix10_svc_async_exit()
> before unregistering the child devices via platform_device_unregister().
> stratix10_svc_async_exit() clears the hash table and frees all active
> transaction handlers.
> If a child driver concurrently polls a transaction via stratix10_svc_async_poll()
> during teardown, could it access the freed handle pointer and cause a
> use-after-free?
> Should the parent driver fully unregister child devices (ensuring their remove
> callbacks have finished executing) before tearing down the asynchronous
> infrastructure?

this might be true when timeout expires and becomes NULL.
will exit gracefully after unregister.
will fix in v3.

> diff --git a/include/linux/firmware/intel/stratix10-smc.h b/include/linux/firmware/intel/stratix10-smc.h
> index 9116512169dc..d7ad6bd6f669 100644
> --- a/include/linux/firmware/intel/stratix10-smc.h
> +++ b/include/linux/firmware/intel/stratix10-smc.h
> @@ -640,6 +640,70 @@ INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_COMPLETED_WRITE)
>  #define INTEL_SIP_SMC_FCS_GET_PROVISION_DATA \
>  	INTEL_SIP_SMC_STD_CALL_VAL(INTEL_SIP_SMC_FUNCID_FCS_GET_PROVISION_DATA)
>  
> +/**
> + * Request INTEL_SIP_SMC_ASYNC_FCS_CRYPTION_EXT
> + * Async call to perform encryption/decryption
> + *
> + * Call register usage:
> + * a0 INTEL_SIP_SMC_ASYNC_FCS_CRYPTION_EXT
> + * a1 transaction job id
> + * a2 session ID
> + * a3 context ID
> + * a4 cryption operating mode (1 for encryption and 0 for decryption)
> + * a5 physical address of source
> + * a6 size of source
> + * a7 physical address of destination
> + * a8 size of destination
> + * a9 sdos ownership
> + * a10 smmu remapped address of source
> + * a11 smmu remapped address of destination
> + * a12-a17 not used
> + *
> + * Return status:
> + * a0 INTEL_SIP_SMC_STATUS_OK or INTEL_SIP_SMC_STATUS_ERROR
> + * a1-a17 not used
> + */
> +#define INTEL_SIP_SMC_ASYNC_FUNC_ID_FCS_CRYPTION_EXT (0x12F)
> +#define INTEL_SIP_SMC_ASYNC_FCS_CRYPTION_EXT \
> +	INTEL_SIP_SMC_ASYNC_VAL(INTEL_SIP_SMC_ASYNC_FUNC_ID_FCS_CRYPTION_EXT)
> +
> +/**
> + * Request INTEL_SIP_SMC_ASYNC_FCS_OPEN_CS_SESSION
> + * Async call to open and establish a crypto service session with firmware
> + *
> + * Call register usage:
> + * a0 INTEL_SIP_SMC_FCS_OPEN_CRYPTO_SERVICE_SESSION
> + * a1 transaction job id
> + * a2-a17 not used
> + *
> + * Return status:
> + * a0 INTEL_SIP_SMC_STATUS_OK ,INTEL_SIP_SMC_STATUS_REJECTED
> + * or INTEL_SIP_SMC_STATUS_BUSY
> + * a1-a17 not used
> + */
> +#define INTEL_SIP_SMC_ASYNC_FUNC_ID_FCS_OPEN_CS_SESSION (0x13A)
> +#define INTEL_SIP_SMC_ASYNC_FCS_OPEN_CS_SESSION \
> +	INTEL_SIP_SMC_ASYNC_VAL(INTEL_SIP_SMC_ASYNC_FUNC_ID_FCS_OPEN_CS_SESSION)
> +
> +/**
> + * Request INTEL_SIP_SMC_ASYNC_FCS_CLOSE_CS_SESSION
> + * Async call to close a service session
> + *
> + * Call register usage:
> + * a0 INTEL_SIP_SMC_ASYNC_FCS_CLOSE_CS_SESSION
> + * a1 transaction job id
> + * a2 session ID
> + * a3-a17 not used
> + *
> + * Return status:
> + * a0 INTEL_SIP_SMC_STATUS_OK ,INTEL_SIP_SMC_STATUS_REJECTED
> + * or INTEL_SIP_SMC_STATUS_BUSY
> + * a1-a17 not used
> + */
> +#define INTEL_SIP_SMC_ASYNC_FUNC_ID_FCS_CLOSE_CS_SESSION (0x13B)
> +#define INTEL_SIP_SMC_ASYNC_FCS_CLOSE_CS_SESSION \
> +	INTEL_SIP_SMC_ASYNC_VAL(INTEL_SIP_SMC_ASYNC_FUNC_ID_FCS_CLOSE_CS_SESSION)
> +
>  /**
>   * Request INTEL_SIP_SMC_HWMON_READTEMP
>   * Sync call to request temperature
> diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
> index 3edd93502bf8..285fddafeacb 100644
> --- a/include/linux/firmware/intel/stratix10-svc-client.h
> +++ b/include/linux/firmware/intel/stratix10-svc-client.h
> @@ -7,6 +7,8 @@
>  #ifndef __STRATIX10_SVC_CLIENT_H
>  #define __STRATIX10_SVC_CLIENT_H
>  
> +#include <linux/types.h>
> +
>  /*
>   * Service layer driver supports client names
>   *
> @@ -122,6 +124,15 @@ struct stratix10_svc_chan;
>   * @COMMAND_SMC_SVC_VERSION: Non-mailbox SMC SVC API Version,
>   * return status is SVC_STATUS_OK
>   *
> + * @COMMAND_FCS_CRYPTO_OPEN_SESSION: open the crypto service session(s),
> + * return status is SVC_STATUS_OK or SVC_STATUS_ERROR
> + *
> + * @COMMAND_FCS_CRYPTO_CLOSE_SESSION: close the crypto service session(s),
> + * return status is SVC_STATUS_OK or SVC_STATUS_ERROR
> + *
> + * @COMMAND_FCS_SDOS_DATA_EXT: extend SDOS data encryption & decryption,
> + * return status is SVC_STATUS_OK or SVC_STATUS_ERROR
> + *
>   * @COMMAND_MBOX_SEND_CMD: send generic mailbox command, return status is
>   * SVC_STATUS_OK or SVC_STATUS_ERROR
>   *
> @@ -185,6 +196,11 @@ enum stratix10_svc_command_code {
>  	COMMAND_FCS_RANDOM_NUMBER_GEN,
>  	/* for general status poll */
>  	COMMAND_POLL_SERVICE_STATUS = 40,
> +	/* for crypto service */
> +	COMMAND_FCS_CRYPTO_OPEN_SESSION = 50,
> +	COMMAND_FCS_CRYPTO_CLOSE_SESSION,
> +	/* for extended SDOS encrypt/decrypt */
> +	COMMAND_FCS_SDOS_DATA_EXT = 82,
>  	/* for generic mailbox send command */
>  	COMMAND_MBOX_SEND_CMD = 100,
>  	/* Non-mailbox SMC Call */
> @@ -210,7 +226,7 @@ struct stratix10_svc_client_msg {
>  	void *payload_output;
>  	size_t payload_length_output;
>  	enum stratix10_svc_command_code command;
> -	u64 arg[3];
> +	u64 arg[4];
>  };
>  
>  /**