Re: [RFC PATCH v4 01/14] coco: host: arm64: Add host TSM callback and IDE stream allocation support

["Dan Williams (nvidia)" <djbw@kernel.org>]

Aneesh Kumar K.V (Arm) wrote:
> Register the TSM callback when the DA feature is supported by KVM.
> 
> This driver handles IDE stream setup for both the root port and PCIe
> endpoints. Root port IDE stream enablement itself is managed by RMM.
> 
> In addition, the driver registers pci_tsm_ops with the TSM subsystem.

Do you want to call out that this is an infrastructure / scaffolding
patch that only handles the PCI-TSM skeleton. The CCA meat comes later,
in particular IDE key management. Tell a bit more of the story 

Otherwise, mostly looks good.

Minor comments below...

> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> ---
>  arch/arm64/include/asm/rmi_smc.h         |   2 +
>  drivers/firmware/smccc/rmm.c             |  12 ++
>  drivers/firmware/smccc/rmm.h             |   8 +
>  drivers/firmware/smccc/smccc.c           |   1 +
>  drivers/virt/coco/Kconfig                |   2 +
>  drivers/virt/coco/Makefile               |   1 +
>  drivers/virt/coco/arm-cca-host/Kconfig   |  19 ++
>  drivers/virt/coco/arm-cca-host/Makefile  |   5 +
>  drivers/virt/coco/arm-cca-host/arm-cca.c | 225 +++++++++++++++++++++++
>  drivers/virt/coco/arm-cca-host/rmi-da.h  |  46 +++++
>  10 files changed, 321 insertions(+)
>  create mode 100644 drivers/virt/coco/arm-cca-host/Kconfig
>  create mode 100644 drivers/virt/coco/arm-cca-host/Makefile
>  create mode 100644 drivers/virt/coco/arm-cca-host/arm-cca.c
>  create mode 100644 drivers/virt/coco/arm-cca-host/rmi-da.h
> 
> diff --git a/arch/arm64/include/asm/rmi_smc.h b/arch/arm64/include/asm/rmi_smc.h
> index fa23818e1b4c..109d6cc6ef37 100644
> --- a/arch/arm64/include/asm/rmi_smc.h
> +++ b/arch/arm64/include/asm/rmi_smc.h
[..]
> diff --git a/drivers/firmware/smccc/rmm.c b/drivers/firmware/smccc/rmm.c
> index 2a6187df3285..7444cc3a588c 100644
> --- a/drivers/firmware/smccc/rmm.c
> +++ b/drivers/firmware/smccc/rmm.c
[..]
> diff --git a/drivers/firmware/smccc/rmm.h b/drivers/firmware/smccc/rmm.h
> index a47a650d4f51..37d0d95a099e 100644
> --- a/drivers/firmware/smccc/rmm.h
> +++ b/drivers/firmware/smccc/rmm.h
[..]
> diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
> index fc9b44b7c687..2bf2d59e686d 100644
> --- a/drivers/firmware/smccc/smccc.c
> +++ b/drivers/firmware/smccc/smccc.c
> @@ -97,6 +97,7 @@ static int __init smccc_devices_init(void)
>  		 * the required SMCCC function IDs at a supported revision.
>  		 */
>  		register_rsi_device(pdev);
> +		register_rmi_device(pdev);
>  	}

Would splitting the above three hunks make this series stand on its own
relative to the base CCA series? I assume likely not as soon as we get
to patch2.

Otherwise, just curious what your intended merge strategy is for this,
tsm.git or arm.git, and what help this needs?

[..]
snip code that looks good.

