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

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

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

> 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 ;)
>

The RHI spec update is still pending. In the meantime, the relevant
details can be found in the RMI spec under section B4.4.69, which
describes the RmiVdevMeasureFlags type. I’ll update the cover letter to
reference the specific spec details that these patches are based on.

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

something like

static inline void vdev_meas_params_free(struct rhi_vdev_measurement_params *params)
{
	struct page *pages = virt_to_page(params);

	free_shared_pages(pages, sizeof(struct rhi_vdev_measurement_params));
}
DEFINE_FREE(vdev_meas_params_free, struct rhi_vdev_measurement_params *, if (_T) vdev_meas_params_free(_T))


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

-aneesh