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

Jonathan Cameron
2025-10-29T17:37:53Z

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