From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 958861A5B9D; Sat, 2 Aug 2025 10:54:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1754132081; cv=none; b=ORno8zz5F8/W/6u1xhptRX7LIce3V+yJgCdGliwiLZr6GwI9alqo/iVAu6myvXsxSbXdXHMIdz6yy9kWwQvmBqz+0TD+m+8tmIWpTCqLZFXMEGhHgilF7dZM5X24PGkdTAcwlkoXktlg2HOJVZPa25dEOAQB8SLNTCTw4xGt5Y4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1754132081; c=relaxed/simple; bh=O1FbT7nkwc9fzVfMdXpkf1PCeahMIKJ1NDoaIjn3lVc=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=RjCZfLyLhBeTQDArggnnB4QW80w40LObVeTDDkWWFDpWG1cz2sXZYDrO0owBD/uHgu8nKq8WtsyOkUXAjQyRoxs/j89qhHsKuSEx3Xfhux6RSYX5tstHi9HjalaWSCb/r84I+rtwG6dEpDb8Bp8xgmfUNUJWRr6kDIun4mhpzF4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=K/LTkmPc; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="K/LTkmPc" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 88C5BC4CEEF; Sat, 2 Aug 2025 10:54:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1754132081; bh=O1FbT7nkwc9fzVfMdXpkf1PCeahMIKJ1NDoaIjn3lVc=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=K/LTkmPcxEdimJ4/ZtLElXXa+g2dxn8ADkRyZfHjwdjM65VRXAYVJ0r9W2u6bWM6o CnanAZjhmKBGldJdiESIpW9Nqezi+rwXNSKKWTaPUwJlRc98jBPyz6RemIBFzAc2yu dJh8LMKW75xPHPYblMs/FfEazaQN4ooJQz74/sNptNfgrVOq4yBlcVc/vmOAVhXfkH 3yIYI7JhTpCAzv6tHCp4RnBm+QhEYkCLgTllaYRDSxgUoFhmSSDqXad3A+e5K4K9PM MaHpdsbjSm1MJN3EWSLrlbYQ2vT601FdoSIxeWUkKbGcGGOnhKtDZkGedPuLjLgLT8 EJA/Y9yj0vS9Q== X-Mailer: emacs 30.1 (via feedmail 11-beta-1 I) From: Aneesh Kumar K.V To: Jonathan Cameron Cc: linux-coco@lists.linux.dev, kvmarm@lists.linux.dev, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, aik@amd.com, lukas@wunner.de, Samuel Ortiz , Xu Yilun , Jason Gunthorpe , Suzuki K Poulose , Steven Price , Catalin Marinas , Marc Zyngier , Will Deacon , Oliver Upton Subject: Re: [RFC PATCH v1 13/38] coco: host: arm64: Create a PDEV with rmm In-Reply-To: <20250730133959.0000051b@huawei.com> References: <20250728135216.48084-1-aneesh.kumar@kernel.org> <20250728135216.48084-14-aneesh.kumar@kernel.org> <20250730133959.0000051b@huawei.com> Date: Sat, 02 Aug 2025 16:24:32 +0530 Message-ID: Precedence: bulk X-Mailing-List: linux-pci@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain Jonathan Cameron writes: > On Mon, 28 Jul 2025 19:21:50 +0530 > "Aneesh Kumar K.V (Arm)" wrote: > >> Create the realm physical device with RMM. >> >> Signed-off-by: Aneesh Kumar K.V (Arm) > Hi Aneesh, > > Various small things inline. > > Jonathan > >> --- >> arch/arm64/include/asm/rmi_cmds.h | 31 +++++ >> arch/arm64/include/asm/rmi_smc.h | 72 ++++++++++- >> drivers/virt/coco/arm-cca-host/Makefile | 2 +- >> drivers/virt/coco/arm-cca-host/arm-cca.c | 10 +- >> drivers/virt/coco/arm-cca-host/rmm-da.c | 150 +++++++++++++++++++++++ >> drivers/virt/coco/arm-cca-host/rmm-da.h | 5 + >> 6 files changed, 267 insertions(+), 3 deletions(-) >> create mode 100644 drivers/virt/coco/arm-cca-host/rmm-da.c >> >> diff --git a/arch/arm64/include/asm/rmi_cmds.h b/arch/arm64/include/asm/rmi_cmds.h >> index ef53147c1984..f0817bd3bab4 100644 >> --- a/arch/arm64/include/asm/rmi_cmds.h >> +++ b/arch/arm64/include/asm/rmi_cmds.h >> @@ -505,4 +505,35 @@ static inline int rmi_rtt_unmap_unprotected(unsigned long rd, >> return res.a0; >> } >> >> +static inline unsigned long rmi_pdev_aux_count(unsigned long flags, u64 *aux_count) >> +{ >> + struct arm_smccc_res res; >> + >> + arm_smccc_1_1_invoke(SMC_RMI_PDEV_AUX_COUNT, flags, &res); >> + >> + *aux_count = res.a1; >> + return res.a0; >> +} >> + >> +static inline unsigned long rmi_pdev_create(unsigned long pdev_phys, >> + unsigned long pdev_params_phys) >> +{ >> + struct arm_smccc_res res; >> + >> + arm_smccc_1_1_invoke(SMC_RMI_PDEV_CREATE, >> + pdev_phys, pdev_params_phys, &res); >> + >> + return res.a0; >> +} >> + >> +static inline unsigned long rmi_pdev_get_state(unsigned long pdev_phys, unsigned long *state) > > I'd use the enum for the state type and range check in here. > > >> +{ >> + struct arm_smccc_res res; >> + >> + arm_smccc_1_1_invoke(SMC_RMI_PDEV_GET_STATE, pdev_phys, &res); >> + >> + *state = res.a1; >> + 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 42708d500048..a84ed61e5001 100644 >> --- a/arch/arm64/include/asm/rmi_smc.h >> +++ b/arch/arm64/include/asm/rmi_smc.h >> @@ -26,7 +26,7 @@ >> #define SMC_RMI_DATA_CREATE SMC_RMI_CALL(0x0153) >> #define SMC_RMI_DATA_CREATE_UNKNOWN SMC_RMI_CALL(0x0154) >> #define SMC_RMI_DATA_DESTROY SMC_RMI_CALL(0x0155) >> - > > Probably keep > >> +#define SMC_RMI_PDEV_AUX_COUNT SMC_RMI_CALL(0x0156) > > and add one here as different groups. > >> #define SMC_RMI_REALM_ACTIVATE SMC_RMI_CALL(0x0157) >> #define SMC_RMI_REALM_CREATE SMC_RMI_CALL(0x0158) >> #define SMC_RMI_REALM_DESTROY SMC_RMI_CALL(0x0159) >> @@ -47,6 +47,9 @@ >> #define SMC_RMI_RTT_INIT_RIPAS SMC_RMI_CALL(0x0168) >> #define SMC_RMI_RTT_SET_RIPAS SMC_RMI_CALL(0x0169) >> >> +#define SMC_RMI_PDEV_CREATE SMC_RMI_CALL(0x0176) >> +#define SMC_RMI_PDEV_GET_STATE SMC_RMI_CALL(0x0178) >> + >> #define RMI_ABI_MAJOR_VERSION 1 >> #define RMI_ABI_MINOR_VERSION 0 >> >> @@ -268,4 +271,71 @@ struct rec_run { >> struct rec_exit exit; >> }; >> >> +enum rmi_pdev_state { > Type never used, but I think it should be. See above. > >> + RMI_PDEV_NEW, >> + RMI_PDEV_NEEDS_KEY, >> + RMI_PDEV_HAS_KEY, >> + RMI_PDEV_READY, > Seems someone put another state in here, but I guess you'll update > when the rest of the stack catches up. > RMI_PDEV_IDE_RESETTING, > > Maybe throw a comment here for now. > >> + RMI_PDEV_COMMUNICATING, >> + RMI_PDEV_STOPPING, >> + RMI_PDEV_STOPPED, >> + RMI_PDEV_ERROR, >> +}; >> + >> +#define MAX_PDEV_AUX_GRANULES 32 >> +#define MAX_IOCOH_ADDR_RANGE 16 >> +#define MAX_FCOH_ADDR_RANGE 4 >> + >> +#define RMI_PDEV_SPDM_TRUE BIT(0) >> +#define RMI_PDEV_IDE_TRUE BIT(1) >> +#define RMI_PDEV_FOCOH BIT(2) >> +#define RMI_PDEV_P2P_STREAM BIT(3) > > I'd stick flags in the name (assuming this is > RmiPDevFlags? I'm not sure as the spec I could > find has different usages other after BIT(!)) > > >> + >> +#define RMI_HASH_SHA_256 0 >> +#define RMI_HASH_SHA_512 1 >> + >> +struct rmi_pdev_addr_range { >> + unsigned long base; >> + unsigned long top; > > Whilst we only care about platforms where this is u64, maybe > just make that explicit so we can trivially see this > matches the spec? > >> +}; >> + >> +struct rmi_pdev_params { > > Reference? Looks like B4.4.25 RmiPdevParams type > in alp15. Though as there are some fields missing > I guess this chagned. > This patch series is currently based on the ALP12 specification, available here: https://developer.arm.com/-/cdn-downloads/permalink/Architectures/Armv9/DEN0137_1.1-alp12.zip The next revision of the series will be updated to align with the ALP15 specification. > >> + union { >> + struct { >> + u64 flags; >> + u64 pdev_id; >> + u64 segment_id; >> + u64 ecam_addr; >> + u64 root_id; >> + u64 cert_id; >> + u64 rid_base; >> + u64 rid_top; >> + u64 hash_algo; >> + u64 num_aux; >> + u64 ide_sid; > > Called ncoh_ide_side in the alpha 15. Maybe match that unless > it is changing name in future version. > >> + u64 ncoh_num_addr_range; >> + u64 coh_num_addr_range; >> + }; >> + u8 padding1[0x100]; >> + }; >> + >> + union { /* 0x100 */ >> + u64 aux_granule[MAX_PDEV_AUX_GRANULES]; >> + u8 padding2[0x100]; >> + }; >> + >> + union { /* 0x200 */ >> + struct { >> + struct rmi_pdev_addr_range ncoh_addr_range[MAX_IOCOH_ADDR_RANGE]; >> + }; >> + u8 padding3[0x100]; >> + }; >> + union { /* 0x300 */ >> + struct { >> + struct rmi_pdev_addr_range coh_addr_range[MAX_FCOH_ADDR_RANGE]; >> + }; >> + u8 padding4[0x100]; >> + }; > > Maybe pad out the rest? Mostly so we can see here that it is 4k. > ok > >> +}; >> + >> #endif /* __ASM_RMI_SMC_H */ > >> diff --git a/drivers/virt/coco/arm-cca-host/arm-cca.c b/drivers/virt/coco/arm-cca-host/arm-cca.c >> index c8b0e6db1f47..84d97dd41191 100644 >> --- a/drivers/virt/coco/arm-cca-host/arm-cca.c >> +++ b/drivers/virt/coco/arm-cca-host/arm-cca.c >> @@ -124,7 +124,15 @@ static int cca_tsm_connect(struct pci_dev *pdev) >> rc = tsm_ide_stream_register(pdev, ide); >> if (rc) >> goto err_tsm; >> - > > Tidy up. I'd leave it. > >> + /* >> + * Take a module reference so that we won't call unregister >> + * without rme_unasign_device >> + */ >> + if (!try_module_get(THIS_MODULE)) { >> + rc = -ENXIO; >> + goto err_tsm; >> + } >> + rme_asign_device(pdev); >> /* >> * Once ide is setup enable the stream at endpoint >> * Root port will be done by RMM >> diff --git a/drivers/virt/coco/arm-cca-host/rmm-da.c b/drivers/virt/coco/arm-cca-host/rmm-da.c >> new file mode 100644 >> index 000000000000..426e530ac182 >> --- /dev/null >> +++ b/drivers/virt/coco/arm-cca-host/rmm-da.c >> @@ -0,0 +1,150 @@ > > >> + >> +static int init_pdev_params(struct pci_dev *pdev, struct rmi_pdev_params *params) >> +{ >> + void *aux; > > Probably makes sense to reduce scope of aux to within the loop. > >> + int rid, ret, i; >> + phys_addr_t aux_phys; >> + struct cca_host_dsc_pf0 *dsc_pf0; >> + struct pci_dev *rp = pcie_find_root_port(pdev); > Only used in one place for now anyway. Probably better > to move it down there. > >> + struct pci_config_window *cfg = pdev->bus->sysdata; >> + >> + dsc_pf0 = to_cca_dsc_pf0(pdev); > > I'd do that at declaration. Seems little advantage in doing it > down here. > >> + /* assign the ep device with RMM */ >> + rid = pci_dev_id(pdev); >> + params->pdev_id = rid; >> + /* slot number for certificate chain */ >> + params->cert_id = 0; >> + /* io coherent spdm/ide and non p2p */ >> + params->flags = RMI_PDEV_SPDM_TRUE | RMI_PDEV_IDE_TRUE; >> + params->ide_sid = dsc_pf0->sel_stream->stream_id; >> + params->hash_algo = RMI_HASH_SHA_256; >> + /* use the rid and MMIO resources of the epdev */ > > Spell out epdev or associate it with the pdev here more clearly. > >> + params->rid_top = params->rid_base = rid; >> + params->ecam_addr = cfg->res.start; >> + params->root_id = pci_dev_id(rp); > > params->root_id = pci_dev_id(pcie_find_root_port(pdev)); > to me acts as better documentation fo what is going on than using > the local variable rp. > >> + >> + params->ncoh_num_addr_range = pci_res_to_pdev_addr(params->ncoh_addr_range, >> + ARRAY_SIZE(params->ncoh_addr_range), >> + pdev->resource, >> + DEVICE_COUNT_RESOURCE); >> + >> + rmi_pdev_aux_count(params->flags, ¶ms->num_aux); >> + pr_debug("%s using %ld pdev aux granules\n", __func__, (unsigned long)params->num_aux); >> + dsc_pf0->num_aux = params->num_aux; >> + for (i = 0; i < params->num_aux; i++) { > > void *aux = (void *)__get_free_page(GFP_KERNEL); > >> + aux = (void *)__get_free_page(GFP_KERNEL); >> + if (!aux) { >> + ret = -ENOMEM; >> + goto err_free_aux; >> + } >> + >> + aux_phys = virt_to_phys(aux); >> + if (rmi_granule_delegate(aux_phys)) { >> + ret = -ENXIO; >> + free_page((unsigned long)aux); >> + goto err_free_aux; >> + } >> + params->aux_granule[i] = aux_phys; >> + dsc_pf0->aux[i] = aux; >> + } >> + return 0; >> + >> +err_free_aux: >> + free_aux_pages(i, dsc_pf0->aux[i]); > > I think you want > free_aux_pages(i, dsc_pf0->aux); > Assuming this is supposed to unwind the loop above. > >> + return -ENOMEM; >> +} >> + > Trivial: Single line probably fine here. >> + >> +int rme_asign_device(struct pci_dev *pci_dev) >> +{ >> + int ret; >> + void *rmm_pdev; >> + unsigned long state; >> + phys_addr_t rmm_pdev_phys; >> + struct rmi_pdev_params *params; >> + struct cca_host_dsc_pf0 *dsc_pf0; >> + >> + dsc_pf0 = to_cca_dsc_pf0(pci_dev); > > Might as well save a line with > > struct cca_host_dsc_pf0 *dsc_pf0 = to_cca_dsc_pf0(pci_dev); > >> + rmm_pdev = (void *)get_zeroed_page(GFP_KERNEL); >> + if (!rmm_pdev) { >> + ret = -ENOMEM; > > return -ENOMEM; > >> + goto err_out; >> + } >> + >> + rmm_pdev_phys = virt_to_phys(rmm_pdev); >> + if (rmi_granule_delegate(rmm_pdev_phys)) { >> + ret = -ENXIO; >> + goto err_free_pdev; >> + } >> + >> + params = (struct rmi_pdev_params *)get_zeroed_page(GFP_KERNEL); >> + if (!params) { >> + ret = -ENOMEM; >> + goto err_granule_undelegate; >> + } >> + >> + ret = init_pdev_params(pci_dev, params); >> + if (ret) >> + goto err_free_params; >> + >> + ret = rmi_pdev_create(rmm_pdev_phys, virt_to_phys(params)); >> + pr_info("rmi_pdev_create(0x%llx, 0x%llx): %d\n", rmm_pdev_phys, virt_to_phys(params), ret); > > RFC, but even so I'd demote to debug and use dynamic debug stuff to enable > them for your testing. > >> + if (ret) >> + goto err_free_aux; >> + >> + rmi_pdev_get_state(rmm_pdev_phys, &state); >> + if (state != RMI_PDEV_NEW) >> + goto err_free_aux; > > Nothing to unwind in rmi_pdev_create()? We've told the > RMM about it then we blow away it's resources. Seems unwise! > Maybe this is cleaned up elsewhere but if so a comment is > probably good. > `pdev_create` is expected to set the device state to `RMI_PDEV_NEW`, so the subsequent check is redundant if `rmi_pdev_create` returns success. I will drop that. > >> + >> + dsc_pf0->rmm_pdev = rmm_pdev; >> + free_page((unsigned long)params); >> + return 0; >> + >> +err_free_aux: >> + free_aux_pages(dsc_pf0->num_aux, dsc_pf0->aux); >> +err_free_params: >> + free_page((unsigned long)params); >> +err_granule_undelegate: >> + rmi_granule_undelegate(rmm_pdev_phys); >> +err_free_pdev: >> + free_page((unsigned long)rmm_pdev); >> +err_out: > > One of my favourite nitpicks. Why not just return if nothing to do? > That tends to save a reviewer a bit of scrolling when checking > error paths do correct unwinding. > >> + return ret; >> +} >> diff --git a/drivers/virt/coco/arm-cca-host/rmm-da.h b/drivers/virt/coco/arm-cca-host/rmm-da.h >> index 840cb584acdd..179ba68f2430 100644 >> --- a/drivers/virt/coco/arm-cca-host/rmm-da.h >> +++ b/drivers/virt/coco/arm-cca-host/rmm-da.h >> @@ -14,6 +14,10 @@ >> struct cca_host_dsc_pf0 { >> struct pci_tsm_pf0 pci; >> struct pci_ide *sel_stream; >> + >> + void *rmm_pdev; >> + int num_aux; >> + void *aux[MAX_PDEV_AUX_GRANULES]; >> }; >> >> static inline struct cca_host_dsc_pf0 *to_cca_dsc_pf0(struct pci_dev *pdev) >> @@ -26,4 +30,5 @@ static inline struct cca_host_dsc_pf0 *to_cca_dsc_pf0(struct pci_dev *pdev) >> return container_of(tsm, struct cca_host_dsc_pf0, pci.tsm); >> } >> >> +int rme_asign_device(struct pci_dev *pdev); >> #endif Thanks for the review comments. I'll update the patch with the suggested changes. -aneesh