Message-ID: <20251119152201.00000876@huawei.com>
Date: Wed, 19 Nov 2025 15:22:01 +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 v2 01/11] coco: guest: arm64: Guest TSM callback and
 realm device lock support

On Mon, 17 Nov 2025 19:29:57 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@...nel.org> wrote:

> Register the TSM callback when the DA feature is supported by RSI. The
> build order is also adjusted so that the TSM class is created before the
> arm-cca-guest driver is initialized.
> 
> In addition, add support for the TDISP lock sequence. Writing a TSM
> (TEE Security Manager) device name from `/sys/class/tsm` into `tsm/lock`
> triggers the realm device lock operation.
> 
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@...nel.org>
Hi Aneesh,

Some minor stuff in here.   One general comment is that there
are some fixlets for the CCA code surrounding what this patch
should focus on. Break those out as separate cleanup or base
this on top of the fixed up version of that code.

Thanks,

Jonathan

> diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c
> index 1b716d18b80e..2ec0f5dff02e 100644
> --- a/arch/arm64/kernel/rsi.c
> +++ b/arch/arm64/kernel/rsi.c
> @@ -15,6 +15,7 @@
>  #include <asm/rsi.h>
>  
>  static struct realm_config config;
> +static unsigned long rsi_feat_reg0;
>  
>  unsigned long prot_ns_shared;
>  EXPORT_SYMBOL(prot_ns_shared);
> @@ -22,6 +23,12 @@ EXPORT_SYMBOL(prot_ns_shared);
>  DEFINE_STATIC_KEY_FALSE_RO(rsi_present);
>  EXPORT_SYMBOL(rsi_present);
>  
> +bool rsi_has_da_feature(void)
> +{
> +	return u64_get_bits(rsi_feat_reg0, RSI_FEATURE_REGISTER_0_DA);

I'm not keen on mixing explicit size of a u64 with the implicit
fact an unsigned long is effectively a u64.  Can we tidy up the types
to match?
 
> +}
> +EXPORT_SYMBOL_GPL(rsi_has_da_feature);

> diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
> index cb52021912b3..57556d7c1cec 100644
> --- a/drivers/virt/coco/Makefile
> +++ b/drivers/virt/coco/Makefile
> @@ -6,6 +6,6 @@ obj-$(CONFIG_EFI_SECRET)	+= efi_secret/
>  obj-$(CONFIG_ARM_PKVM_GUEST)	+= pkvm-guest/
>  obj-$(CONFIG_SEV_GUEST)		+= sev-guest/
>  obj-$(CONFIG_INTEL_TDX_GUEST)	+= tdx-guest/
> -obj-$(CONFIG_ARM_CCA_GUEST)	+= arm-cca-guest/
>  obj-$(CONFIG_TSM) 		+= tsm-core.o
>  obj-$(CONFIG_TSM_GUEST)		+= guest/
> +obj-$(CONFIG_ARM_CCA_GUEST)	+= arm-cca-guest/

Check all patches for noise like this.  It may be a valid change but in this series
it's just adding confusion.

> diff --git a/drivers/virt/coco/arm-cca-guest/Kconfig b/drivers/virt/coco/arm-cca-guest/Kconfig
> index a42359a90558..66b2d9202b66 100644
> --- a/drivers/virt/coco/arm-cca-guest/Kconfig
> +++ b/drivers/virt/coco/arm-cca-guest/Kconfig
> @@ -1,11 +1,17 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +
>  config ARM_CCA_GUEST
>  	tristate "Arm CCA Guest driver"
>  	depends on ARM64
> +	depends on PCI_TSM
>  	select TSM_REPORTS
>  	select AUXILIARY_BUS
> +	select TSM
>  	help
> -	  The driver provides userspace interface to request and
> +	  The driver provides userspace interface to request an

Push this fixlet to the series adding this or a follow up if that isn't being respun.
Definitely shouldn't be in here.

