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

[Aneesh Kumar K . V]

"Dan Williams (nvidia)" <djbw@xxxxxxxxxx> writes:

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

Sure, I’ll update the commit message.

>
> Minor comments below...
>
>> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@xxxxxxxxxx>
>> ---
>>  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.
>

Yes, I’ll split this into a separate patch.

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

Yes. This get updated later in
https://lore.kernel.org/all/20260427065121.916615-14-aneesh.kumar@xxxxxxxxxx

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

Yes, I’ll switch to that.

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

Will do the latter. ie, I’ll call tsm_register() only in the final
patch.

-aneesh