[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <yq5acy9e9eon.fsf@kernel.org>
Date: Sat, 02 Aug 2025 16:24:32 +0530
From: Aneesh Kumar K.V <aneesh.kumar@...nel.org>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
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 13/38] coco: host: arm64: Create a PDEV with rmm
Jonathan Cameron <Jonathan.Cameron@...wei.com> writes:
> On Mon, 28 Jul 2025 19:21:50 +0530
> "Aneesh Kumar K.V (Arm)" <aneesh.kumar@...nel.org> wrote:
>
>> Create the realm physical device with RMM.
>>
>> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@...nel.org>
> 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
Powered by blists - more mailing lists