Re: [RFC PATCH 08/21] crypto/ccp: Implement SEV TIO firmware interface

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 23 Aug 2024 23:21:22 +1000
Alexey Kardashevskiy <aik@amd.com> wrote:

> Implement SEV TIO PSP command wrappers in sev-dev-tio.c, these make
> SPDM calls and store the data in the SEV-TIO-specific structs.
> 
> Implement tsm_ops for the hypervisor, the TSM module will call these
> when loaded on the host and its tsm_set_ops() is called. The HV ops
> are implemented in sev-dev-tsm.c.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
Superficial comments online inline.

Jonathan

> ---
>  drivers/crypto/ccp/Makefile      |    2 +
>  arch/x86/include/asm/sev.h       |   20 +
>  drivers/crypto/ccp/sev-dev-tio.h |  105 ++
>  drivers/crypto/ccp/sev-dev.h     |    2 +
>  include/linux/psp-sev.h          |   60 +
>  drivers/crypto/ccp/sev-dev-tio.c | 1565 ++++++++++++++++++++
>  drivers/crypto/ccp/sev-dev-tsm.c |  397 +++++
>  drivers/crypto/ccp/sev-dev.c     |   10 +-
>  8 files changed, 2159 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/Makefile b/drivers/crypto/ccp/Makefile
> index 394484929dae..d9871465dd08 100644
> --- a/drivers/crypto/ccp/Makefile
> +++ b/drivers/crypto/ccp/Makefile
> @@ -11,6 +11,8 @@ ccp-$(CONFIG_PCI) += sp-pci.o
>  ccp-$(CONFIG_CRYPTO_DEV_SP_PSP) += psp-dev.o \
>                                     sev-dev.o \
>                                     tee-dev.o \
> +				   sev-dev-tio.o \
> +				   sev-dev-tsm.o \
spaces vs tabs. I guess go for consistency.

>                                     platform-access.o \
>                                     dbc.o \
>                                     hsti.o

> diff --git a/drivers/crypto/ccp/sev-dev-tio.c b/drivers/crypto/ccp/sev-dev-tio.c
> new file mode 100644
> index 000000000000..42741b17c747
> --- /dev/null
> +++ b/drivers/crypto/ccp/sev-dev-tio.c
> @@ -0,0 +1,1565 @@
> +// SPDX-License-Identifier: GPL-2.0-only

> +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;
> +	case SPDM_DOBJ_ID_CERTIFICATE:
> +		n = sizeof(struct spdm_dobj_hdr_cert);
> +		break;
> +	case SPDM_DOBJ_ID_MEASUREMENT:
> +		n = sizeof(struct spdm_dobj_hdr_meas);
> +		break;
> +	case SPDM_DOBJ_ID_REPORT:
> +		n = sizeof(struct spdm_dobj_hdr_report);
> +		break;
> +	default:
> +		WARN_ON(1);
> +		n = 0;
> +		break;
> +	}
> +
> +	return n;

Early returns will make this more readable.

> +}


> +/**
> + * 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.
> + * @segment_id: The PCIe Segment Identifier of the device to connect to.

Doesn't seem to be there.

> + * @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_guest_request - TIO_GUEST_REQUEST command
> + *
> + * @length: Length in bytes of this command buffer
> + * @spdm_ctrl: SPDM control structure defined in Chapter 2.

Some more fields that aren't documented.  They all should be for
kernel-doc or the scripts will moan. 
I'd just run the script and fixup all the warnings and errors.


> + * @gctx_paddr: system physical address of guest context page
> + * @tdi_ctx_paddr: SPA of page donated by hypervisor
> + * @req_paddr: system physical address of request page
> + * @res_paddr: system physical address of response page
> + */
> +struct sev_data_tio_guest_request {
> +	u32 length;				/* In */
> +	u32 reserved;
> +	struct spdm_ctrl spdm_ctrl;		/* In */
> +	struct sla_addr_t dev_ctx_sla;
> +	struct sla_addr_t tdi_ctx_sla;
> +	u64 gctx_paddr;
> +	u64 req_paddr;				/* In */
> +	u64 res_paddr;				/* In */
> +} __packed;
> +



