Re: [PATCH v3] virt: arm-cca-guest: use migrate_disable() for attestation token requests

[Kohei Enju <enju.kohei@fujitsu.com>]

Hi all,

Just a gentle ping on this v3 patch.

This v3 switches the implementation to use migrate_disable() and calls
the RSI attestation token operations directly, following the feedback
on the previous versions.

Any comments would be appreciated.

Thanks,
Kohei

On 06/12 20:09, Kohei Enju wrote:
> The RSI attestation token init and continue calls must be issued from
> the same CPU. arm_cca_report_new() currently snapshots the CPU number
> and uses smp_call_function_single() to issue those calls on that CPU.
> 
> With CONFIG_DEBUG_PREEMPT=y, the smp_processor_id() call used for the
> snapshot triggers a debug splat [0] because it runs in preemptible
> context. The snapshot does not pin the task to that CPU; it is only used
> to choose the target CPU for smp_call_function_single(), which can fail
> if that CPU is no longer available.
> 
> Use migrate_disable() and issue the token init and continue operations
> directly, without the smp_call_function_single() callbacks. This keeps
> the token request sequence on the same CPU while preserving a sleepable
> context for the GFP_KERNEL allocations needed after the init call.
> 
> [0]
>  BUG: using smp_processor_id() in preemptible [00000000] code: cca-workload-at/264
>  caller is debug_smp_processor_id+0x20/0x30
>  CPU: 0 UID: 0 PID: 264 Comm: cca-workload-at Not tainted 7.1.0-rc1-00044-g55542ab273f2 #80 PREEMPT(lazy)
>  Hardware name: linux,dummy-virt (DT)
>  Call trace:
>   [...]
>   check_preemption_disabled+0xd8/0xf8
>   debug_smp_processor_id+0x20/0x30
>   arm_cca_report_new+0x48/0x278
>   tsm_report_read+0x154/0x1f8
>   tsm_report_outblob_read+0x20/0x38
>   configfs_bin_read_iter+0x118/0x208
>   vfs_read+0x220/0x318
>   [...]
> 
> Fixes: 7999edc484ca ("virt: arm-cca-guest: TSM_REPORT support for realms")
> Signed-off-by: Kohei Enju <enju.kohei@fujitsu.com>
> ---
> Changes:
> v3:
>   - Switch to migrate_disable() and call RSI directly, removing
>     smp_call_function_single() (Will, Suzuki)
>   - Remove arm_cca_attestation_init() helper and unused fields from
>     arm_cca_token_info
>   - Drop Reviewed-by tags since the approach changed from v2
> v2: https://lore.kernel.org/linux-arm-kernel/20260519101217.155740-1-enju.kohei@fujitsu.com/
>   - Add comment about why this path doesn't use migrate_disable()
> v1: https://lore.kernel.org/linux-arm-kernel/20260518033157.1865498-1-enju.kohei@fujitsu.com/
> ---
>  .../virt/coco/arm-cca-guest/arm-cca-guest.c   | 97 +++++++------------
>  1 file changed, 36 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c b/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
> index 66d00b6ceb78..a38df08da6fa 100644
> --- a/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
> +++ b/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
> @@ -16,54 +16,38 @@
>  
>  /**
>   * struct arm_cca_token_info - a descriptor for the token buffer.
> - * @challenge:		Pointer to the challenge data
> - * @challenge_size:	Size of the challenge data
>   * @granule:		PA of the granule to which the token will be written
>   * @offset:		Offset within granule to start of buffer in bytes
> - * @result:		result of rsi_attestation_token_continue operation
>   */
>  struct arm_cca_token_info {
> -	void           *challenge;
> -	unsigned long   challenge_size;
>  	phys_addr_t     granule;
>  	unsigned long   offset;
> -	unsigned long   result;
>  };
>  
> -static void arm_cca_attestation_init(void *param)
> -{
> -	struct arm_cca_token_info *info;
> -
> -	info = (struct arm_cca_token_info *)param;
> -
> -	info->result = rsi_attestation_token_init(info->challenge,
> -						  info->challenge_size);
> -}
> -
>  /**
>   * arm_cca_attestation_continue - Retrieve the attestation token data.
>   *
> - * @param: pointer to the arm_cca_token_info
> + * @info: pointer to the arm_cca_token_info
>   *
>   * Attestation token generation is a long running operation and therefore
>   * the token data may not be retrieved in a single call. Moreover, the
>   * token retrieval operation must be requested on the same CPU on which the
>   * attestation token generation was initialised.
> - * This helper function is therefore scheduled on the same CPU multiple
> + * This helper function must therefore be executed on the same CPU multiple
>   * times until the entire token data is retrieved.
>   */
> -static void arm_cca_attestation_continue(void *param)
> +static unsigned long
> +arm_cca_attestation_continue(struct arm_cca_token_info *info)
>  {
> +	unsigned long ret;
>  	unsigned long len;
>  	unsigned long size;
> -	struct arm_cca_token_info *info;
> -
> -	info = (struct arm_cca_token_info *)param;
>  
>  	size = RSI_GRANULE_SIZE - info->offset;
> -	info->result = rsi_attestation_token_continue(info->granule,
> -						      info->offset, size, &len);
> +	ret = rsi_attestation_token_continue(info->granule, info->offset, size,
> +					     &len);
>  	info->offset += len;
> +	return ret;
>  }
>  
>  /**
> @@ -74,8 +58,8 @@ static void arm_cca_attestation_continue(void *param)
>   *
>   * Initialise the attestation token generation using the challenge data
>   * passed in the TSM descriptor. Allocate memory for the attestation token
> - * and schedule calls to retrieve the attestation token on the same CPU
> - * on which the attestation token generation was initialised.
> + * and retrieve the attestation token on the same CPU on which the
> + * attestation token generation was initialised.
>   *
>   * The challenge data must be at least 32 bytes and no more than 64 bytes. If
>   * less than 64 bytes are provided it will be zero padded to 64 bytes.
> @@ -85,12 +69,11 @@ static void arm_cca_attestation_continue(void *param)
>   * * %-EINVAL  - A parameter was not valid.
>   * * %-ENOMEM  - Out of memory.
>   * * %-EFAULT  - Failed to get IPA for memory page(s).
> - * * A negative status code as returned by smp_call_function_single().
>   */
>  static int arm_cca_report_new(struct tsm_report *report, void *data)
>  {
> -	int ret;
> -	int cpu;
> +	int ret = 0;
> +	unsigned long rsi_result;
>  	long max_size;
>  	unsigned long token_size = 0;
>  	struct arm_cca_token_info info;
> @@ -103,37 +86,33 @@ static int arm_cca_report_new(struct tsm_report *report, void *data)
>  
>  	/*
>  	 * The attestation token 'init' and 'continue' calls must be
> -	 * performed on the same CPU. smp_call_function_single() is used
> -	 * instead of simply calling get_cpu() because of the need to
> -	 * allocate outblob based on the returned value from the 'init'
> -	 * call and that cannot be done in an atomic context.
> +	 * performed on the same CPU, so disable CPU migration around
> +	 * those operations.
>  	 */
> -	cpu = smp_processor_id();
> +	migrate_disable();
>  
> -	info.challenge = desc->inblob;
> -	info.challenge_size = desc->inblob_len;
> -
> -	ret = smp_call_function_single(cpu, arm_cca_attestation_init,
> -				       &info, true);
> -	if (ret)
> -		return ret;
> -	max_size = info.result;
> -
> -	if (max_size <= 0)
> -		return -EINVAL;
> +	max_size = rsi_attestation_token_init(desc->inblob, desc->inblob_len);
> +	if (max_size <= 0) {
> +		ret = -EINVAL;
> +		goto exit_migrate_enable;
> +	}
>  
>  	/* Allocate outblob */
>  	token = kvzalloc(max_size, GFP_KERNEL);
> -	if (!token)
> -		return -ENOMEM;
> +	if (!token) {
> +		ret = -ENOMEM;
> +		goto exit_migrate_enable;
> +	}
>  
>  	/*
>  	 * Since the outblob may not be physically contiguous, use a page
>  	 * to bounce the buffer from RMM.
>  	 */
>  	buf = alloc_pages_exact(RSI_GRANULE_SIZE, GFP_KERNEL);
> -	if (!buf)
> -		return -ENOMEM;
> +	if (!buf) {
> +		ret = -ENOMEM;
> +		goto exit_migrate_enable;
> +	}
>  
>  	/* Get the PA of the memory page(s) that were allocated */
>  	info.granule = (unsigned long)virt_to_phys(buf);
> @@ -144,21 +123,15 @@ static int arm_cca_report_new(struct tsm_report *report, void *data)
>  		info.offset = 0;
>  		do {
>  			/*
> -			 * Schedule a call to retrieve a sub-granule chunk
> -			 * of data per loop iteration.
> +			 * Retrieve a sub-granule chunk of data per loop
> +			 * iteration.
>  			 */
> -			ret = smp_call_function_single(cpu,
> -						       arm_cca_attestation_continue,
> -						       (void *)&info, true);
> -			if (ret != 0) {
> -				token_size = 0;
> -				goto exit_free_granule_page;
> -			}
> -		} while (info.result == RSI_INCOMPLETE &&
> +			rsi_result = arm_cca_attestation_continue(&info);
> +		} while (rsi_result == RSI_INCOMPLETE &&
>  			 info.offset < RSI_GRANULE_SIZE);
>  
>  		/* Break out in case of failure */
> -		if (info.result != RSI_SUCCESS && info.result != RSI_INCOMPLETE) {
> +		if (rsi_result != RSI_SUCCESS && rsi_result != RSI_INCOMPLETE) {
>  			ret = -ENXIO;
>  			token_size = 0;
>  			goto exit_free_granule_page;
> @@ -173,12 +146,14 @@ static int arm_cca_report_new(struct tsm_report *report, void *data)
>  			break;
>  		memcpy(&token[token_size], buf, info.offset);
>  		token_size += info.offset;
> -	} while (info.result == RSI_INCOMPLETE);
> +	} while (rsi_result == RSI_INCOMPLETE);
>  
>  	report->outblob = no_free_ptr(token);
>  exit_free_granule_page:
>  	report->outblob_len = token_size;
>  	free_pages_exact(buf, RSI_GRANULE_SIZE);
> +exit_migrate_enable:
> +	migrate_enable();
>  	return ret;
>  }
>  
> -- 
> 2.43.0
> 
>