| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e22b9d92-cd54-4094-ab72-23d80e2744fb@amd.com>
Date: Wed, 12 Nov 2025 13:05:56 +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.
Agrh. I asked the Cursor's AI to fix them all and yet it missed a bunch :)
> 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.
imho bitfields are okay until different endianness but here is always LE.
>> + * @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 am just repeating the SEV TIO spec (which is getting polished now so I'll sync up with that).
> 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.
Well, if the PSP said "need expansion" but did not say "of what", then I'd thinkg it is screwed and I do not really want to continue. May return 0 but make it WARN_ON_ONCE(!rc1 && !rc2) if it is better.
>
>> + 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?
>
>> +#define pdev_to_tsm_pf0(pdev) (((pdev)->tsm && (pdev)->tsm->dsm_dev) ? \
>> + ((struct pci_tsm_pf0 *)((pdev)->tsm->dsm_dev->tsm)) : \
>> + NULL)
>> +
>> +#define tsm_pf0_to_data(t) (&(container_of((t), struct tio_dsm, tsm)->data))
>> +
>> +static int sev_tio_spdm_cmd(struct pci_tsm_pf0 *dsm, int ret)
>> +{
>> + struct tsm_dsm_tio *dev_data = tsm_pf0_to_data(dsm);
>> + struct tsm_spdm *spdm = &dsm->spdm;
>> + struct pci_doe_mb *doe_mb;
>> +
>> + /* Check the main command handler response before entering the loop */
>> + if (ret == 0 && dev_data->psp_ret != SEV_RET_SUCCESS)
>> + return -EINVAL;
>> + else if (ret <= 0)
>> + return ret;
>
> if (ret <= 0)
> return ret;
>
> as returned already in the if path.
>
>> +
>> + /* ret > 0 means "SPDM requested" */
>> + while (ret > 0) {
>> + /* The proto can change at any point */
>> + if (ret == TSM_PROTO_CMA_SPDM) {
>> + doe_mb = dsm->doe_mb;
>> + } else if (ret == TSM_PROTO_SECURED_CMA_SPDM) {
>> + doe_mb = dsm->doe_mb_sec;
>> + } else {
>> + ret = -EFAULT;
>> + break;
> I'd be tempted to do
> else
> return -EFAULT;
> here and similar if the other error path below.
>> + }
>> +
>> + ret = pci_doe(doe_mb, PCI_VENDOR_ID_PCI_SIG, ret,
>> + spdm->req, spdm->req_len, spdm->rsp, spdm->rsp_len);
>> + if (ret < 0)
>> + break;
>> +
>> + WARN_ON_ONCE(ret == 0); /* The response should never be empty */
>> + spdm->rsp_len = ret;
>> + ret = sev_tio_continue(dev_data, &dsm->spdm);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int stream_enable(struct pci_ide *ide)
>> +{
>> + struct pci_dev *rp = pcie_find_root_port(ide->pdev);
>> + int ret;
>> +
>> + ret = pci_ide_stream_enable(rp, ide);
>
> I would suggest using a goto for the cleanup to avoid having
> an unusual code flow, but more interesting to me is why pci_ide_stream_enable()
> has side effects on failure that means we have to call pci_ide_stream_disable().
>
> Looking at Dan's patch I can see that we might have programmed things
> but then the hardware failed to set them up.
> Perhaps we should push
> cleanup into that function or at least add something to docs to point
> out that we must cleanup whether or not that function succeeds?
pci_ide_stream_enable() is called on the rootport and then the endpoint and the revert is done on the rootport, cannot push the cleanup to pci_ide_stream_enable().
>
>> + if (!ret)
>> + ret = pci_ide_stream_enable(ide->pdev, ide);
>> +
>> + if (ret)
>> + pci_ide_stream_disable(rp, ide);
>> +
>> + return ret;
>> +}
>> +
>> +static int streams_enable(struct pci_ide **ide)
>> +{
>> + int ret = 0;
>> +
>> + for (int i = 0; i < TIO_IDE_MAX_TC; ++i) {
>> + if (ide[i]) {
>> + ret = stream_enable(ide[i]);
>> + if (ret)
> Personal taste thing so up to you but I'd do return ret;
> here and return 0 at the end.
>> + break;
>> + }
>> + }
>> +
>> + return ret;
>> +}
>
>> +static u8 streams_setup(struct pci_ide **ide, u8 *ids)
>> +{
>> + bool def = false;
>> + u8 tc_mask = 0;
>> + int i;
>> +
>> + for (i = 0; i < TIO_IDE_MAX_TC; ++i) {
>> + if (!ide[i]) {
>> + ids[i] = 0xFF;
>> + continue;
>> + }
>> +
>> + tc_mask |= 1 << i;
>
> Kind of obvious either way but perhaps a slight preference for
> tc_mask |= BIT(i);
>
>> + ids[i] = ide[i]->stream_id;
>> +
>> + if (!def) {
>> + struct pci_ide_partner *settings;
>> +
>> + settings = pci_ide_to_settings(ide[i]->pdev, ide[i]);
>> + settings->default_stream = 1;
>> + def = true;
>> + }
>> +
>> + stream_setup(ide[i]);
>> + }
>> +
>> + return tc_mask;
>> +}
>> +
>> +static int streams_register(struct pci_ide **ide)
>> +{
>> + int ret = 0, i;
>> +
>> + for (i = 0; i < TIO_IDE_MAX_TC; ++i) {
>> + if (!ide[i])
>> + continue;
> Worth doing if the line is long or there is more to do here. Maybe there
> is in some follow up series, but fo rnow
> if (ide[i]) {
> ret = pci_ide_stream_register(ide[i]);
> if (ret)
> return ret;
> }
> Seems cleaner to me.
>
>> +
>> + ret = pci_ide_stream_register(ide[i]);
>> + if (ret)
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>
>> +static void stream_teardown(struct pci_ide *ide)
>> +{
>> + pci_ide_stream_teardown(ide->pdev, ide);
>> + pci_ide_stream_teardown(pcie_find_root_port(ide->pdev), ide);
>> +}
>> +
>> +static void streams_teardown(struct pci_ide **ide)
>
> Seems unbalanced to have stream_alloc take
> the struct tsm_dsm_tio * pointer to just access dev_data->ide
> but the teardown passes that directly.
>
> I'm be happier if they were both passed the same thing (either struct pci_ide **ide
> or struct tsm_dsm_tio *dev_data).
> Slight preference for struct pci_ide **ide in stream_alloc as well.
>
>> +{
>> + for (int i = 0; i < TIO_IDE_MAX_TC; ++i) {
>> + if (ide[i]) {
>> + stream_teardown(ide[i]);
>> + pci_ide_stream_free(ide[i]);
>> + ide[i] = NULL;
>> + }
>> + }
>> +}
>> +
>> +static int stream_alloc(struct pci_dev *pdev, struct tsm_dsm_tio *dev_data,
>> + unsigned int tc)
>> +{
>> + struct pci_dev *rp = pcie_find_root_port(pdev);
>> + struct pci_ide *ide;
>> +
>> + if (dev_data->ide[tc]) {
>> + pci_err(pdev, "Stream for class=%d already registered", tc);
>> + return -EBUSY;
>> + }
>> +
>> + /* FIXME: find a better way */
>> + if (nr_ide_streams != TIO_DEFAULT_NR_IDE_STREAMS)
>> + pci_notice(pdev, "Enable non-default %d streams", nr_ide_streams);
>> + pci_ide_set_nr_streams(to_pci_host_bridge(rp->bus->bridge), nr_ide_streams);
>> +
>> + ide = pci_ide_stream_alloc(pdev);
>> + if (!ide)
>> + return -EFAULT;
>> +
>> + /* Blindly assign streamid=0 to TC=0, and so on */
>> + ide->stream_id = tc;
>> +
>> + dev_data->ide[tc] = ide;
>> +
>> + return 0;
>> +}
>
>
>> +
>> +static int dsm_create(struct pci_tsm_pf0 *dsm)
>> +{
>> + struct pci_dev *pdev = dsm->base_tsm.pdev;
>> + u8 segment_id = pdev->bus ? pci_domain_nr(pdev->bus) : 0;
>> + struct pci_dev *rootport = pcie_find_root_port(pdev);
>> + u16 device_id = pci_dev_id(pdev);
>> + struct tsm_dsm_tio *dev_data = tsm_pf0_to_data(dsm);
>> + struct page *req_page;
>> + u16 root_port_id;
>> + u32 lnkcap = 0;
>> + int ret;
>> +
>> + if (pci_read_config_dword(rootport, pci_pcie_cap(rootport) + PCI_EXP_LNKCAP,
>> + &lnkcap))
>> + return -ENODEV;
>> +
>> + root_port_id = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
>> +
>> + req_page = alloc_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO);
>
> Allocate then leak a page? req_page isn't used.
>
>> + if (!req_page)
>> + return -ENOMEM;
>> +
>> + ret = sev_tio_dev_create(dev_data, device_id, root_port_id, segment_id);
>> + if (ret)
>> + goto free_resp_exit;
>> +
>> + return 0;
>> +
>> +free_resp_exit:
>> + __free_page(req_page);
>> + return ret;
>> +}
>> +
>> +static int dsm_connect(struct pci_dev *pdev)
>> +{
>> + struct pci_tsm_pf0 *dsm = pdev_to_tsm_pf0(pdev);
>> + struct tsm_dsm_tio *dev_data = tsm_pf0_to_data(dsm);
>> + u8 ids[TIO_IDE_MAX_TC];
>> + u8 tc_mask;
>> + int ret;
>> +
>> + ret = stream_alloc(pdev, dev_data, 0);
>
> As above. I'd use dev_data->ide instead of dev_data as the parameter here
> to match the other cases later. I'm guessing this parameter choice
> was something that underwent evolution and perhaps ended up less
> than ideal.
>
>> + if (ret)
>> + return ret;
>> +
>> + ret = dsm_create(dsm);
>> + if (ret)
>> + goto ide_free_exit;
>> +
>> + tc_mask = streams_setup(dev_data->ide, ids);
>> +
>> + ret = sev_tio_dev_connect(dev_data, tc_mask, ids, dsm->cert_slot, &dsm->spdm);
>> + ret = sev_tio_spdm_cmd(dsm, ret);
>> + if (ret)
>> + goto free_exit;
>> +
>> + streams_enable(dev_data->ide);
>> +
>> + ret = streams_register(dev_data->ide);
>> + if (ret)
>> + goto free_exit;
>> +
>> + return 0;
>> +
>> +free_exit:
>> + sev_tio_dev_reclaim(dev_data, &dsm->spdm);
>> +
>> + streams_disable(dev_data->ide);
>> +ide_free_exit:
>> +
>> + streams_teardown(dev_data->ide);
>> +
>> + if (ret > 0)
>> + ret = -EFAULT;
>
> My gut feeling would be that this manipulation of positive
> return codes should be pushed to where it is more obvious why it is happening.
> Either inside the functions or int the if (ret) blocks above.
> It's sufficiently unusual I'd like it to be more obvious.
>
>> + return ret;
>> +}
>> +
>> +static void dsm_disconnect(struct pci_dev *pdev)
>> +{
>> + bool force = SYSTEM_HALT <= system_state && system_state <= SYSTEM_RESTART;
>> + struct pci_tsm_pf0 *dsm = pdev_to_tsm_pf0(pdev);
>> + struct tsm_dsm_tio *dev_data = tsm_pf0_to_data(dsm);
>> + int ret;
>> +
>> + ret = sev_tio_dev_disconnect(dev_data, &dsm->spdm, force);
>> + ret = sev_tio_spdm_cmd(dsm, ret);
>> + if (ret && !force) {
>> + ret = sev_tio_dev_disconnect(dev_data, &dsm->spdm, true);
>> + sev_tio_spdm_cmd(dsm, ret);
>> + }
>> +
>> + sev_tio_dev_reclaim(dev_data, &dsm->spdm);
>> +
>> + streams_disable(dev_data->ide);
>> + streams_unregister(dev_data->ide);
>> + streams_teardown(dev_data->ide);
>> +}
>> +
>> +static struct pci_tsm_ops sev_tsm_ops = {
>> + .probe = dsm_probe,
>> + .remove = dsm_remove,
>> + .connect = dsm_connect,
>> + .disconnect = dsm_disconnect,
>> +};
>> +
>> +void sev_tsm_init_locked(struct sev_device *sev, void *tio_status_page)
>> +{
>> + struct sev_tio_status *t __free(kfree) = kzalloc(sizeof(*t), GFP_KERNEL);
> Whilst it is fine here, general advice is don't mix gotos and cleanup.h magic
> in a given function (see the comments in the header).
>
>> + struct tsm_dev *tsmdev;
>> + int ret;
>> +
>> + WARN_ON(sev->tio_status);
>> +
>> + if (!t)
>> + return;
>> +
>> + ret = sev_tio_init_locked(tio_status_page);
>> + if (ret) {
>> + pr_warn("SEV-TIO STATUS failed with %d\n", ret);
>> + goto error_exit;
>> + }
>> +
>> + tsmdev = tsm_register(sev->dev, &sev_tsm_ops);
>> + if (IS_ERR(tsmdev))
>> + goto error_exit;
>> +
>> + memcpy(t, tio_status_page, sizeof(*t));
>
> If it weren't for the goto then this could be
>
> struct sev_tio_status *t __free(kfree) =
> kmemdup(tio_status_page, sizeof(*t), GFP_KERNEL;
> if (!t)
> return;
>
>
>> +
>> + pr_notice("SEV-TIO status: EN=%d INIT_DONE=%d rq=%d..%d rs=%d..%d "
>> + "scr=%d..%d out=%d..%d dev=%d tdi=%d algos=%x\n",
>> + t->tio_en, t->tio_init_done,
>> + t->spdm_req_size_min, t->spdm_req_size_max,
>> + t->spdm_rsp_size_min, t->spdm_rsp_size_max,
>> + t->spdm_scratch_size_min, t->spdm_scratch_size_max,
>> + t->spdm_out_size_min, t->spdm_out_size_max,
>> + t->devctx_size, t->tdictx_size,
>> + t->tio_crypto_alg);
>> +
>> + sev->tsmdev = tsmdev;
>> + sev->tio_status = no_free_ptr(t);
>> +
>> + return;
>> +
>> +error_exit:
>> + pr_err("Failed to enable SEV-TIO: ret=%d en=%d initdone=%d SEV=%d\n",
>> + ret, t->tio_en, t->tio_init_done,
>> + boot_cpu_has(X86_FEATURE_SEV));
>> + pr_err("Check BIOS for: SMEE, SEV Control, SEV-ES ASID Space Limit=99,\n"
>> + "SNP Memory (RMP Table) Coverage, RMP Coverage for 64Bit MMIO Ranges\n"
>> + "SEV-SNP Support, SEV-TIO Support, PCIE IDE Capability\n");
>
> Superficially feels to me that some of this set of helpful messages is only relevant
> to some error paths - maybe just print the most relevant parts where those problems
> are detected?
Well, really only "SNP Memory (RMP Table) Coverage" will make SNP_INIT_EX fail (and it is off by default) but for SEV-TIO to work - all these need to be enabled so I mention them all here. I guess I'll remove the whole "Check BIOS" thing, the defaults have changed now.
Thanks for the review, I'll fix what I have not commented on.
>> +}
>> +
>> +void sev_tsm_uninit(struct sev_device *sev)
>> +{
>> + if (sev->tsmdev)
>> + tsm_unregister(sev->tsmdev);
>> +
>> + sev->tsmdev = NULL;
>> +}
>
>
--
Alexey
Powered by blists - more mailing lists