Re: [PATCH v2 06/11] coco: guest: arm64: Add support for reading cached objects from host

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

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

> On Mon, 17 Nov 2025 19:30:02 +0530
> "Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> wrote:
>
>> Teach rsi_device_start() to pull the interface report and device
>> certificate from the host by querying size, sharing a decrypted buffer
>> for the read, copying the payload to private memory. Also track the
>> fetched blobs in struct cca_guest_dsc so later stages can hand them to
>> the attestation flow.
>> 
>> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
>> ---
>>  arch/arm64/include/asm/rhi.h             |  7 +++
>>  drivers/virt/coco/arm-cca-guest/rhi-da.c | 80 ++++++++++++++++++++++++
>>  drivers/virt/coco/arm-cca-guest/rhi-da.h |  1 +
>>  drivers/virt/coco/arm-cca-guest/rsi-da.h |  8 +++
>>  4 files changed, 96 insertions(+)
>> 
>> diff --git a/arch/arm64/include/asm/rhi.h b/arch/arm64/include/asm/rhi.h
>> index ce2ed8a440c3..738470dfb869 100644
>> --- a/arch/arm64/include/asm/rhi.h
>> +++ b/arch/arm64/include/asm/rhi.h
>> @@ -39,6 +39,13 @@
>>  				 RHI_DA_FEATURE_VDEV_SET_TDI_STATE)
>>  #define RHI_DA_FEATURES			SMC_RHI_CALL(0x004B)
>>  
>> +#define RHI_DA_OBJECT_CERTIFICATE		0x1
>> +#define RHI_DA_OBJECT_MEASUREMENT		0x2
>> +#define RHI_DA_OBJECT_INTERFACE_REPORT		0x3
>> +#define RHI_DA_OBJECT_VCA			0x4
>> +#define RHI_DA_OBJECT_SIZE		SMC_RHI_CALL(0x004C)
>> +#define RHI_DA_OBJECT_READ		SMC_RHI_CALL(0x004D)
>> +
>>  #define RHI_DA_VDEV_CONTINUE		SMC_RHI_CALL(0x0051)
>>  #define RHI_DA_VDEV_GET_INTERFACE_REPORT SMC_RHI_CALL(0x0052)
>>  
>> diff --git a/drivers/virt/coco/arm-cca-guest/rhi-da.c b/drivers/virt/coco/arm-cca-guest/rhi-da.c
>> index aa17bb3ee562..d29aee0fca58 100644
>> --- a/drivers/virt/coco/arm-cca-guest/rhi-da.c
>> +++ b/drivers/virt/coco/arm-cca-guest/rhi-da.c
>> @@ -248,3 +248,83 @@ int rhi_update_vdev_measurements_cache(struct pci_dev *pdev,
>>  
>>  	return ret;
>>  }
>> +
>> +int rhi_read_cached_object(int vdev_id, int da_object_type, void **object, int *object_size)
>> +{
>> +	int ret;
>> +	int max_data_len;
>> +	struct page *shared_pages;
>> +	void *data_buf_shared, *data_buf_private;
>> +	struct rsi_host_call *rhicall;
>> +
>> +	rhicall = kmalloc(sizeof(struct rsi_host_call), GFP_KERNEL);
>> +	if (!rhicall)
>> +		return -ENOMEM;
>> +
>> +	rhicall->imm = 0;
>> +	rhicall->gprs[0] = RHI_DA_OBJECT_SIZE;
>> +	rhicall->gprs[1] = vdev_id;
>> +	rhicall->gprs[2] = da_object_type;
>> +
>> +	ret = rsi_host_call(virt_to_phys(rhicall));
>> +	if (ret != RSI_SUCCESS) {
>> +		ret = -EIO;
>> +		goto err_return;
>> +	}
>> +
>> +	if (rhicall->gprs[0] != RHI_DA_SUCCESS) {
>> +		ret = -EIO;
>> +		goto err_return;
>> +	}
>> +
>> +	/* validate against the max cache object size used on host. */
>> +	max_data_len = rhicall->gprs[1];
>> +	if (max_data_len > MAX_CACHE_OBJ_SIZE || max_data_len == 0) {
>> +		ret = -EIO;
>> +		goto err_return;
>> +	}
>> +	*object_size = max_data_len;
>
> I raise this below, but not sure why this is set way before setting
> *object.  Can set it later and use max_data_len which I think is
> clearer naming anyway.
>
>> +
>> +	data_buf_private = kmalloc(*object_size, GFP_KERNEL);
>> +	if (!data_buf_private) {
>> +		ret = -ENOMEM;
>> +		goto err_return;
>> +	}
>> +
>> +	shared_pages = alloc_shared_pages(NUMA_NO_NODE, GFP_KERNEL, max_data_len);
>> +	if (!shared_pages) {
>> +		ret = -ENOMEM;
>> +		goto err_shared_alloc;
>> +	}
>> +	data_buf_shared = page_address(shared_pages);
>> +
>> +	rhicall->imm = 0;
>> +	rhicall->gprs[0] = RHI_DA_OBJECT_READ;
>> +	rhicall->gprs[1] = vdev_id;
>> +	rhicall->gprs[2] = da_object_type;
>> +	rhicall->gprs[3] = 0; /* offset within the data buffer */
>> +	rhicall->gprs[4] = max_data_len;
>> +	rhicall->gprs[5] = virt_to_phys(data_buf_shared);
>> +	ret = rsi_host_call(virt_to_phys(rhicall));
>> +	if (ret != RSI_SUCCESS || rhicall->gprs[0] != RHI_DA_SUCCESS) {
>> +		ret = -EIO;
>> +		goto err_rhi_call;
>> +	}
>> +
>> +	memcpy(data_buf_private, data_buf_shared, *object_size);
>
> Given data_buf_private() only seems useful if we aren't in an error
> condition, why not move allocation to here and use kmemdup() ?
>
>> +	free_shared_pages(shared_pages, max_data_len);
>> +
>> +	*object = data_buf_private;
>> +	kfree(rhicall);
>> +	return 0;
>> +
>> +err_rhi_call:
>> +	free_shared_pages(shared_pages, max_data_len);
>> +err_shared_alloc:
>> +	kfree(data_buf_private);
>> +err_return:
>> +	*object = NULL;
>
> Is it necessary to zero the passed in variable given this function never
> touched it and is returning an error. If it is, can you do that unconditionally
> at start of function and override only on success?
>
>> +	*object_size = 0;
>
> Likewise for the size - I'm not sure why you set that much earlier
> than *object.
>
> With those two gone, this feels like it would be well suited for
> some __free magic to handle everything here.
> You will need to deal with the free_shared_pages() though which will
> require an extra structure definition and helpers to wrap up what
> is allocated - similar to what tdx_page_array_alloc does (though without
> the bulk aspects of that)
>
> http://lore.kernel.org/all/20251117022311.2443900-7-yilun.xu@linux.intel.com/
>
> Or given the shared page stuff is the inner most aspect anyway you could
> just do a helper function from alloc_shared_pages to free_shared_pages
> calls so that you can call that free_shared_pages unconditionally before
> checking return value.
>
> Note that if you do go with DEFINE_FREE() you 'could' pass in the storage.
> I objected to that elsewhere but there is precedence.  So have a
> struct shared_pages {
> 	struct page *page;
> 	size_t order;
> }
> define one on the stack and pass it in so that you avoid an extra allocation.
> Not a pattern I particularly like though and this isn't expected to be
> a particularly fast path so I'd just dynamically allocate a struct shared_pages
> inside alloc_shared_pages.
>
>
>> +	kfree(rhicall);
>> +	return ret;
>> +}

