Re: [RFC PATCH v1 38/38] coco: guest: arm64: Add support for fetching device info

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

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

> On Mon, 28 Jul 2025 19:22:15 +0530
> "Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> wrote:
>

...

>> +	return ret;
>> +}
>> +
>>  int rsi_device_lock(struct pci_dev *pdev)
>>  {
>>  	unsigned long ret;
>>  	unsigned long tdisp_version;
>>  	struct rsi_device_measurements_params *rsi_dev_meas;
>> +	struct rsi_device_info *dev_info;
>>  	struct cca_guest_dsc *dsm = to_cca_guest_dsc(pdev);
>>  	int vdev_id = (pci_domain_nr(pdev->bus) << 16) |
>>  		PCI_DEVID(pdev->bus->number, pdev->devfn);
>> @@ -252,8 +344,44 @@ int rsi_device_lock(struct pci_dev *pdev)
>>  		return -EIO;
>>  	}
>>  
>> +	/* RMM expects sizeof(dev_info) (512 bytes) aligned address */
>> +	dev_info = kmalloc(sizeof(*dev_info), GFP_KERNEL);
>
> Use a __free(kfree) here (and direct returns on errors) given it's freed
> in all paths and we don't care if it is freed before or after verifying the digests.
>
> I'm being slow today, but what in that enforces the alignment?  I guess
> it's that the structure happens to be big enough that it happens naturally?
>
> I'd allocate max(512, sizeof(*dev_info)) to make it explicitly the case.
>

yes, struct rsi_device_info is 512 bytes.

>
>> +	if (!dev_info) {
>> +		ret = -ENOMEM;
>> +		goto err_out;
>> +	}
>> +
>> +	ret = rsi_rdev_get_info(vdev_id, dsm->instance_id, virt_to_phys(dev_info));
>> +	if (ret != RSI_SUCCESS) {
>> +		pci_err(pdev, "failed to get device digests (%lu)\n", ret);
>> +		ret = -EIO;
>> +		kfree(dev_info);
>> +		goto err_out;
>> +	}
>> +
>> +	dsm->dev_info.attest_type   = dev_info->attest_type;
>> +	dsm->dev_info.cert_id       = dev_info->cert_id;
>> +	dsm->dev_info.hash_algo     = dev_info->hash_algo;
>> +	memcpy(dsm->dev_info.cert_digest, dev_info->cert_digest, SHA512_DIGEST_SIZE);
>> +	memcpy(dsm->dev_info.meas_digest, dev_info->meas_digest, SHA512_DIGEST_SIZE);
>> +	memcpy(dsm->dev_info.report_digest, dev_info->report_digest, SHA512_DIGEST_SIZE);
>> +
>
> Can't you memcpy the whole thing in one go?
>

yes. But won't that be confusing? Is there a difference?
Also struct dsm_device_info is not same as struct rsi_device_info. We
don't need to keep all that padding in dsm_device_info.

>
>> +	kfree(dev_info);
>> +	/*
>> +	 * Verify that the digests of the provided reports match with the
>> +	 * digests from RMM
>> +	 */
>> +	ret = verify_digests(dsm);
>> +	if (ret) {
>> +		pci_err(pdev, "device digest validation failed (%ld)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +err_out:
> I'll always grumble about these.  To me it always makes the code
> less readable. Some others disagree though ;( 
>>  	return ret;
>>  }
>> +
>
> Looks like this should have been in an earlier patch.
>
>>  static inline unsigned long rsi_rdev_start(struct pci_dev *pdev,
>>  					   unsigned long vdev_id, unsigned long inst_id)
>>  {
>> diff --git a/drivers/virt/coco/arm-cca-guest/rsi-da.h b/drivers/virt/coco/arm-cca-guest/rsi-da.h
>> index f26156d9be81..e8953b8e85a3 100644
>> --- a/drivers/virt/coco/arm-cca-guest/rsi-da.h
>> +++ b/drivers/virt/coco/arm-cca-guest/rsi-da.h
>> @@ -10,6 +10,7 @@
>>  #include <linux/pci-tsm.h>
>>  #include <asm/rsi_smc.h>
>>  #include <asm/rhi.h>
>> +#include <crypto/sha2.h>
>>  
>>  struct pci_tdisp_device_interface_report {
>>  	u16 interface_info;
>> @@ -33,6 +34,17 @@ struct pci_tdisp_mmio_range {
>>  #define TSM_INTF_REPORT_MMIO_RESERVED		GENMASK(15, 4)
>>  #define TSM_INTF_REPORT_MMIO_RANGE_ID		GENMASK(31, 16)
>>  
>> +struct dsm_device_info {
>> +	u64 flags;
>> +	u64 attest_type;
>> +	u64 cert_id;
>> +	u64 hash_algo;
>> +	u8 cert_digest[SHA512_DIGEST_SIZE];
>> +	u8 meas_digest[SHA512_DIGEST_SIZE];
>> +	u8 report_digest[SHA512_DIGEST_SIZE];
>> +};
>> +
>
> One probably enough.
>
>> +
>>  struct cca_guest_dsc {
>>  	struct pci_tsm_pf0 pci;
>>  	unsigned long instance_id;
>> @@ -42,6 +54,7 @@ struct cca_guest_dsc {
>>  	int certificate_size;
>>  	void *measurements;
>>  	int measurements_size;
>> +	struct dsm_device_info dev_info;
>>  };
>>  
>>  static inline struct cca_guest_dsc *to_cca_guest_dsc(struct pci_dev *pdev)

-aneesh