Message-ID: <442d4c48-8d6a-4129-b21a-cc2de4f0fcd0@amd.com>
Date: Mon, 17 Nov 2025 12:32:57 +1100
From: Alexey Kardashevskiy <aik@....com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
 linux-pci@...r.kernel.org, Tom Lendacky <thomas.lendacky@....com>,
 John Allen <john.allen@....com>, Herbert Xu <herbert@...dor.apana.org.au>,
 "David S. Miller" <davem@...emloft.net>, Ashish Kalra
 <ashish.kalra@....com>, Joerg Roedel <joro@...tes.org>,
 Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
 Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
 Dan Williams <dan.j.williams@...el.com>, Bjorn Helgaas
 <bhelgaas@...gle.com>, Eric Biggers <ebiggers@...gle.com>,
 Brijesh Singh <brijesh.singh@....com>, Gary R Hook <gary.hook@....com>,
 "Borislav Petkov (AMD)" <bp@...en8.de>, Kim Phillips <kim.phillips@....com>,
 Vasant Hegde <vasant.hegde@....com>, Jason Gunthorpe <jgg@...pe.ca>,
 Michael Roth <michael.roth@....com>, Xu Yilun <yilun.xu@...ux.intel.com>,
 Gao Shiyuan <gaoshiyuan@...du.com>, Sean Christopherson <seanjc@...gle.com>,
 Nikunj A Dadhania <nikunj@....com>, Dionna Glaze <dionnaglaze@...gle.com>,
 iommu@...ts.linux.dev, linux-coco@...ts.linux.dev
Subject: Re: [PATCH kernel 6/6] crypto/ccp: Implement SEV-TIO PCIe IDE
 (phase1)



