Message-ID: <20251029183306.0000485c@huawei.com>
Date: Wed, 29 Oct 2025 18:33:06 +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 RESEND v2 06/12] coco: host: arm64: Add RMM device
 communication helpers

On Mon, 27 Oct 2025 15:25:56 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@...nel.org> wrote:

> - add SMCCC IDs/wrappers for RMI_PDEV_COMMUNICATE/RMI_PDEV_ABORT
> - describe the RMM device-communication ABI (struct rmi_dev_comm_*,
>   cache flags, protocol/object IDs, busy error code)
> - track per-PF0 communication state (buffers, workqueue, cache metadata) and
>   serialize access behind object_lock
> - plumb a DOE/SPDM worker (pdev_communicate_work) plus shared helpers that
>   submit the SMCCC call, cache multi-part responses, and handle retries/abort
> - hook the new helpers into the physical function connect path so IDE
>   setup can drive the device to the expected state
> 
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@...nel.org>

Hi Aneesh,

Comments inline.

> diff --git a/drivers/virt/coco/arm-cca-host/rmi-da.c b/drivers/virt/coco/arm-cca-host/rmi-da.c
> index 390b8f05c7cf..592abe0dd252 100644
> --- a/drivers/virt/coco/arm-cca-host/rmi-da.c
> +++ b/drivers/virt/coco/arm-cca-host/rmi-da.c