> diff --git a/drivers/virt/coco/arm-cca-host/Makefile b/drivers/virt/coco/arm-cca-host/Makefile
> new file mode 100644
> index 000000000000..c236827f002c
> --- /dev/null
> +++ b/drivers/virt/coco/arm-cca-host/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +obj-$(CONFIG_ARM_CCA_HOST) += arm-cca-host.o
> +
> +arm-cca-host-y	+=  arm-cca.o
> diff --git a/drivers/virt/coco/arm-cca-host/arm-cca.c b/drivers/virt/coco/arm-cca-host/arm-cca.c
> new file mode 100644
> index 000000000000..67f7e80106e8
> --- /dev/null
> +++ b/drivers/virt/coco/arm-cca-host/arm-cca.c
> @@ -0,0 +1,225 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2026 ARM Ltd.
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/pci-tsm.h>
> +#include <linux/pci-ide.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/tsm.h>
> +#include <linux/vmalloc.h>
> +#include <linux/cleanup.h>
> +
> +#include "rmi-da.h"
> +
> +/* Total number of stream id supported at root port level */
> +#define MAX_STREAM_ID	256
> +
> +static struct pci_tsm *cca_tsm_pci_probe(struct tsm_dev *tsm_dev, struct pci_dev *pdev)
> +{
> +	int ret;
> +
> +	if (!is_pci_tsm_pf0(pdev)) {
> +		struct cca_host_fn_dsc *fn_dsc __free(kfree) =
> +			kzalloc(sizeof(*fn_dsc), GFP_KERNEL);

kzalloc_obj(*fn_dsc)

> +
> +		if (!fn_dsc)
> +			return NULL;
> +
> +		ret = pci_tsm_link_constructor(pdev, &fn_dsc->pci, tsm_dev);
> +		if (ret)
> +			return NULL;
> +
> +		return &no_free_ptr(fn_dsc)->pci;
> +	}
> +
> +	if (!pdev->ide_cap)
> +		return NULL;

Bailing early?

Maybe the RMM knows something about this device not needing IDE? I have
a similar question in patch2 around trusted sources for whether a device
is internal or not. 

> +
> +	struct cca_host_pf0_ep_dsc *pf0_ep_dsc __free(kfree) =
> +		kzalloc(sizeof(*pf0_ep_dsc), GFP_KERNEL);
> +	if (!pf0_ep_dsc)
> +		return NULL;
> +
> +	ret = pci_tsm_pf0_constructor(pdev, &pf0_ep_dsc->pci, tsm_dev);
> +	if (ret)
> +		return NULL;
> +
> +	pci_dbg(pdev, "tsm enabled\n");
> +	return &no_free_ptr(pf0_ep_dsc)->pci.base_tsm;
> +}
> +
> +static void cca_tsm_pci_remove(struct pci_tsm *tsm)
> +{
> +	struct pci_dev *pdev = tsm->pdev;
> +
> +	if (is_pci_tsm_pf0(pdev)) {
> +		struct cca_host_pf0_ep_dsc *pf0_ep_dsc = to_cca_pf0_ep_dsc(pdev);
> +
> +		pci_tsm_pf0_destructor(&pf0_ep_dsc->pci);
> +		kfree(pf0_ep_dsc);
> +	} else {
> +		kfree(to_cca_fn_dsc(pdev));
> +	}
> +}
> +
> +/* For now global for simplicity. Protected by pci_tsm_rwsem */
> +static DECLARE_BITMAP(cca_stream_ids, MAX_STREAM_ID);
> +static int alloc_stream_id(struct pci_host_bridge *hb)
> +{
> +	int stream_id;
> +
> +redo_alloc:
> +	stream_id = find_first_zero_bit(cca_stream_ids, MAX_STREAM_ID);
> +	if (stream_id == MAX_STREAM_ID)
> +		return stream_id;
> +
> +	if (ida_exists(&hb->ide_stream_ids_ida, stream_id)) {
> +		/* mark the stream allocated in the global bitmap. */
> +		set_bit(stream_id, cca_stream_ids);
> +		goto redo_alloc;
> +	}
> +	return stream_id;

Is 256 total an RMM limit, and/or does it require globally unique
stream-ids? If not you could do what SEV-TIO does and just set stream-id
== stream-index.

> +}
> +
> +static inline bool cca_pdev_need_sel_ide_streams(struct pci_dev *pdev)
> +{
> +	return pci_pcie_type(pdev) == PCI_EXP_TYPE_ENDPOINT;
> +}
> +
> +static int cca_tsm_connect(struct pci_dev *pdev)
> +{
> +	struct pci_dev *rp = pcie_find_root_port(pdev);
> +	struct cca_host_pf0_ep_dsc *pf0_ep_dsc;
> +	struct pci_ide *ide;
> +	int ret, stream_id = 0;
> +
> +	/* Only function 0 supports connect in host */
> +	if (WARN_ON(!is_pci_tsm_pf0(pdev)))
> +		return -EIO;
> +
> +	pf0_ep_dsc = to_cca_pf0_ep_dsc(pdev);
> +	if (cca_pdev_need_sel_ide_streams(pdev)) {
> +		/* Allocate stream id */
> +		stream_id = alloc_stream_id(pci_find_host_bridge(pdev->bus));
> +		if (stream_id == MAX_STREAM_ID)
> +			return -EBUSY;
> +		set_bit(stream_id, cca_stream_ids);
> +
> +		ide = pci_ide_stream_alloc(pdev);
> +		if (!ide) {
> +			ret = -ENOMEM;
> +			goto err_stream_alloc;
> +		}
> +
> +		pf0_ep_dsc->sel_stream = ide;
> +		ide->stream_id = stream_id;
> +		ret = pci_ide_stream_register(ide);
> +		if (ret)
> +			goto err_stream;
> +		/*
> +		 * Configure IDE capability for target device
> +		 *
> +		 * Some test devices work only with DEFAULT_STREAM enabled.
> +		 * For simplicity, enable DEFAULT_STREAM for all devices. A
> +		 * future decent solution may be to have a quirk table to
> +		 * specify which devices need DEFAULT_STREAM.
> +		 */
> +		ide->partner[PCI_IDE_EP].default_stream = 1;
> +		pci_ide_stream_setup(pdev, ide);
> +		pci_ide_stream_setup(rp, ide);
> +
> +		ret = tsm_ide_stream_register(ide);
> +		if (ret)
> +			goto err_tsm;
> +
> +		/*
> +		 * Once ide is setup, enable the stream at the endpoint
> +		 * Root port will be done by RMM
> +		 */
> +		pci_ide_stream_enable(pdev, ide);

The end point of these patches follows the spec recommendation of
delaying enable until after key programming.

> +	}
> +	return 0;

Should this be making security claims to userspace without taking any
action for non-endpoint devices that happen to be passed in?

Thinking about a bisection case this should either fail here, print a
message that is removed in the final enabling patch, or do the
__maybe_unused arrangement to land all the CCA bits first and then do
this hookup. Up to you.