I'm not sure we need a new struct here, since other shared pages do have
type like struct rhi_vdev_measurement_params. The read-cached object is
the only exception.

That said, I’ve updated the other allocation to use __free() as suggested.

int rhi_read_cached_object(int vdev_id, int da_object_type, void **object, int *object_size)
{
	int ret;
	int max_data_len;
	void *data_buf_shared;
	struct page *shared_pages;

	*object_size = 0;
	*object = NULL;

	struct rsi_host_call *rhicall __free(kfree) =
		kmalloc(sizeof(struct rsi_host_call), GFP_KERNEL);
	if (!rhicall)
		return -ENOMEM;

	rhicall->imm = 0;
	rhicall->gprs[0] = RHI_DA_OBJECT_SIZE;
	rhicall->gprs[1] = vdev_id;
	rhicall->gprs[2] = da_object_type;

	ret = rsi_host_call(virt_to_phys(rhicall));
	if (ret != RSI_SUCCESS)
		return -EIO;

	if (rhicall->gprs[0] != RHI_DA_SUCCESS)
		return -EIO;

	/* validate against the max cache object size used on host. */
	max_data_len = rhicall->gprs[1];
	if (max_data_len > MAX_CACHE_OBJ_SIZE || max_data_len == 0)
		return -EIO;

	shared_pages = alloc_shared_pages(NUMA_NO_NODE, GFP_KERNEL, max_data_len);
	if (!shared_pages)
		return -ENOMEM;

	data_buf_shared = page_address(shared_pages);

	rhicall->imm = 0;
	rhicall->gprs[0] = RHI_DA_OBJECT_READ;
	rhicall->gprs[1] = vdev_id;
	rhicall->gprs[2] = da_object_type;
	rhicall->gprs[3] = 0; /* offset within the data buffer */
	rhicall->gprs[4] = max_data_len;
	rhicall->gprs[5] = virt_to_phys(data_buf_shared);
	ret = rsi_host_call(virt_to_phys(rhicall));
	if (ret != RSI_SUCCESS || rhicall->gprs[0] != RHI_DA_SUCCESS) {
		free_shared_pages(shared_pages, max_data_len);
		return -EIO;
	}

	void *data_buf_private __free(kvfree) =
		kvmemdup(data_buf_shared, max_data_len, GFP_KERNEL);
	/* free the shared pages irrespective of error condition */
	free_shared_pages(shared_pages, max_data_len);
	if (!data_buf_private)
		return -ENOMEM;

	*object = data_buf_private;
	*object_size = max_data_len;
	return 0;
}