On 11/11/25 22:47, Jonathan Cameron wrote:
> On Tue, 11 Nov 2025 17:38:18 +1100
> Alexey Kardashevskiy <aik@....com> wrote:
> 
>> Implement the SEV-TIO (Trusted I/O) firmware interface for PCIe TDISP
>> (Trust Domain In-Socket Protocol). This enables secure communication
>> between trusted domains and PCIe devices through the PSP (Platform
>> Security Processor).
>>
>> The implementation includes:
>> - Device Security Manager (DSM) operations for establishing secure links
>> - SPDM (Security Protocol and Data Model) over DOE (Data Object Exchange)
>> - IDE (Integrity Data Encryption) stream management for secure PCIe
>>
>> This module bridges the SEV firmware stack with the generic PCIe TSM
>> framework.
>>
>> This is phase1 as described in Documentation/driver-api/pci/tsm.rst.
>>
>> On AMD SEV, the AMD PSP firmware acts as TSM (manages the security/trust).
>> The CCP driver provides the interface to it and registers in the TSM
>> subsystem.
>>
>> Implement SEV TIO PSP command wrappers in sev-dev-tio.c and store
>> the data in the SEV-TIO-specific structs.
>>
>> Implement TSM hooks and IDE setup in sev-dev-tsm.c.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@....com>
> Hi Alexey,
> 
> Various minor comments inline.
> 
> Thanks,
> 
> Jonathan
> 
>> diff --git a/drivers/crypto/ccp/sev-dev-tio.h b/drivers/crypto/ccp/sev-dev-tio.h
>> new file mode 100644
>> index 000000000000..c72ac38d4351
>> --- /dev/null
>> +++ b/drivers/crypto/ccp/sev-dev-tio.h
> 
>> diff --git a/drivers/crypto/ccp/sev-dev-tio.c b/drivers/crypto/ccp/sev-dev-tio.c
>> new file mode 100644
>> index 000000000000..ca0db6e64839
>> --- /dev/null
>> +++ b/drivers/crypto/ccp/sev-dev-tio.c
> 
>> +static size_t sla_dobj_id_to_size(u8 id)
>> +{
>> +	size_t n;
>> +
>> +	BUILD_BUG_ON(sizeof(struct spdm_dobj_hdr_resp) != 0x10);
>> +	switch (id) {
>> +	case SPDM_DOBJ_ID_REQ:
>> +		n = sizeof(struct spdm_dobj_hdr_req);
>> +		break;
>> +	case SPDM_DOBJ_ID_RESP:
>> +		n = sizeof(struct spdm_dobj_hdr_resp);
>> +		break;
>> +	default:
>> +		WARN_ON(1);
>> +		n = 0;
>> +		break;
>> +	}
>> +
>> +	return n;
> 
> Maybe just returning above would be simpler and remove need for local variables
> etc.
> 
>> +}
>>
>> + * struct sev_data_tio_dev_connect - TIO_DEV_CONNECT
>> + *
>> + * @length: Length in bytes of this command buffer.
>> + * @spdm_ctrl: SPDM control structure defined in Section 5.1.
>> + * @device_id: The PCIe Routing Identifier of the device to connect to.
>> + * @root_port_id: The PCIe Routing Identifier of the root port of the device
> 
> A few of these aren't present in the structure so out of date docs I think
> 
>> + * @segment_id: The PCIe Segment Identifier of the device to connect to.
>> + * @dev_ctx_sla: Scatter list address of the device context buffer.
>> + * @tc_mask: Bitmask of the traffic classes to initialize for SEV-TIO usage.
>> + *           Setting the kth bit of the TC_MASK to 1 indicates that the traffic
>> + *           class k will be initialized.
>> + * @cert_slot: Slot number of the certificate requested for constructing the SPDM session.
>> + * @ide_stream_id: IDE stream IDs to be associated with this device.
>> + *                 Valid only if corresponding bit in TC_MASK is set.
>> + */
>> +struct sev_data_tio_dev_connect {
>> +	u32 length;
>> +	u32 reserved1;
>> +	struct spdm_ctrl spdm_ctrl;
>> +	u8 reserved2[8];
>> +	struct sla_addr_t dev_ctx_sla;
>> +	u8 tc_mask;
>> +	u8 cert_slot;
>> +	u8 reserved3[6];
>> +	u8 ide_stream_id[8];
>> +	u8 reserved4[8];
>> +} __packed;
>> +
>> +/**
>> + * struct sev_data_tio_dev_disconnect - TIO_DEV_DISCONNECT
>> + *
>> + * @length: Length in bytes of this command buffer.
>> + * @force: Force device disconnect without SPDM traffic.
>> + * @spdm_ctrl: SPDM control structure defined in Section 5.1.
>> + * @dev_ctx_sla: Scatter list address of the device context buffer.
>> + */
>> +struct sev_data_tio_dev_disconnect {
>> +	u32 length;
>> +	union {
>> +		u32 flags;
>> +		struct {
>> +			u32 force:1;
>> +		};
>> +	};
>> +	struct spdm_ctrl spdm_ctrl;
>> +	struct sla_addr_t dev_ctx_sla;
>> +} __packed;
>> +
>> +/**
>> + * struct sev_data_tio_dev_meas - TIO_DEV_MEASUREMENTS
>> + *
>> + * @length: Length in bytes of this command buffer
> 
> flags need documentation as well.
> Generally I'd avoid bitfields and rely on defines so we don't hit
> the weird stuff the c spec allows to be done with bitfields.
> 
>> + * @raw_bitstream: 0: Requests the digest form of the attestation report
>> + *                 1: Requests the raw bitstream form of the attestation report
>> + * @spdm_ctrl: SPDM control structure defined in Section 5.1.
>> + * @dev_ctx_sla: Scatter list address of the device context buffer.
>> + */
>> +struct sev_data_tio_dev_meas {
>> +	u32 length;
>> +	union {
>> +		u32 flags;
>> +		struct {
>> +			u32 raw_bitstream:1;
>> +		};
>> +	};
>> +	struct spdm_ctrl spdm_ctrl;
>> +	struct sla_addr_t dev_ctx_sla;
>> +	u8 meas_nonce[32];
>> +} __packed;
> 
>> +/**
>> + * struct sev_data_tio_dev_reclaim - TIO_DEV_RECLAIM command
>> + *
>> + * @length: Length in bytes of this command buffer
>> + * @dev_ctx_paddr: SPA of page donated by hypervisor
>> + */
>> +struct sev_data_tio_dev_reclaim {
>> +	u32 length;
>> +	u32 reserved;
> Why a u32 for this reserved, but u8[] arrays for some of thos in
> other structures like sev_data_tio_asid_fence_status.
> I'd aim for consistency on that. I don't really mind which option!
>> +	struct sla_addr_t dev_ctx_sla;
>> +} __packed;
>> +
>> +/**
>> + * struct sev_data_tio_asid_fence_clear - TIO_ASID_FENCE_CLEAR command
>> + *
>> + * @length: Length in bytes of this command buffer
>> + * @dev_ctx_paddr: Scatter list address of device context
>> + * @gctx_paddr: System physical address of guest context page
> 
> As below wrt to complete docs.
> 
>> + *
>> + * This command clears the ASID fence for a TDI.
>> + */
>> +struct sev_data_tio_asid_fence_clear {
>> +	u32 length;				/* In */
>> +	u32 reserved1;
>> +	struct sla_addr_t dev_ctx_paddr;	/* In */
>> +	u64 gctx_paddr;			/* In */
>> +	u8 reserved2[8];
>> +} __packed;
>> +
>> +/**
>> + * struct sev_data_tio_asid_fence_status - TIO_ASID_FENCE_STATUS command
>> + *
>> + * @length: Length in bytes of this command buffer
> Kernel-doc complains about undocument structure elements, so you probably have
> to add a trivial comment for the two reserved ones to keep it happy.
> 
>> + * @dev_ctx_paddr: Scatter list address of device context
>> + * @asid: Address Space Identifier to query
>> + * @status_pa: System physical address where fence status will be written
>> + *
>> + * This command queries the fence status for a specific ASID.
>> + */
>> +struct sev_data_tio_asid_fence_status {
>> +	u32 length;				/* In */
>> +	u8 reserved1[4];
>> +	struct sla_addr_t dev_ctx_paddr;	/* In */
>> +	u32 asid;				/* In */
>> +	u64 status_pa;
>> +	u8 reserved2[4];
>> +} __packed;
>> +
>> +static struct sla_buffer_hdr *sla_buffer_map(struct sla_addr_t sla)
>> +{
>> +	struct sla_buffer_hdr *buf;
>> +
>> +	BUILD_BUG_ON(sizeof(struct sla_buffer_hdr) != 0x40);
>> +	if (IS_SLA_NULL(sla))
>> +		return NULL;
>> +
>> +	if (sla.page_type == SLA_PAGE_TYPE_SCATTER) {
>> +		struct sla_addr_t *scatter = sla_to_va(sla);
>> +		unsigned int i, npages = 0;
>> +		struct page **pp;
>> +
>> +		for (i = 0; i < SLA_SCATTER_LEN(sla); ++i) {
>> +			if (WARN_ON_ONCE(SLA_SZ(scatter[i]) > SZ_4K))
>> +				return NULL;
>> +
>> +			if (WARN_ON_ONCE(scatter[i].page_type == SLA_PAGE_TYPE_SCATTER))
>> +				return NULL;
>> +
>> +			if (IS_SLA_EOL(scatter[i])) {
>> +				npages = i;
>> +				break;
>> +			}
>> +		}
>> +		if (WARN_ON_ONCE(!npages))
>> +			return NULL;
>> +
>> +		pp = kmalloc_array(npages, sizeof(pp[0]), GFP_KERNEL);
> 
> Could use a __free to avoid needing to manually clean this up.
> 
>> +		if (!pp)
>> +			return NULL;
>> +
>> +		for (i = 0; i < npages; ++i)
>> +			pp[i] = sla_to_page(scatter[i]);
>> +
>> +		buf = vm_map_ram(pp, npages, 0);
>> +		kfree(pp);
>> +	} else {
>> +		struct page *pg = sla_to_page(sla);
>> +
>> +		buf = vm_map_ram(&pg, 1, 0);
>> +	}
>> +
>> +	return buf;
>> +}
> 
>> +
>> +/* Expands a buffer, only firmware owned buffers allowed for now */
>> +static int sla_expand(struct sla_addr_t *sla, size_t *len)
>> +{
>> +	struct sla_buffer_hdr *oldbuf = sla_buffer_map(*sla), *newbuf;
>> +	struct sla_addr_t oldsla = *sla, newsla;
>> +	size_t oldlen = *len, newlen;
>> +
>> +	if (!oldbuf)
>> +		return -EFAULT;
>> +
>> +	newlen = oldbuf->capacity_sz;
>> +	if (oldbuf->capacity_sz == oldlen) {
>> +		/* This buffer does not require expansion, must be another buffer */
>> +		sla_buffer_unmap(oldsla, oldbuf);
>> +		return 1;
>> +	}
>> +
>> +	pr_notice("Expanding BUFFER from %ld to %ld bytes\n", oldlen, newlen);
>> +
>> +	newsla = sla_alloc(newlen, true);
>> +	if (IS_SLA_NULL(newsla))
>> +		return -ENOMEM;
>> +
>> +	newbuf = sla_buffer_map(newsla);
>> +	if (!newbuf) {
>> +		sla_free(newsla, newlen, true);
>> +		return -EFAULT;
>> +	}
>> +
>> +	memcpy(newbuf, oldbuf, oldlen);
>> +
>> +	sla_buffer_unmap(newsla, newbuf);
>> +	sla_free(oldsla, oldlen, true);
>> +	*sla = newsla;
>> +	*len = newlen;
>> +
>> +	return 0;
>> +}
>> +
>> +static int sev_tio_do_cmd(int cmd, void *data, size_t data_len, int *psp_ret,
>> +			  struct tsm_dsm_tio *dev_data, struct tsm_spdm *spdm)
>> +{
>> +	int rc;
>> +
>> +	*psp_ret = 0;
>> +	rc = sev_do_cmd(cmd, data, psp_ret);
>> +
>> +	if (WARN_ON(!spdm && !rc && *psp_ret == SEV_RET_SPDM_REQUEST))
>> +		return -EIO;
>> +
>> +	if (rc == 0 && *psp_ret == SEV_RET_EXPAND_BUFFER_LENGTH_REQUEST) {
>> +		int rc1, rc2;
>> +
>> +		rc1 = sla_expand(&dev_data->output, &dev_data->output_len);
>> +		if (rc1 < 0)
>> +			return rc1;
>> +
>> +		rc2 = sla_expand(&dev_data->scratch, &dev_data->scratch_len);
>> +		if (rc2 < 0)
>> +			return rc2;
>> +
>> +		if (!rc1 && !rc2)
>> +			/* Neither buffer requires expansion, this is wrong */
> 
> Is this check correct?  I think you return 1 on no need to expand.
> 
>> +			return -EFAULT;
>> +
>> +		*psp_ret = 0;
>> +		rc = sev_do_cmd(cmd, data, psp_ret);
>> +	}
>> +
>> +	if (spdm && (rc == 0 || rc == -EIO) && *psp_ret == SEV_RET_SPDM_REQUEST) {
>> +		struct spdm_dobj_hdr_resp *resp_hdr;
>> +		struct spdm_dobj_hdr_req *req_hdr;
>> +		struct sev_tio_status *tio_status = to_tio_status(dev_data);
>> +		size_t resp_len = tio_status->spdm_req_size_max -
>> +			(sla_dobj_id_to_size(SPDM_DOBJ_ID_RESP) + sizeof(struct sla_buffer_hdr));
>> +
>> +		if (!dev_data->cmd) {
>> +			if (WARN_ON_ONCE(!data_len || (data_len != *(u32 *) data)))
>> +				return -EINVAL;
>> +			if (WARN_ON(data_len > sizeof(dev_data->cmd_data)))
>> +				return -EFAULT;
>> +			memcpy(dev_data->cmd_data, data, data_len);
>> +			memset(&dev_data->cmd_data[data_len], 0xFF,
>> +			       sizeof(dev_data->cmd_data) - data_len);
>> +			dev_data->cmd = cmd;
>> +		}
>> +
>> +		req_hdr = sla_to_dobj_req_hdr(dev_data->reqbuf);
>> +		resp_hdr = sla_to_dobj_resp_hdr(dev_data->respbuf);
>> +		switch (req_hdr->data_type) {
>> +		case DOBJ_DATA_TYPE_SPDM:
>> +			rc = TSM_PROTO_CMA_SPDM;
>> +			break;
>> +		case DOBJ_DATA_TYPE_SECURE_SPDM:
>> +			rc = TSM_PROTO_SECURED_CMA_SPDM;
>> +			break;
>> +		default:
>> +			rc = -EINVAL;
>> +			return rc;
> 
> 			return -EINVAL;
> 
>> +		}
>> +		resp_hdr->data_type = req_hdr->data_type;
>> +		spdm->req_len = req_hdr->hdr.length - sla_dobj_id_to_size(SPDM_DOBJ_ID_REQ);
>> +		spdm->rsp_len = resp_len;
>> +	} else if (dev_data && dev_data->cmd) {
>> +		/* For either error or success just stop the bouncing */
>> +		memset(dev_data->cmd_data, 0, sizeof(dev_data->cmd_data));
>> +		dev_data->cmd = 0;
>> +	}
>> +
>> +	return rc;
>> +}
>> +
> 
>> +static int spdm_ctrl_init(struct tsm_spdm *spdm, struct spdm_ctrl *ctrl,
>> +			  struct tsm_dsm_tio *dev_data)
> 
> This seems like a slightly odd function as it is initializing two different
> things.  To me I'd read this as only initializing the spdm_ctrl structure.
> Perhaps split, or rename?
> 
>> +{
>> +	ctrl->req = dev_data->req;
>> +	ctrl->resp = dev_data->resp;
>> +	ctrl->scratch = dev_data->scratch;
>> +	ctrl->output = dev_data->output;
>> +
>> +	spdm->req = sla_to_data(dev_data->reqbuf, SPDM_DOBJ_ID_REQ);
>> +	spdm->rsp = sla_to_data(dev_data->respbuf, SPDM_DOBJ_ID_RESP);
>> +	if (!spdm->req || !spdm->rsp)
>> +		return -EFAULT;
>> +
>> +	return 0;
>> +}
> 
>> +
>> +int sev_tio_init_locked(void *tio_status_page)
>> +{
>> +	struct sev_tio_status *tio_status = tio_status_page;
>> +	struct sev_data_tio_status data_status = {
>> +		.length = sizeof(data_status),
>> +	};
>> +	int ret = 0, psp_ret = 0;
> 
> ret always set before use so don't initialize.
> 
>> +
>> +	data_status.status_paddr = __psp_pa(tio_status_page);
>> +	ret = __sev_do_cmd_locked(SEV_CMD_TIO_STATUS, &data_status, &psp_ret);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (tio_status->length < offsetofend(struct sev_tio_status, tdictx_size) ||
>> +	    tio_status->flags & 0xFFFFFF00)
>> +		return -EFAULT;
>> +
>> +	if (!tio_status->tio_en && !tio_status->tio_init_done)
>> +		return -ENOENT;
>> +
>> +	if (tio_status->tio_init_done)
>> +		return -EBUSY;
>> +
>> +	struct sev_data_tio_init ti = { .length = sizeof(ti) };
>> +
>> +	ret = __sev_do_cmd_locked(SEV_CMD_TIO_INIT, &ti, &psp_ret);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = __sev_do_cmd_locked(SEV_CMD_TIO_STATUS, &data_status, &psp_ret);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> 	return __sev_do_cmd_locked() perhaps.
> 
>> +}
>> +
>> +int sev_tio_dev_create(struct tsm_dsm_tio *dev_data, u16 device_id,
>> +		       u16 root_port_id, u8 segment_id)
>> +{
>> +	struct sev_tio_status *tio_status = to_tio_status(dev_data);
>> +	struct sev_data_tio_dev_create create = {
>> +		.length = sizeof(create),
>> +		.device_id = device_id,
>> +		.root_port_id = root_port_id,
>> +		.segment_id = segment_id,
>> +	};
>> +	void *data_pg;
>> +	int ret;
>> +
>> +	dev_data->dev_ctx = sla_alloc(tio_status->devctx_size, true);
>> +	if (IS_SLA_NULL(dev_data->dev_ctx))
>> +		return -ENOMEM;
>> +
>> +	data_pg = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
>> +	if (!data_pg) {
>> +		ret = -ENOMEM;
>> +		goto free_ctx_exit;
>> +	}
>> +
>> +	create.dev_ctx_sla = dev_data->dev_ctx;
>> +	ret = sev_tio_do_cmd(SEV_CMD_TIO_DEV_CREATE, &create, sizeof(create),
>> +			     &dev_data->psp_ret, dev_data, NULL);
>> +	if (ret)
>> +		goto free_data_pg_exit;
>> +
>> +	dev_data->data_pg = data_pg;
>> +
>> +	return ret;
> 
> return 0;  Might as well make it clear this is always a good path.
> 
>> +
>> +free_data_pg_exit:
>> +	snp_free_firmware_page(data_pg);
>> +free_ctx_exit:
>> +	sla_free(create.dev_ctx_sla, tio_status->devctx_size, true);
>> +	return ret;
>> +}
> 
>> +
>> +int sev_tio_dev_connect(struct tsm_dsm_tio *dev_data, u8 tc_mask, u8 ids[8], u8 cert_slot,
>> +			struct tsm_spdm *spdm)
>> +{
>> +	struct sev_data_tio_dev_connect connect = {
>> +		.length = sizeof(connect),
>> +		.tc_mask = tc_mask,
>> +		.cert_slot = cert_slot,
>> +		.dev_ctx_sla = dev_data->dev_ctx,
>> +		.ide_stream_id = {
>> +			ids[0], ids[1], ids[2], ids[3],
>> +			ids[4], ids[5], ids[6], ids[7]
>> +		},
>> +	};
>> +	int ret;
>> +
>> +	if (WARN_ON(IS_SLA_NULL(dev_data->dev_ctx)))
>> +		return -EFAULT;
>> +	if (!(tc_mask & 1))
>> +		return -EINVAL;
>> +
>> +	ret = spdm_ctrl_alloc(dev_data, spdm);
>> +	if (ret)
>> +		return ret;
>> +	ret = spdm_ctrl_init(spdm, &connect.spdm_ctrl, dev_data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = sev_tio_do_cmd(SEV_CMD_TIO_DEV_CONNECT, &connect, sizeof(connect),
>> +			     &dev_data->psp_ret, dev_data, spdm);
>> +
>> +	return ret;
> 
> return sev_tio_do_cmd...
> 
>> +}
>> +
>> +int sev_tio_dev_disconnect(struct tsm_dsm_tio *dev_data, struct tsm_spdm *spdm, bool force)
>> +{
>> +	struct sev_data_tio_dev_disconnect dc = {
>> +		.length = sizeof(dc),
>> +		.dev_ctx_sla = dev_data->dev_ctx,
>> +		.force = force,
>> +	};
>> +	int ret;
>> +
>> +	if (WARN_ON_ONCE(IS_SLA_NULL(dev_data->dev_ctx)))
>> +		return -EFAULT;
>> +
>> +	ret = sspdm_ctrl_initpdm_ctrl_init(spdm, &dc.spdm_ctrl, dev_data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = sev_tio_do_cmd(SEV_CMD_TIO_DEV_DISCONNECT, &dc, sizeof(dc),
>> +			     &dev_data->psp_ret, dev_data, spdm);
>> +
>> +	return ret;
> 
> return sev_tio..
> 
>> +}
>> +
>> +int sev_tio_asid_fence_clear(struct sla_addr_t dev_ctx, u64 gctx_paddr, int *psp_ret)
>> +{
>> +	struct sev_data_tio_asid_fence_clear c = {
>> +		.length = sizeof(c),
>> +		.dev_ctx_paddr = dev_ctx,
>> +		.gctx_paddr = gctx_paddr,
>> +	};
>> +
>> +	return sev_do_cmd(SEV_CMD_TIO_ASID_FENCE_CLEAR, &c, psp_ret);
>> +}
>> +
>> +int sev_tio_asid_fence_status(struct tsm_dsm_tio *dev_data, u16 device_id, u8 segment_id,
>> +			      u32 asid, bool *fenced)
> The meaning of return codes in these functions is a mix of errno and SEV specific.
> Perhaps some documentation to make that clear, or even a typedef for the SEV ones?
>> +{
>> +	u64 *status = prep_data_pg(u64, dev_data);
>> +	struct sev_data_tio_asid_fence_status s = {
>> +		.length = sizeof(s),
>> +		.dev_ctx_paddr = dev_data->dev_ctx,
>> +		.asid = asid,
>> +		.status_pa = __psp_pa(status),
>> +	};
>> +	int ret;
>> +
>> +	ret = sev_do_cmd(SEV_CMD_TIO_ASID_FENCE_STATUS, &s, &dev_data->psp_ret);
>> +
>> +	if (ret == SEV_RET_SUCCESS) {
> 
> I guess this gets more complex in future series to mean that checking
> the opposite isn't the way to go?
> 	if (ret != SEV_RET_SUCCESS)
> 		return ret;
> 
> If not I'd do that to reduce indent and have a nice quick exit
> for errors.
> 
>> +		u8 dma_status = *status & 0x3;
>> +		u8 mmio_status = (*status >> 2) & 0x3;
>> +
>> +		switch (dma_status) {
>> +		case 0:
> These feel like magic numbers that could perhaps have defines to give
> them pretty names?
>> +			*fenced = false;
>> +			break;
>> +		case 1:
>> +		case 3:
>> +			*fenced = true;
>> +			break;
>> +		default:
>> +			pr_err("%04x:%x:%x.%d: undefined DMA fence state %#llx\n",
>> +			       segment_id, PCI_BUS_NUM(device_id),
>> +			       PCI_SLOT(device_id), PCI_FUNC(device_id), *status);
>> +			*fenced = true;
>> +			break;
>> +		}
>> +
>> +		switch (mmio_status) {
>> +		case 0:
> Same as above, names might be nice.
>> +			*fenced = false;
>> +			break;
>> +		case 3:
>> +			*fenced = true;
>> +			break;
>> +		default:
>> +			pr_err("%04x:%x:%x.%d: undefined MMIO fence state %#llx\n",
>> +			       segment_id, PCI_BUS_NUM(device_id),
>> +			       PCI_SLOT(device_id), PCI_FUNC(device_id), *status);
>> +			*fenced = true;
>> +			break;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
> 
>> diff --git a/drivers/crypto/ccp/sev-dev-tsm.c b/drivers/crypto/ccp/sev-dev-tsm.c
>> new file mode 100644
>> index 000000000000..4702139185a2
>> --- /dev/null
>> +++ b/drivers/crypto/ccp/sev-dev-tsm.c
> 
>> +
>> +static uint nr_ide_streams = TIO_DEFAULT_NR_IDE_STREAMS;
>> +module_param_named(ide_nr, nr_ide_streams, uint, 0644);
>> +MODULE_PARM_DESC(ide_nr, "Set the maximum number of IDE streams per PHB");
>> +
>> +#define dev_to_sp(dev)		((struct sp_device *)dev_get_drvdata(dev))
>> +#define dev_to_psp(dev)		((struct psp_device *)(dev_to_sp(dev)->psp_data))
>> +#define dev_to_sev(dev)		((struct sev_device *)(dev_to_psp(dev)->sev_data))
>> +#define tsm_dev_to_sev(tsmdev)	dev_to_sev((tsmdev)->dev.parent)
>> +#define tsm_pf0_to_sev(t)	tsm_dev_to_sev((t)->base.owner)
>> +
>> +/*to_pci_tsm_pf0((pdev)->tsm)*/
> 
> Left over of something?

Actually not, to_pci_tsm_pf0() is a static helper in drivers/pci/tsm.c and pdev_to_tsm_pf0() (below) is the same thing defined for drivers/crypto/ccp/sev-dev-tsm.c and I wonder if to_pci_tsm_pf0() is better be exported. pdev_to_tsm_pf0() does not need all the checks as if we are that far past the initial setup, we can skip on some checks which to_pci_tsm_pf0() performs so I have not exported to_pci_tsm_pf0() but left the comment. Thanks,

> 
>> +#define pdev_to_tsm_pf0(pdev)	(((pdev)->tsm && (pdev)->tsm->dsm_dev) ? \
>> +				((struct pci_tsm_pf0 *)((pdev)->tsm->dsm_dev->tsm)) : \
>> +				NULL)
>> +

-- 
Alexey

Powered by blists - more mailing lists