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

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

Jonathan Cameron <jonathan.cameron@huawei.com> writes:

> On Mon, 17 Nov 2025 19:29:58 +0530
> "Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> 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@kernel.org>
> 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.
>

This should be similar to other calls

	struct rsi_host_call *rhicall __free(kfree) =
		kmalloc(sizeof(struct rsi_host_call), GFP_KERNEL);

I’ll update the code to reflect that. Thanks for pointing it out.

>
>> +	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.
>

how about enum rhi_tdi_state ?

>
>
>> +				    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

-aneesh