Re: [PATCH RESEND v2 05/12] coco: host: arm64: Build and register RMM pdev descriptors

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

On Mon, 27 Oct 2025 15:25:55 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> wrote:

> Add the SMCCC plumbing for RMI_PDEV_AUX_COUNT, RMI_PDEV_CREATE, and
> RMI_PDEV_GET_STATE, describe the pdev state enum/flags in rmi_smc.h,
> and extend the PF0 descriptor so we can hold the RMM-side pdev handle
> plus its auxiliary granules.
> 
> Implement pdev_create() to delegate backing pages, populate the pdev
> parameters from the device's RID, ECAM window, IDE stream, and
> non-coherent address ranges, and invoke RMI_PDEV_CREATE. The helper
> keeps track of the allocated/assigned granules and unwinds them on
> failure, so the host driver can reliably establish the pdev channel
> before kicking off further IDE/TSM setup.
> 
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
Hi Aneesh

A few things inline.

J
> ---
>  arch/arm64/include/asm/rmi_cmds.h       |  31 ++++++
>  arch/arm64/include/asm/rmi_smc.h        |  94 +++++++++++++++-
>  drivers/virt/coco/arm-cca-host/Makefile |   2 +-
>  drivers/virt/coco/arm-cca-host/rmi-da.c | 141 ++++++++++++++++++++++++
>  drivers/virt/coco/arm-cca-host/rmi-da.h |   5 +
>  5 files changed, 271 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/virt/coco/arm-cca-host/rmi-da.c
> 

> diff --git a/arch/arm64/include/asm/rmi_smc.h b/arch/arm64/include/asm/rmi_smc.h
> index fe1c91ffc0ab..10f87a18f09a 100644
> --- a/arch/arm64/include/asm/rmi_smc.h
> +++ b/arch/arm64/include/asm/rmi_smc.h
> @@ -26,7 +26,7 @@
>  #define SMC_RMI_DATA_CREATE		SMC_RMI_CALL(0x0153)
>  #define SMC_RMI_DATA_CREATE_UNKNOWN	SMC_RMI_CALL(0x0154)
>  #define SMC_RMI_DATA_DESTROY		SMC_RMI_CALL(0x0155)
> -

I'd avoid the white space change. If it makes sense push it to the KVM series
that I guess this was introduced in.

> +#define SMC_RMI_PDEV_AUX_COUNT		SMC_RMI_CALL(0x0156)

>  #define RMI_ABI_MAJOR_VERSION	1
>  #define RMI_ABI_MINOR_VERSION	0
>  
> @@ -269,4 +272,93 @@ struct rec_run {
>  	struct rec_exit exit;
>  };

> +
> +#define MAX_PDEV_AUX_GRANULES	32
> +#define MAX_IOCOH_ADDR_RANGE	16
> +#define MAX_FCOH_ADDR_RANGE	4
> +
> +#define RMI_PDEV_FLAGS_SPDM		BIT(0)
> +#define RMI_PDEV_FLAGS_NCOH_IDE		BIT(1)
> +#define RMI_PDEV_FLAGS_NCOH_ADDR	BIT(2)
> +#define RMI_PDEV_FLAGS_COH_IDE		BIT(3)
> +#define RMI_PDEV_FLAGS_COH_ADDR		BIT(4)
> +#define RMI_PDEV_FLAGS_P2P		BIT(5)
> +#define RMI_PDEV_FLAGS_COMP_TRUST	BIT(6)
> +#define RMI_PDEV_FLAGS_CATEGORY		GENMASK(8, 7)
> +
> +#define RMI_PDEV_CMEM_CXL_CATEGORY	BIT(7)

This smells like it's the value 1 in the RMI_PDEV_FLAGS_CATEGORY field?
If so don't use a bit definition like this. Instead a suitably named field value definition.
e.g. #define RMI_PDEV_FLAGS_CATEGORY_CXL 1