> +int sev_tio_tdi_status(struct tsm_dev_tio *dev_data, struct tsm_tdi_tio *tdi_data,
> +		       struct tsm_spdm *spdm)
> +{
> +	struct sev_tio_tdi_status_data *data = (struct sev_tio_tdi_status_data *) dev_data->data;
> +	struct sev_data_tio_tdi_status status = {
> +		.length = sizeof(status),
> +		.dev_ctx_sla = dev_data->dev_ctx,
> +		.tdi_ctx_sla = tdi_data->tdi_ctx,
> +	};
> +	int ret;
> +
> +	if (IS_SLA_NULL(dev_data->dev_ctx) || IS_SLA_NULL(tdi_data->tdi_ctx))
> +		return -ENXIO;
> +
> +	memset(data, 0, sizeof(*data));
> +
> +	spdm_ctrl_init(spdm, &status.spdm_ctrl, dev_data);
> +	status.status_paddr = __psp_pa(data);
> +
> +	ret = sev_tio_do_cmd(SEV_CMD_TIO_TDI_STATUS, &status, sizeof(status),
> +			     &dev_data->psp_ret, dev_data, spdm);
> +
> +	return ret;

return sev_tio_do_cmd()

Same in other similar cases.

> +}


> diff --git a/drivers/crypto/ccp/sev-dev-tsm.c b/drivers/crypto/ccp/sev-dev-tsm.c
> new file mode 100644
> index 000000000000..a11dea482d4b
> --- /dev/null
> +++ b/drivers/crypto/ccp/sev-dev-tsm.c
> @@ -0,0 +1,397 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +// Interface to CCP/SEV-TIO for generic PCIe TDISP module
> +
> +#include <linux/pci.h>
> +#include <linux/pci-doe.h>
> +#include <linux/tsm.h>
> +
> +#include <linux/smp.h>
> +#include <asm/sev-common.h>
> +
> +#include "psp-dev.h"
> +#include "sev-dev.h"
> +#include "sev-dev-tio.h"
> +
> +static int mkret(int ret, struct tsm_dev_tio *dev_data)
> +{
> +	if (ret)
> +		return ret;
> +
> +	if (dev_data->psp_ret == SEV_RET_SUCCESS)
> +		return 0;
> +
> +	pr_err("PSP returned an error %d\n", dev_data->psp_ret);
I'm not totally convinced this is worth while vs simply checking
at call sites.

> +	return -EINVAL;
> +}
> +
> +static int dev_connect(struct tsm_dev *tdev, void *private_data)
> +{
> +	u16 device_id = pci_dev_id(tdev->pdev);
> +	u16 root_port_id = 0; // FIXME: this is NOT PCI id, need to figure out how to calculate this
> +	u8 segment_id = tdev->pdev->bus ? pci_domain_nr(tdev->pdev->bus) : 0;
> +	struct tsm_dev_tio *dev_data = tdev->data;
> +	int ret;
> +
> +	if (!dev_data) {
> +		dev_data = kzalloc(sizeof(*dev_data), GFP_KERNEL);
> +		if (!dev_data)
> +			return -ENOMEM;
> +
> +		ret = sev_tio_dev_create(dev_data, device_id, root_port_id, segment_id);
> +		if (ret)
> +			goto free_exit;
> +
> +		tdev->data = dev_data;
> +	}
...

> +
> +free_exit:
> +	sev_tio_dev_reclaim(dev_data, &tdev->spdm);
> +	kfree(dev_data);

Correct to free even if not allocated here?
Perhaps a comment if so.

> +
> +	return ret;
> +}
> +


> +static int ide_refresh(struct tsm_dev *tdev, void *private_data)
> +{
> +	struct tsm_dev_tio *dev_data = tdev->data;
> +	int ret;
> +
> +	if (!dev_data)
> +		return -ENODEV;
> +
> +	ret = sev_tio_ide_refresh(dev_data, &tdev->spdm);
> +
> +	return ret;

	return sev_tio_ide_refresh()

> +}

> +
> +static int tdi_create(struct tsm_tdi *tdi)
> +{
> +	struct tsm_tdi_tio *tdi_data = tdi->data;
> +	int ret;
> +
> +	if (tdi_data)
> +		return -EBUSY;
> +
> +	tdi_data = kzalloc(sizeof(*tdi_data), GFP_KERNEL);
> +	if (!tdi_data)
> +		return -ENOMEM;
> +
> +	ret = sev_tio_tdi_create(tdi->tdev->data, tdi_data, pci_dev_id(tdi->pdev),
> +				 tdi->rseg, tdi->rseg_valid);
> +	if (ret)
> +		kfree(tdi_data);
	if (ret) {
		kfree(tdi_data);
		return ret;
	}

	tid->data = tdi_data;

	return 0;

is slightly longer but a more standard form so easier to review.

> +	else
> +		tdi->data = tdi_data;
> +
> +	return ret;
> +}
> +

