linux-kernel - Re: [RFC PATCH v1 15/38] coco: host: arm64: Stop and destroy the physical device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250730145708.00005181@huawei.com>
Date: Wed, 30 Jul 2025 14:57:08 +0100
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>, <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>,
	Catalin Marinas <catalin.marinas@....com>, Marc Zyngier <maz@...nel.org>,
	Will Deacon <will@...nel.org>, Oliver Upton <oliver.upton@...ux.dev>
Subject: Re: [RFC PATCH v1 15/38] coco: host: arm64: Stop and destroy the
 physical device

On Mon, 28 Jul 2025 19:21:52 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@...nel.org> wrote:

> Add support for stopping and destroying physical devices.

I think it's an odd mix to not do create and destroy in a single patch.
Same with start and stop.
Leaves reviewers thinking perhaps you weren't cleaning up properly
an any error paths are much less obvious.

> 
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@...nel.org>
> ---
>  arch/arm64/include/asm/rmi_cmds.h        | 18 ++++++++++++++++++
>  arch/arm64/include/asm/rmi_smc.h         |  2 ++
>  drivers/virt/coco/arm-cca-host/arm-cca.c |  3 +++
>  drivers/virt/coco/arm-cca-host/rmm-da.c  | 21 +++++++++++++++++++++
>  drivers/virt/coco/arm-cca-host/rmm-da.h  |  1 +
>  5 files changed, 45 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/rmi_cmds.h b/arch/arm64/include/asm/rmi_cmds.h
> index eb0034a675bb..d4ea9f8363f5 100644
> --- a/arch/arm64/include/asm/rmi_cmds.h
> +++ b/arch/arm64/include/asm/rmi_cmds.h
> @@ -547,4 +547,22 @@ static inline unsigned long rmi_pdev_communicate(unsigned long pdev_phys,
>  	return res.a0;
>  }
>  
> +static inline unsigned long rmi_pdev_stop(unsigned long pdev_phys)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_1_1_invoke(SMC_RMI_PDEV_STOP, pdev_phys, &res);
> +
> +	return res.a0;
> +}
> +
> +static inline unsigned long rmi_pdev_destroy(unsigned long pdev_phys)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_1_1_invoke(SMC_RMI_PDEV_DESTROY, pdev_phys, &res);
> +
> +	return res.a0;
> +}
> +
>  #endif /* __ASM_RMI_CMDS_H */
> diff --git a/arch/arm64/include/asm/rmi_smc.h b/arch/arm64/include/asm/rmi_smc.h
> index 8bece465b670..9f25a876238e 100644
> --- a/arch/arm64/include/asm/rmi_smc.h
> +++ b/arch/arm64/include/asm/rmi_smc.h
> @@ -49,7 +49,9 @@
>  
>  #define SMC_RMI_PDEV_COMMUNICATE        SMC_RMI_CALL(0x0175)
>  #define SMC_RMI_PDEV_CREATE             SMC_RMI_CALL(0x0176)
> +#define SMC_RMI_PDEV_DESTROY		SMC_RMI_CALL(0x0177)
>  #define SMC_RMI_PDEV_GET_STATE		SMC_RMI_CALL(0x0178)
> +#define SMC_RMI_PDEV_STOP		SMC_RMI_CALL(0x017c)
>  
>  #define RMI_ABI_MAJOR_VERSION	1
>  #define RMI_ABI_MINOR_VERSION	0
> diff --git a/drivers/virt/coco/arm-cca-host/arm-cca.c b/drivers/virt/coco/arm-cca-host/arm-cca.c
> index 294a6ef60d5f..c65b81f0706f 100644
> --- a/drivers/virt/coco/arm-cca-host/arm-cca.c
> +++ b/drivers/virt/coco/arm-cca-host/arm-cca.c
> @@ -210,12 +210,15 @@ static void cca_tsm_disconnect(struct pci_dev *pdev)
>  	ide = dsc_pf0->sel_stream;
>  	dsc_pf0->sel_stream = NULL;
>  	pci_ide_stream_disable(pdev, ide);
> +	rme_unassign_device(pdev);
> +	module_put(THIS_MODULE);
>  	tsm_ide_stream_unregister(ide);
>  	pci_ide_stream_teardown(rp, ide);
>  	pci_ide_stream_teardown(pdev, ide);
>  	pci_ide_stream_unregister(ide);
>  	clear_bit(ide->stream_id, cca_stream_ids);
>  	pci_ide_stream_free(ide);
> +	free_dev_communication_buffers(&dsc_pf0->comm_data);
>  }
>  
>  static const struct pci_tsm_ops cca_pci_ops = {
> diff --git a/drivers/virt/coco/arm-cca-host/rmm-da.c b/drivers/virt/coco/arm-cca-host/rmm-da.c
> index d123940ce82e..ec8c5bfcee35 100644
> --- a/drivers/virt/coco/arm-cca-host/rmm-da.c
> +++ b/drivers/virt/coco/arm-cca-host/rmm-da.c
> @@ -346,3 +346,24 @@ int schedule_rme_ide_setup(struct pci_dev *pdev)
>  
>  	return 0;
>  }
> +
> +void rme_unassign_device(struct pci_dev *pdev)
> +{
> +	unsigned long ret;
> +	unsigned long state;
> +	phys_addr_t rmm_pdev_phys;
> +	struct cca_host_dsc_pf0 *dsc_pf0;
> +
> +	dsc_pf0 = to_cca_dsc_pf0(pdev);
> +	rmm_pdev_phys = virt_to_phys(dsc_pf0->rmm_pdev);

As with previous patches, I'd set as many of these are seems reasonable at the
variable declations.

> +	ret = rmi_pdev_stop(rmm_pdev_phys);
> +	if (WARN_ON(ret != RMI_SUCCESS))
> +		return;
> +
> +	state = do_pdev_communicate(pdev->tsm, RMI_PDEV_STOPPED);
> +	/* ignore the error state and destroy the device */
WARN_ON is rather heavy if you want to ignore it.

> +	WARN_ON(state != RMI_PDEV_STOPPED);
> +	ret = rmi_pdev_destroy(rmm_pdev_phys);
> +	if (WARN_ON(ret != RMI_SUCCESS))
> +		return;
> +}
> diff --git a/drivers/virt/coco/arm-cca-host/rmm-da.h b/drivers/virt/coco/arm-cca-host/rmm-da.h
> index b9ddc4d9112b..c401be55d770 100644
> --- a/drivers/virt/coco/arm-cca-host/rmm-da.h
> +++ b/drivers/virt/coco/arm-cca-host/rmm-da.h
> @@ -71,5 +71,6 @@ static inline struct cca_host_comm_data *to_cca_comm_data(struct pci_dev *pdev)
>  }
>  
>  int rme_asign_device(struct pci_dev *pdev);
> +void rme_unassign_device(struct pci_dev *pdev);
>  int schedule_rme_ide_setup(struct pci_dev *pdev);
>  #endif