>  	  attestation report from the Realm Management Monitor(RMM).
> +	  If the DA feature is supported, it also register with TSM framework.
>  
>  	  If you choose 'M' here, this module will be called
>  	  arm-cca-guest.
> diff --git a/drivers/virt/coco/arm-cca-guest/Makefile b/drivers/virt/coco/arm-cca-guest/Makefile
> index 75a120e24fda..bc3b2be4019f 100644
> --- a/drivers/virt/coco/arm-cca-guest/Makefile
> +++ b/drivers/virt/coco/arm-cca-guest/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +#

Stray change.

>  obj-$(CONFIG_ARM_CCA_GUEST) += arm-cca-guest.o
>  
>  arm-cca-guest-y +=  arm-cca.o
> diff --git a/drivers/virt/coco/arm-cca-guest/arm-cca.c b/drivers/virt/coco/arm-cca-guest/arm-cca.c
> index dc96171791db..288fa53ad0af 100644
> --- a/drivers/virt/coco/arm-cca-guest/arm-cca.c
> +++ b/drivers/virt/coco/arm-cca-guest/arm-cca.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Copyright (C) 2023 ARM Ltd.
> + * Copyright (C) 2025 ARM Ltd.
Usually we just extend the date span rather than claiming everything is
2025. e.g.
 * Copyright (C) 2023-2025 ARM Ltd.

>   */

> @@ -192,6 +194,57 @@ static void unregister_cca_tsm_report(void *data)
>  	tsm_report_unregister(&arm_cca_tsm_report_ops);
>  }
>  
> +static struct pci_tsm *cca_tsm_lock(struct tsm_dev *tsm_dev, struct pci_dev *pdev)
> +{
> +	int ret;
> +
> +	struct cca_guest_dsc *cca_dsc __free(kfree) =
> +		kzalloc(sizeof(*cca_dsc), GFP_KERNEL);
> +	if (!cca_dsc)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ret = pci_tsm_devsec_constructor(pdev, &cca_dsc->pci, tsm_dev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return ERR_PTR(-EIO);

Perhaps add a comment so you have something like

	return ERR_PTR(-EIO); /* For now always return an error */

Just makes it a tiny bit easier to review as no need to go check this
is removed in a later patch.

> +}
> +
> +static void cca_tsm_unlock(struct pci_tsm *tsm)
> +{
> +	struct cca_guest_dsc *cca_dsc = to_cca_guest_dsc(tsm->pdev);
> +
> +	kfree(cca_dsc);
> +}
> +
> +static struct pci_tsm_ops cca_devsec_pci_ops = {
> +	.lock = cca_tsm_lock,
> +	.unlock = cca_tsm_unlock,
> +};
> +
> +static void cca_devsec_tsm_remove(void *tsm_dev)
> +{
> +	tsm_unregister(tsm_dev);
> +}
> +
> +static int cca_devsec_tsm_register(struct auxiliary_device *adev)
> +{
> +	struct tsm_dev *tsm_dev;
> +	int rc;
> +
> +	tsm_dev = tsm_register(&adev->dev, &cca_devsec_pci_ops);
> +	if (IS_ERR(tsm_dev))
> +		return PTR_ERR(tsm_dev);
> +
> +	rc = devm_add_action_or_reset(&adev->dev, cca_devsec_tsm_remove, tsm_dev);
> +	if (rc) {
> +		cca_devsec_tsm_remove(tsm_dev);

Take a look at what the _or_reset() does in the devm_add_action_or_reset().
Short story, you should not need this call if the devm machinery has returned
an error.

> +		return rc;
> +	}
> +
> +	return 0;
> +}

> diff --git a/drivers/virt/coco/arm-cca-guest/rsi-da.h b/drivers/virt/coco/arm-cca-guest/rsi-da.h
> new file mode 100644
> index 000000000000..5ad3b740710e
> --- /dev/null
> +++ b/drivers/virt/coco/arm-cca-guest/rsi-da.h

> +static inline int rsi_vdev_id(struct pci_dev *pdev)
> +{
> +	return (pci_domain_nr(pdev->bus) << 16) |
> +	       PCI_DEVID(pdev->bus->number, pdev->devfn);

I'm struggling to find where this is actually defined in the various specs
so good to have a reference in a comment here.

> +}
> +
> +#endif

Powered by blists - more mailing lists