> +
> +static int doe_send_req_resp(struct pci_tsm *tsm)
> +{
> +	int ret, data_obj_type;
> +	struct cca_host_comm_data *comm_data = to_cca_comm_data(tsm->pdev);
> +	struct rmi_dev_comm_exit *io_exit = &comm_data->io_params->exit;
> +	u8 protocol = io_exit->protocol;
> +
> +	if (protocol == RMI_PROTOCOL_SPDM)
> +		data_obj_type = PCI_DOE_FEATURE_CMA;
> +	else if (protocol == RMI_PROTOCOL_SECURE_SPDM)
> +		data_obj_type = PCI_DOE_FEATURE_SSESSION;
> +	else
> +		return -EINVAL;
> +
> +	/* delay the send */
> +	if (io_exit->req_delay)
> +		fsleep(io_exit->req_delay);
> +
> +	ret = pci_tsm_doe_transfer(tsm->dsm_dev, data_obj_type,
> +				   comm_data->req_buff, io_exit->req_len,
> +				   comm_data->rsp_buff, PAGE_SIZE);
> +	return ret;

	return pci_tsm_doe_transfer()

> +}
> +
> +static inline bool pending_dev_communicate(struct rmi_dev_comm_exit *io_exit)
> +{
> +	bool pending = io_exit->flags & (RMI_DEV_COMM_EXIT_CACHE_REQ |
> +					 RMI_DEV_COMM_EXIT_CACHE_RSP |
> +					 RMI_DEV_COMM_EXIT_SEND |
> +					 RMI_DEV_COMM_EXIT_WAIT |
> +					 RMI_DEV_COMM_EXIT_MULTI);
> +	return pending;
> +}
> +
> +static int ___do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm)
> +{
> +	int ret, nbytes, cp_len;
> +	struct cache_object **cache_objp, *cache_obj;
> +	struct cca_host_pf0_dsc *pf0_dsc = to_cca_pf0_dsc(tsm->dsm_dev);
> +	struct cca_host_comm_data *comm_data = to_cca_comm_data(tsm->pdev);
> +	struct rmi_dev_comm_enter *io_enter = &comm_data->io_params->enter;
> +	struct rmi_dev_comm_exit *io_exit = &comm_data->io_params->exit;
> +
> +redo_communicate:
> +
> +	if (type == PDEV_COMMUNICATE)
> +		ret = rmi_pdev_communicate(virt_to_phys(pf0_dsc->rmm_pdev),
> +					   virt_to_phys(comm_data->io_params));
> +	else
> +		ret = RMI_ERROR_INPUT;

Treat this separately for simpler code (assuming you don't have other code that
makes this more complex in later patches).

	if (type != PDEV_COMMUNICATE)
		return -ENXIO;

	ret = rmi_pdev_communicate(virt_to_phys(pf0_dsc->rmm_pdev),
				   virt_to_phys(comm_data->io_params));
	if (ret != RMI...

> +	if (ret != RMI_SUCCESS) {
> +		if (ret == RMI_BUSY)
> +			return -EBUSY;
> +		return -ENXIO;
> +	}
> +
> +	if (io_exit->flags & RMI_DEV_COMM_EXIT_CACHE_REQ ||
> +	    io_exit->flags & RMI_DEV_COMM_EXIT_CACHE_RSP) {
> +
> +		switch (io_exit->cache_obj_id) {
> +		case RMI_DEV_VCA:
> +			cache_objp = &pf0_dsc->vca;
> +			break;
> +		case RMI_DEV_CERTIFICATE:
> +			cache_objp = &pf0_dsc->cert_chain.cache;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		cache_obj = *cache_objp;
> +	}
> +
> +	if (io_exit->flags & RMI_DEV_COMM_EXIT_CACHE_REQ)
> +		cp_len = io_exit->req_cache_len;
> +	else
> +		cp_len = io_exit->rsp_cache_len;
> +
> +	/* response and request len should be <= SZ_4k */
> +	if (cp_len > CACHE_CHUNK_SIZE)
> +		return -EINVAL;
> +
> +	if (io_exit->flags & RMI_DEV_COMM_EXIT_CACHE_REQ ||
> +	    io_exit->flags & RMI_DEV_COMM_EXIT_CACHE_RSP) {
> +		int cache_remaining;
> +		struct cache_object *new_obj;
> +
> +		/* new allocation */
> +		if (!cache_obj) {
> +			cache_obj = kvmalloc(struct_size(cache_obj, buf, CACHE_CHUNK_SIZE),
> +					     GFP_KERNEL);
> +			if (!cache_obj)
> +				return -ENOMEM;
> +
> +			cache_obj->size = CACHE_CHUNK_SIZE;
> +			cache_obj->offset = 0;
> +			*cache_objp = cache_obj;
> +		}
> +
> +		cache_remaining = cache_obj->size - cache_obj->offset;
> +		if (cp_len > cache_remaining) {
> +
> +			if (cache_obj->size + CACHE_CHUNK_SIZE > MAX_CACHE_OBJ_SIZE)
> +				return -EINVAL;
> +
> +			new_obj = kvmalloc(struct_size(cache_obj, buf,
> +						       cache_obj->size + CACHE_CHUNK_SIZE),
> +					   GFP_KERNEL);

Is kvrealloc()? Would avoid need for explicit memcpy / freeing of old object.

> +			if (!new_obj)
> +				return -ENOMEM;
> +			memcpy(new_obj, cache_obj, struct_size(cache_obj, buf, cache_obj->size));
> +			new_obj->size = cache_obj->size + CACHE_CHUNK_SIZE;
> +			*cache_objp = new_obj;
> +			kvfree(cache_obj);
> +		}
> +
> +		/* cache object can change above. */
> +		cache_obj = *cache_objp;
> +	}
> +
> +
> +	if (io_exit->flags & RMI_DEV_COMM_EXIT_CACHE_REQ) {
> +		memcpy(cache_obj->buf + cache_obj->offset,
> +		       (comm_data->req_buff + io_exit->req_cache_offset), io_exit->req_cache_len);
> +		cache_obj->offset += io_exit->req_cache_len;
> +	}
> +
> +	if (io_exit->flags & RMI_DEV_COMM_EXIT_CACHE_RSP) {
> +		memcpy(cache_obj->buf + cache_obj->offset,
> +		       (comm_data->rsp_buff + io_exit->rsp_cache_offset), io_exit->rsp_cache_len);
> +		cache_obj->offset += io_exit->rsp_cache_len;
> +	}
> +
> +	/*
> +	 * wait for last packet request from RMM.
> +	 * We should not find this because our device communication is synchronous
> +	 */
> +	if (io_exit->flags & RMI_DEV_COMM_EXIT_WAIT)
> +		return -ENXIO;
> +
> +	/* next packet to send */
> +	if (io_exit->flags & RMI_DEV_COMM_EXIT_SEND) {
> +		nbytes = doe_send_req_resp(tsm);
> +		if (nbytes < 0) {
> +			/* report error back to RMM */
> +			io_enter->status = RMI_DEV_COMM_ERROR;
> +		} else {
> +			/* send response back to RMM */
> +			io_enter->resp_len = nbytes;
> +			io_enter->status = RMI_DEV_COMM_RESPONSE;
> +		}
> +	} else {
> +		/* no data transmitted => no data received */
> +		io_enter->resp_len = 0;
> +		io_enter->status = RMI_DEV_COMM_NONE;
> +	}
> +
> +	if (pending_dev_communicate(io_exit))
> +		goto redo_communicate;
> +
> +	return 0;
> +}
> +
> +static int __do_dev_communicate(enum dev_comm_type type,
> +				struct pci_tsm *tsm, unsigned long error_state)
> +{
> +	int ret;
> +	int state;
> +	struct rmi_dev_comm_enter *io_enter;
> +	struct cca_host_pf0_dsc *pf0_dsc = to_cca_pf0_dsc(tsm->dsm_dev);
> +
> +	io_enter = &pf0_dsc->comm_data.io_params->enter;
> +	io_enter->resp_len = 0;
> +	io_enter->status = RMI_DEV_COMM_NONE;
> +
> +	ret = ___do_dev_communicate(type, tsm);

Think up a more meaningful name.  Counting _ doesn't make for readable code.

> +	if (ret) {
> +		if (type == PDEV_COMMUNICATE)
> +			rmi_pdev_abort(virt_to_phys(pf0_dsc->rmm_pdev));
> +
> +		state = error_state;
> +	} else {
> +		/*
> +		 * Some device communication error will transition the
> +		 * device to error state. Report that.
> +		 */
> +		if (type == PDEV_COMMUNICATE)
> +			ret = rmi_pdev_get_state(virt_to_phys(pf0_dsc->rmm_pdev),
> +						 (enum rmi_pdev_state *)&state);
> +		if (ret)
> +			state = error_state;
Whilst not strictly needed I'd do this as:

		if (type == PDEV_COMMUNICATE) {
			ret = rmi_pdev_get_state(virt_to_phys(pf0_dsc->rmm_pdev),
						 (enum rmi_pdev_state *)&state);
			if (ret)
				state = error_state;
		}

Just to make it clear that reg check is just on the output of the call above.
If we didn't make that call it is definitely zero but nice not to have
to reason about it.


> +	}
> +
> +	if (state == error_state)
> +		pci_err(tsm->pdev, "device communication error\n");
> +
> +	return state;
> +}
> +
> +static int do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm,
> +			      unsigned long target_state,
> +			      unsigned long error_state)
> +{
> +	int state;
> +
> +	do {
> +		state = __do_dev_communicate(type, tsm, error_state);
> +
> +		if (state == target_state || state == error_state)
> +			break;

Might as well return rather than break;

> +	} while (1);
> +
> +	return state;
> +}

> +void pdev_communicate_work(struct work_struct *work)
> +{
> +	unsigned long state;
> +	struct pci_tsm *tsm;
> +	struct dev_comm_work *setup_work;
> +	struct cca_host_pf0_dsc *pf0_dsc;
> +
> +	setup_work = container_of(work, struct dev_comm_work, work);
> +	tsm = setup_work->tsm;
> +	pf0_dsc = to_cca_pf0_dsc(tsm->dsm_dev);
Could combine these 3 with declarations for shorter code without much
change to readability.

> +
> +	guard(mutex)(&pf0_dsc->object_lock);
> +	state = do_pdev_communicate(tsm, setup_work->target_state);
> +	WARN_ON(state != setup_work->target_state);
> +
> +	complete(&setup_work->complete);
> +}


> diff --git a/drivers/virt/coco/arm-cca-host/rmi-da.h b/drivers/virt/coco/arm-cca-host/rmi-da.h
> index 6764bf8d98ce..1d513e0b74d9 100644
> --- a/drivers/virt/coco/arm-cca-host/rmi-da.h
> +++ b/drivers/virt/coco/arm-cca-host/rmi-da.h

> +struct cca_host_comm_data {
> +	void *rsp_buff;
> +	void *req_buff;
> +	struct rmi_dev_comm_data *io_params;
> +	/*
> +	 * Only one device communication request can be active at

wrap comments to 80 chars. This is around 70ish

> +	 * a time. This limitation comes from using the DOE mailbox
> +	 * at the pdev level. Requests such as get_measurements may
> +	 * span multiple mailbox messages, which must not be
> +	 * interleaved with other SPDM requests.
> +	 */
> +	struct workqueue_struct *work_queue;
> +};
> +
>  /* dsc = device security context */
>  struct cca_host_pf0_dsc {
> +	struct cca_host_comm_data comm_data;
>  	struct pci_tsm_pf0 pci;
>  	struct pci_ide *sel_stream;
>  
>  	void *rmm_pdev;
>  	int num_aux;
>  	void *aux[MAX_PDEV_AUX_GRANULES];
> +
> +	struct mutex object_lock;
> +	struct {
> +		struct cache_object *cache;
> +
> +		void *public_key;
> +		size_t public_key_size;
> +
> +		bool valid;
> +	} cert_chain;
> +	struct cache_object *vca;

There are a enough slightly non obvious things in here like
this vca that I think this structure would benefit from full kernel-doc.

>  };

Powered by blists - more mailing lists