Re: [PATCH RESEND v2 04/12] coco: host: arm64: Add host TSM callback and IDE stream allocation support

Jonathan Cameron
2025-10-29T17:18:10Z

On Mon, 27 Oct 2025 15:25:54 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> 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.
>
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
Minor stuff inline.

> diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
> index 3dbf0d067cc5..9cabe750533c 100644
> --- a/drivers/firmware/smccc/smccc.c
> +++ b/drivers/firmware/smccc/smccc.c
> @@ -15,6 +15,7 @@
> #include <asm/archrandom.h>
> #ifdef CONFIG_ARM64
> #include <asm/rsi_cmds.h>
> +#include <asm/rmi_smc.h>
> #endif
>
> static u32 smccc_version = ARM_SMCCC_VERSION_1_0;
> @@ -99,10 +100,27 @@ static void __init register_rsi_device(struct platform_device *pdev)
> "arm_cca_guest", RSI_DEV_NAME, NULL, 0);
>
> }
> +
> +static void __init register_rmi_device(struct platform_device *pdev)
> +{
> + struct arm_smccc_res res;
> + unsigned long host_version = RMI_ABI_VERSION(RMI_ABI_MAJOR_VERSION,
> + RMI_ABI_MINOR_VERSION);
> +
> + arm_smccc_1_1_invoke(SMC_RMI_VERSION, host_version, &res);
> + if (res.a0 == RMI_SUCCESS)
> + __devm_auxiliary_device_create(&pdev->dev,
> + "arm_cca_host", RMI_DEV_NAME, NULL, 0);
> +}
> #else
> static void __init register_rsi_device(struct platform_device *pdev)
> {
>
> +}
> +
> +static void __init register_rmi_device(struct platform_device *pdev)
> +{
> +
> }
> #endif

Same comment as before applies. I'd split this to a separate c file and stub
in a header.

> 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..18e5bf6adea4
> --- /dev/null
> +++ b/drivers/virt/coco/arm-cca-host/arm-cca.c
> @@ -0,0 +1,192 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2025 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 <linux/kvm_host.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 rc;
> +
> + if (!is_pci_tsm_pf0(pdev)) {
> + struct cca_host_fn_dsc *fn_dsc __free(kfree) =
> + kzalloc(sizeof(*fn_dsc), GFP_KERNEL);
> +
> + if (!fn_dsc)
> + return NULL;
> +
> + rc = pci_tsm_link_constructor(pdev, &fn_dsc->pci, tsm_dev);
> + if (rc)
> + return NULL;
> +
> + return &no_free_ptr(fn_dsc)->pci;
> + }
> +
> + if (!pdev->ide_cap)
> + return NULL;
> +
> + struct cca_host_pf0_dsc *pf0_dsc __free(kfree) =
> + kzalloc(sizeof(*pf0_dsc), GFP_KERNEL);

Not sure why this indent. I'd go with more consistent choice of just one tab.

struct cca_host_pf0_dsc *pf0_dsc __free(kfree) =
kzalloc(sizeof(*pf0_dsc), GFP_KERNEL);