> +
> +static int guest_request(struct tsm_tdi *tdi, u32 guest_rid, u64 kvmid, void *req_data,

Probably wrap nearer 80 chars.

> +			 enum tsm_tdisp_state *state, void *private_data)
> +{
> +	struct tsm_dev_tio *dev_data = tdi->tdev->data;
> +	struct tio_guest_request *req = req_data;
> +	int ret;
> +
> +	if (!tdi->data)
> +		return -EFAULT;
> +
> +	if (dev_data->cmd == 0) {
> +		ret = sev_tio_guest_request(&req->data, guest_rid, kvmid,
> +					    dev_data, tdi->data, &tdi->tdev->spdm);
> +		req->fw_err = dev_data->psp_ret;

If the above returned an error is psp_ret always set? I think not.
So maybe separate if (ret) condition, then set this and finally call
the code below.

> +		ret = mkret(ret, dev_data);
> +		if (ret > 0)
> +			return ret;
> +	} else if (dev_data->cmd == SEV_CMD_TIO_GUEST_REQUEST) {
> +		ret = sev_tio_continue(dev_data, &tdi->tdev->spdm);
> +		ret = mkret(ret, dev_data);
> +		if (ret > 0)
> +			return ret;
> +	}
> +
> +	if (dev_data->cmd == 0 && state) {
> +		ret = sev_tio_tdi_status(tdi->tdev->data, tdi->data, &tdi->tdev->spdm);
> +		ret = mkret(ret, dev_data);
> +		if (ret > 0)
> +			return ret;
> +	} else if (dev_data->cmd == SEV_CMD_TIO_TDI_STATUS) {
> +		ret = sev_tio_continue(dev_data, &tdi->tdev->spdm);
> +		ret = mkret(ret, dev_data);
> +		if (ret > 0)
> +			return ret;
> +
> +		ret = sev_tio_tdi_status_fin(tdi->tdev->data, tdi->data, state);
> +	}
> +
> +	return ret;
> +}
> +
> +static int tdi_status(struct tsm_tdi *tdi, void *private_data, struct tsm_tdi_status *ts)
> +{
> +	struct tsm_dev_tio *dev_data = tdi->tdev->data;
> +	int ret;
> +
> +	if (!tdi->data)
> +		return -ENODEV;
> +
> +#if 0 /* Not implemented yet */
> +	if (dev_data->cmd == 0) {
> +		ret = sev_tio_tdi_info(tdi->tdev->data, tdi->data, ts);
> +		ret = mkret(ret, dev_data);
> +		if (ret)
> +			return ret;
> +	}
> +#endif
> +
> +	if (dev_data->cmd == 0) {
> +		ret = sev_tio_tdi_status(tdi->tdev->data, tdi->data, &tdi->tdev->spdm);
> +		ret = mkret(ret, dev_data);
> +		if (ret)
> +			return ret;
> +
> +		ret = sev_tio_tdi_status_fin(tdi->tdev->data, tdi->data, &ts->state);

Given code as it stands. Might as well return here.

> +	} else if (dev_data->cmd == SEV_CMD_TIO_TDI_STATUS) {
Making this just an if.

> +		ret = sev_tio_continue(dev_data, &tdi->tdev->spdm);
> +		ret = mkret(ret, dev_data);
> +		if (ret)
> +			return ret;
> +
> +		ret = sev_tio_tdi_status_fin(tdi->tdev->data, tdi->data, &ts->state);
and here.

> +	} else {

Making this the inline code as no need for else.

> +		pci_err(tdi->pdev, "Wrong state, cmd 0x%x in flight\n",
> +			dev_data->cmd);
> +	}
> +
> +	return ret;
> +}
> +
> +struct tsm_ops sev_tsm_ops = {
> +	.dev_connect = dev_connect,
> +	.dev_reclaim = dev_reclaim,
> +	.dev_status = dev_status,
> +	.ide_refresh = ide_refresh,
> +	.tdi_bind = tdi_bind,
> +	.tdi_reclaim = tdi_reclaim,
> +	.guest_request = guest_request,
> +	.tdi_status = tdi_status,
> +};