> diff --git a/drivers/virt/coco/arm-cca-host/rmi-da.c b/drivers/virt/coco/arm-cca-host/rmi-da.c
> new file mode 100644
> index 000000000000..390b8f05c7cf
> --- /dev/null
> +++ b/drivers/virt/coco/arm-cca-host/rmi-da.c

> +
> +static int init_pdev_params(struct pci_dev *pdev, struct rmi_pdev_params *params)
> +{
> +	int rid, ret, i;
> +	phys_addr_t aux_phys;
> +	struct pci_config_window *cfg = pdev->bus->sysdata;
> +	struct cca_host_pf0_dsc *pf0_dsc = to_cca_pf0_dsc(pdev);
> +	struct pci_ide *ide = pf0_dsc->sel_stream;
> +
> +	/* assign the ep device with RMM */
> +	rid = pci_dev_id(pdev);
> +	params->pdev_id = rid;
> +	/* slot number for certificate chain */
> +	params->cert_id = 0;
> +	/* io coherent spdm/ide and non p2p */
> +	params->flags = RMI_PDEV_FLAGS_SPDM | RMI_PDEV_FLAGS_NCOH_IDE |
> +			RMI_PDEV_FLAGS_NCOH_ADDR;
> +	params->ncoh_ide_sid = ide->stream_id;
> +	params->hash_algo = RMI_HASH_SHA_256;
> +	/* use the rid and MMIO resources of the end point pdev */
> +	params->rid_base = rid;
> +	params->rid_top = params->rid_base + 1;
> +	params->ecam_addr = cfg->res.start;
> +	params->root_id = pci_dev_id(pcie_find_root_port(pdev));
> +
> +	params->ncoh_num_addr_range = pci_ide_aassoc_register_to_pdev_addr(params->ncoh_addr_range,
> +								ARRAY_SIZE(params->ncoh_addr_range),
> +								&ide->partner[PCI_IDE_RP]);
I'd format as:
	params->ncoh_num_addr_range =
		pci_ide_aassoc_register_to_pdev_addr(params->ncoh_addr_range,
						     ARRAY_SIZE(params->ncoh_addr_range),
						     &ide->partner[PCI_IDE_RP]);

> +
> +	rmi_pdev_aux_count(params->flags, &params->num_aux);
> +	pf0_dsc->num_aux = params->num_aux;
> +	for (i = 0; i < params->num_aux; i++) {
> +		void *aux =  (void *)__get_free_page(GFP_KERNEL);
One space only before cast.

> +
> +		if (!aux) {
> +			ret = -ENOMEM;
> +			goto err_free_aux;
> +		}
> +
> +		aux_phys = virt_to_phys(aux);
> +		if (rmi_granule_delegate(aux_phys)) {
> +			ret = -ENXIO;
> +			free_page((unsigned long)aux);
> +			goto err_free_aux;
> +		}
> +		params->aux_granule[i] = aux_phys;
> +		pf0_dsc->aux[i] = aux;
> +	}
> +	return 0;
> +
> +err_free_aux:
> +	free_aux_pages(i, pf0_dsc->aux);
> +	return ret;
> +}

> diff --git a/drivers/virt/coco/arm-cca-host/rmi-da.h b/drivers/virt/coco/arm-cca-host/rmi-da.h
> index 01dfb42cd39e..6764bf8d98ce 100644
> --- a/drivers/virt/coco/arm-cca-host/rmi-da.h
> +++ b/drivers/virt/coco/arm-cca-host/rmi-da.h

>  struct cca_host_fn_dsc {
> @@ -38,4 +42,5 @@ static inline struct cca_host_fn_dsc *to_cca_fn_dsc(struct pci_dev *pdev)
>  	return container_of(tsm, struct cca_host_fn_dsc, pci);
>  }
>  
> +int pdev_create(struct pci_dev *pdev);
That is a very generic name to find in a header, even one buried deep in drivers.
I'd prefix it with somethin more specific rmi_pdev_create() or something like that.

>  #endif