> + if (!pf0_dsc)
> + return NULL;
> +
> + rc = pci_tsm_pf0_constructor(pdev, &pf0_dsc->pci, tsm_dev);
> + if (rc)
> + return NULL;
> +
> + pci_dbg(pdev, "tsm enabled\n");
> + return &no_free_ptr(pf0_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_dsc *pf0_dsc = to_cca_pf0_dsc(pdev);
> +
> + pci_tsm_pf0_destructor(&pf0_dsc->pci);
> + kfree(pf0_dsc);
> + } else {
> + struct cca_host_fn_dsc *fn_dsc = to_cca_fn_dsc(pdev);
> +
> + kfree(fn_dsc);
> + return;
Maybe something else come in in later patches, but for now this return
is unnecessary.
kfree(to_cca_fn_dsc(pdev));
doesn't loose much if anything wrt to readability.

> + }
> +}
> +
> +/* For now global for simplicity. Protected by pci_tsm_rwsem */
> +static DECLARE_BITMAP(cca_stream_ids, MAX_STREAM_ID);
> +
> +static int cca_tsm_connect(struct pci_dev *pdev)
> +{
> + struct pci_dev *rp = pcie_find_root_port(pdev);
> + struct cca_host_pf0_dsc *pf0_dsc;
> + struct pci_ide *ide;
> + int rc, stream_id;
> +
> + /* Only function 0 supports connect in host */
> + if (WARN_ON(!is_pci_tsm_pf0(pdev)))
> + return -EIO;
> +
> + pf0_dsc = to_cca_pf0_dsc(pdev);
> + /* Allocate stream id */
> + stream_id = find_first_zero_bit(cca_stream_ids, MAX_STREAM_ID);
> + if (stream_id == MAX_STREAM_ID)
> + return -EBUSY;
> + set_bit(stream_id, cca_stream_ids);
> +
> + ide = pci_ide_stream_alloc(pdev);
> + if (!ide) {
> + rc = -ENOMEM;
> + goto err_stream_alloc;
> + }
> +
> + pf0_dsc->sel_stream = ide;
> + ide->stream_id = stream_id;
> + rc = pci_ide_stream_register(ide);
> + if (rc)
> + goto err_stream;
> +
> + pci_ide_stream_setup(pdev, ide);
> + pci_ide_stream_setup(rp, ide);
> +
> + rc = tsm_ide_stream_register(ide);
> + if (rc)
> + 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);
> + return 0;
> +
> +err_tsm:
> + pci_ide_stream_teardown(rp, ide);
> + pci_ide_stream_teardown(pdev, ide);
> + pci_ide_stream_unregister(ide);
> +err_stream:
as below, I'd have
pf0_dsc->sel_stream = NULL;
here

> + pci_ide_stream_free(ide);
> +err_stream_alloc:
> + clear_bit(stream_id, cca_stream_ids);
> +
> + return rc;
> +}
> +
> +static void cca_tsm_disconnect(struct pci_dev *pdev)
> +{
> + int stream_id;
> + struct pci_ide *ide;
> + struct cca_host_pf0_dsc *pf0_dsc;
> +
> + pf0_dsc = to_cca_pf0_dsc(pdev);
> + if (!pf0_dsc)
> + return;
> +
> + ide = pf0_dsc->sel_stream;
> + stream_id = ide->stream_id;
> + pf0_dsc->sel_stream = NULL;
You go through this dance to unset these in disconnect but
not if we get a failure in connect. Whilst it might be fine
it looks a little odd so I'd clear pf0_dsc->sel_stream in the
error path of connect.

> +
> + pci_ide_stream_release(ide);
This helper is a bit irritating as the clearly of pf0_dsc->sel_stream,
if it were in precise opposite of the connect path would occur mid way
through that function. Ah well, looks safe enough to be out of order
just trickier to review.

> + clear_bit(stream_id, cca_stream_ids);
> +}

> +static int cca_link_tsm_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *id)
> +{
> + if (kvm_has_da_feature()) {
Unless you expect to see something else after this, I'd flip logic

struct tsm_dev *tsm_dev;

if (!kvm_has_da_feature())
return -ENODEV;

tsm_dev = tsm_register(&adev->dev, &cca_link_pci_ops);
if (IS_ERR(tsm_dev))
return PTR_ERR(tsm_dev);

return devm_add_action_or_reset(&adev->dev, cca_link_tsm_remove,
tsm_dev);

Here reduces indent and keeps that 'error path' out of line property
that really helps me at least visually parse code.

> + struct tsm_dev *tsm_dev;
> +
> + tsm_dev = tsm_register(&adev->dev, &cca_link_pci_ops);
> + if (IS_ERR(tsm_dev))
> + return PTR_ERR(tsm_dev);
> +
> + return devm_add_action_or_reset(&adev->dev,
> + cca_link_tsm_remove, tsm_dev);
> + }
> + return -ENODEV;
> +}