Message-ID: <20251029171800.0000688b@huawei.com>
Date: Wed, 29 Oct 2025 17:18:00 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: "Aneesh Kumar K.V (Arm)" <aneesh.kumar@...nel.org>
CC: <linux-coco@...ts.linux.dev>, <kvmarm@...ts.linux.dev>,
	<linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<dan.j.williams@...el.com>, <aik@....com>, <lukas@...ner.de>, Samuel Ortiz
	<sameo@...osinc.com>, Xu Yilun <yilun.xu@...ux.intel.com>, Jason Gunthorpe
	<jgg@...pe.ca>, Suzuki K Poulose <Suzuki.Poulose@....com>, Steven Price
	<steven.price@....com>, Bjorn Helgaas <helgaas@...nel.org>, Catalin Marinas
	<catalin.marinas@....com>, Marc Zyngier <maz@...nel.org>, Will Deacon
	<will@...nel.org>, Oliver Upton <oliver.upton@...ux.dev>
Subject: Re: [PATCH RESEND v2 04/12] coco: host: arm64: Add host TSM
 callback and IDE stream allocation support

On Mon, 27 Oct 2025 15:25:54 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@...nel.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@...nel.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;
> +}

Powered by blists - more mailing lists