Re: [PATCH v2 05/11] coco: guest: arm64: Add support for updating measurements from device

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Mon, 17 Nov 2025 19:30:01 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> wrote:

> Fetch device measurements using RSI_RDEV_GET_MEASUREMENTS. The fetched
> device measurements will be cached in the host.
> 
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
Hi Aneesh

Minor stuff inline.

thanks

Jonathan

> ---
>  arch/arm64/include/asm/rhi.h             | 19 ++++++++
>  drivers/virt/coco/arm-cca-guest/rhi-da.c | 48 ++++++++++++++++++++
>  drivers/virt/coco/arm-cca-guest/rhi-da.h |  2 +
>  drivers/virt/coco/arm-cca-guest/rsi-da.c | 58 ++++++++++++++++++++++++
>  drivers/virt/coco/arm-cca-guest/rsi-da.h |  2 +
>  5 files changed, 129 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/rhi.h b/arch/arm64/include/asm/rhi.h
> index 5f140015afc3..ce2ed8a440c3 100644
> --- a/arch/arm64/include/asm/rhi.h
> +++ b/arch/arm64/include/asm/rhi.h
> @@ -42,6 +42,25 @@
>  #define RHI_DA_VDEV_CONTINUE		SMC_RHI_CALL(0x0051)
>  #define RHI_DA_VDEV_GET_INTERFACE_REPORT SMC_RHI_CALL(0x0052)
>  
> +#define RHI_VDEV_MEASURE_SIGNED		BIT(0)
> +#define RHI_VDEV_MEASURE_RAW		BIT(1)
> +#define RHI_VDEV_MEASURE_EXCHANGE	BIT(2)
Whilst I appreciate the specs are still subject to minor changes, 
it would be very helpful if definitions like the ones above were
all accompanied by a spec reference.

Which is another way of saying I can't find these ones ;)

> +struct rhi_vdev_measurement_params {
> +	union {
> +		u64 flags;
> +		u8 padding0[256];
> +	};
> +	union {
> +		u8 indices[32];
> +		u8 padding1[256];
> +	};
> +	union {
> +		u8 nonce[32];
> +		u8 padding2[256];
> +	};
> +};
> +#define RHI_DA_VDEV_GET_MEASUREMENTS	SMC_RHI_CALL(0x0053)
> +
>  #define RHI_DA_TDI_CONFIG_UNLOCKED		0x0
>  #define RHI_DA_TDI_CONFIG_LOCKED		0x1
>  #define RHI_DA_TDI_CONFIG_RUN			0x2
> diff --git a/drivers/virt/coco/arm-cca-guest/rhi-da.c b/drivers/virt/coco/arm-cca-guest/rhi-da.c
> index f4fb8577e1b5..aa17bb3ee562 100644
> --- a/drivers/virt/coco/arm-cca-guest/rhi-da.c
> +++ b/drivers/virt/coco/arm-cca-guest/rhi-da.c
> @@ -200,3 +200,51 @@ int rhi_update_vdev_interface_report_cache(struct pci_dev *pdev)
>  
>  	return ret;
>  }
> +
> +static inline int rhi_vdev_get_measurements(unsigned long vdev_id,
> +					    phys_addr_t vdev_meas_phys,
> +					    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) slightly preferred.

