Re: [PATCH v2 02/11] coco: guest: arm64: Add Realm Host Interface and guest DA helper

[Jonathan Cameron]

On Mon, 17 Nov 2025 19:29:58 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@xxxxxxxxxx> wrote:

> - describe the Realm Host Interface SMC IDs and result codes in a new
>   `asm/rhi.h` header
> - expose `struct rsi_host_call` plus an `rsi_host_call()` helper so we can
>   invoke `SMC_RSI_HOST_CALL` from C code
> - build a guest-side `rhi-da` helper that drives the vdev TDI state machine
>   via RHI host calls and translates the firmware status codes
> 
> This provides the basic RHI plumbing that later DA features rely on.
> 
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@xxxxxxxxxx>
Hi Aneesh, minor comments follow.

> diff --git a/drivers/virt/coco/arm-cca-guest/rhi-da.c b/drivers/virt/coco/arm-cca-guest/rhi-da.c
> new file mode 100644
> index 000000000000..3430d8df4424
> --- /dev/null
> +++ b/drivers/virt/coco/arm-cca-guest/rhi-da.c
...

> +
> +bool rhi_has_da_support(void)
> +{
> +	int ret;
> +	struct rsi_host_call *rhicall;
> +
> +	rhicall = kmalloc(sizeof(struct rsi_host_call), GFP_KERNEL);

Doesn't look to be passed out anywhere, so not obvious why the lifetime
of this extends beyond this function.  Maybe I'm missing something.

> +	if (!rhicall)
> +		return -ENOMEM;
> +
> +	rhicall->imm = 0;
> +	rhicall->gprs[0] = RHI_DA_FEATURES;
> +
> +	ret = rsi_host_call(virt_to_phys(rhicall));
> +	if (ret != RSI_SUCCESS || rhicall->gprs[0] == SMCCC_RET_NOT_SUPPORTED)
> +		return false;
> +
> +	/* For base DA to work we need these to be supported */
> +	if ((rhicall->gprs[0] & RHI_DA_BASE_FEATURE) == RHI_DA_BASE_FEATURE)
> +		return true;
> +
> +	return false;
> +}
> +
> +static inline int rhi_vdev_continue(unsigned long vdev_id, unsigned long cookie)
> +{
> +	unsigned long ret;
> +
> +	struct rsi_host_call *rhi_call __free(kfree) =
> +		kmalloc(sizeof(struct rsi_host_call), GFP_KERNEL);

sizeof(*rhi_call)  Same for all other cases of this.

> +	if (!rhi_call)
> +		return -ENOMEM;
> +
> +	rhi_call->imm = 0;
> +	rhi_call->gprs[0] = RHI_DA_VDEV_CONTINUE;
> +	rhi_call->gprs[1] = vdev_id;
> +	rhi_call->gprs[2] = cookie;
> +
> +	ret = rsi_host_call(virt_to_phys(rhi_call));
> +	if (ret != RSI_SUCCESS)
> +		return -EIO;
> +
> +	return map_rhi_da_error(rhi_call->gprs[0]);
> +}
> +
> +static int __rhi_vdev_abort(unsigned long vdev_id, unsigned long *da_error)
> +{
> +	unsigned long ret;
> +	struct rsi_host_call *rhi_call __free(kfree) =
> +		kmalloc(sizeof(struct rsi_host_call), GFP_KERNEL);

sizeof(*rhi_call) probably preferred.

> +	if (!rhi_call)
> +		return -ENOMEM;
> +
> +	rhi_call->imm = 0;
> +	rhi_call->gprs[0] = RHI_DA_VDEV_ABORT;
> +	rhi_call->gprs[1] = vdev_id;
> +
> +	ret = rsi_host_call(virt_to_phys(rhi_call));
> +	if (ret != RSI_SUCCESS)
> +		return -EIO;
> +
> +	return *da_error = rhi_call->gprs[0];
> +	return 0;

?  Run builds after each patch and you may catch stuff like this.

> +}
> +
> +static bool should_abort_rhi_call_loop(unsigned long vdev_id)
> +{
> +	int ret;
> +
> +	cond_resched();
> +	if (signal_pending(current)) {
> +		unsigned long da_error;
> +
> +		ret = __rhi_vdev_abort(vdev_id, &da_error);
> +		/* consider all kind of error as not aborted */
> +		if (!ret && (da_error == RHI_DA_SUCCESS))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static int __rhi_vdev_set_tdi_state(unsigned long vdev_id,
> +				    unsigned long target_state,

Maybe use an enum for target state? Can name it to align with the
RHIDAVDevTDIState used as the type for this in the RHI spec.


> +				    unsigned long *cookie)
> +{
> +	unsigned long ret;
> +
> +	struct rsi_host_call *rhi_call __free(kfree) =
> +		kmalloc(sizeof(struct rsi_host_call), GFP_KERNEL);
> +	if (!rhi_call)
> +		return -ENOMEM;
> +
> +	rhi_call->imm = 0;
> +	rhi_call->gprs[0] = RHI_DA_VDEV_SET_TDI_STATE;
> +	rhi_call->gprs[1] = vdev_id;
> +	rhi_call->gprs[2] = target_state;
> +
> +	ret = rsi_host_call(virt_to_phys(rhi_call));
> +	if (ret != RSI_SUCCESS)
> +		return -EIO;
> +
> +	*cookie = rhi_call->gprs[1];
> +	return map_rhi_da_error(rhi_call->gprs[0]);
> +}

> diff --git a/drivers/virt/coco/arm-cca-guest/rhi-da.h b/drivers/virt/coco/arm-cca-guest/rhi-da.h
> new file mode 100644
> index 000000000000..8dd77c7ed645
> --- /dev/null
> +++ b/drivers/virt/coco/arm-cca-guest/rhi-da.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 ARM Ltd.

Possibly update if this has changed much this year.

> + */
> +
> +#ifndef _VIRT_COCO_RHI_DA_H_
> +#define _VIRT_COCO_RHI_DA_H_
> +
> +#include <asm/rhi.h>
> +
> +struct pci_dev;
> +bool rhi_has_da_support(void);
> +int rhi_vdev_set_tdi_state(struct pci_dev *pdev, unsigned long target_state);
> +#endif