On Mon, 28 Jul 2025 19:21:52 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