> +	if (!rhi_call)
> +		return -ENOMEM;
> +
> +	rhi_call->imm = 0;
> +	rhi_call->gprs[0] = RHI_DA_VDEV_GET_MEASUREMENTS;
> +	rhi_call->gprs[1] = vdev_id;
> +	rhi_call->gprs[2] = vdev_meas_phys;
> +
> +	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/rsi-da.c b/drivers/virt/coco/arm-cca-guest/rsi-da.c
> index c8ba72e4be3e..aa6e13e4c0ea 100644
> --- a/drivers/virt/coco/arm-cca-guest/rsi-da.c
> +++ b/drivers/virt/coco/arm-cca-guest/rsi-da.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <linux/pci.h>
> +#include <linux/mem_encrypt.h>
>  #include <asm/rsi_cmds.h>
>  
>  #include "rsi-da.h"
> @@ -35,9 +36,50 @@ int cca_device_unlock(struct pci_dev *pdev)
>  	return 0;
>  }
>  
> +struct page *alloc_shared_pages(int nid, gfp_t gfp_mask, unsigned long min_size)
> +{
> +	int ret;
> +	struct page *page;
> +	/* We should normalize the size based on hypervisor page size */
> +	int page_order = get_order(min_size);
> +
> +	/* Always request for zero filled pages */

Not sure the comment is necessary given the visible flag.
If you were saying 'why' then a comment would be fine, but this is just
repeating what we can see in the code.

> +	page = alloc_pages_node(nid, gfp_mask | __GFP_ZERO, page_order);
> +
> +	if (!page)
> +		return NULL;
> +
> +	ret = set_memory_decrypted((unsigned long)page_address(page),
> +				   1 << page_order);
> +	/*
> +	 * If set_memory_decrypted() fails then we don't know what state the
> +	 * page is in, so we can't free it. Instead we leak it.
> +	 * set_memory_decrypted() will already have WARNed.
> +	 */
> +	if (ret)
> +		return NULL;
> +
> +	return page;
> +}
> +
> +int free_shared_pages(struct page *page, unsigned long min_size)
> +{
> +	int ret;
> +	/* We should normalize the size based on hypervisor page size */
> +	int page_order = get_order(min_size);
> +
> +	ret = set_memory_encrypted((unsigned long)page_address(page), 1 << page_order);
> +	/* If we fail to mark it encrypted don't free it back */
> +	if (!ret)
> +		__free_pages(page, page_order);
> +	return ret;
If failing to mark it encrypted is an error I'd find it easier to read this if it were
out of line.

	ret = set_memory...
	if (ret)
		return ret;

	__free_pages();

	return 0;

This is just a preference as someone who reads a lot of code.  Having error handling
as the out of line path is more common and so what my brain (and other reviewers)
has long been trained to expect.

> +}
> +
>  int cca_update_device_object_cache(struct pci_dev *pdev, struct cca_guest_dsc *dsc)
>  {
>  	int ret;
> +	struct page *shared_pages;
> +	struct rhi_vdev_measurement_params *dev_meas;
>  
>  	ret = rhi_update_vdev_interface_report_cache(pdev);
>  	if (ret) {
> @@ -45,5 +87,21 @@ int cca_update_device_object_cache(struct pci_dev *pdev, struct cca_guest_dsc *d
>  		return ret;
>  	}
>  
> +	shared_pages = alloc_shared_pages(NUMA_NO_NODE, GFP_KERNEL, sizeof(struct rhi_vdev_measurement_params));

Perhaps sizeof(*dev_meas) would be both shorter and clearer.

> +	if (!shared_pages)
> +		return -ENOMEM;
> +
> +	dev_meas = (struct rhi_vdev_measurement_params *)page_address(shared_pages);
> +	/* request for signed full transcript */
> +	dev_meas->flags = RHI_VDEV_MEASURE_SIGNED | RHI_VDEV_MEASURE_EXCHANGE;
> +	/* request all measurement block. Set bit 254 */
> +	dev_meas->indices[31] = 0x40;
> +	ret = rhi_update_vdev_measurements_cache(pdev, dev_meas);
> +
> +	free_shared_pages(shared_pages, sizeof(struct rhi_vdev_measurement_params));

It might be worth appropriate DEFINE_FREE() magic to to stash the size away
and simplify this a tiny bit. Kind of depends on whether this is a one off
or those helpers are going to get a reasonable amount of use.

> +	if (ret) {
> +		pci_err(pdev, "failed to get device measurement (%d)\n", ret);
> +		return ret;
> +	}
>  	return 0;
>  }