[PATCH v1 0/8] Support ACPI PSP on Hyper-V

[PATCH v1 0/8] Support ACPI PSP on Hyper-V

[Jeremi Piotrowski]

This patch series introduces support for discovering AMD's PSP from an ACPI
table and extends the CCP driver to allow binding to that device on x86. This
method of PSP discovery is used on Hyper-V when SNP isolation support is
exposed to the guest. There is no ACPI node associated with this PSP, so after
parsing the ASPT it is registered with the system as a platform_device.

I was not quite sure about where to place some of this code but opted for
placing the ASPT parsing code in drivers/acpi/ and the platform device creation
in arch/x86/ because configuring the irq for the PSP through the ACPI interface
requires poking at bits from the architectural vector domain. This was also
inspired by the sev-guest device.

This series is a prerequisite for nested SNP-host support on Hyper-V but is
independent of the SNP-host support patch set. Hyper-V only supports nested
SEV-SNP (not SEV or SEV-ES) so the PSP only supports a subset of the full PSP
command set. Without SNP-host support (which is not upstream yet), the only
PSP command that will succeed is SEV_PLATFORM_STATUS.

Jeremi Piotrowski (8):
  include/acpi: add definition of ASPT table
  ACPI: ASPT: Add helper to parse table
  x86/psp: Register PSP platform device when ASP table is present
  x86/psp: Add IRQ support
  crypto: cpp - Bind to psp platform device on x86
  crypto: ccp - Add vdata for platform device
  crypto: ccp - Skip DMA coherency check for platform psp
  crypto: ccp - Allow platform device to be psp master device

 arch/x86/kernel/Makefile          |   2 +-
 arch/x86/kernel/psp.c             | 213 ++++++++++++++++++++++++++++++
 drivers/acpi/Makefile             |   1 +
 drivers/acpi/aspt.c               | 104 +++++++++++++++
 drivers/crypto/ccp/sp-dev.c       |  67 +++++++++-
 drivers/crypto/ccp/sp-dev.h       |  16 ++-
 drivers/crypto/ccp/sp-pci.c       |  48 -------
 drivers/crypto/ccp/sp-platform.c  |  73 +++++++++-
 include/acpi/actbl1.h             |  46 +++++++
 include/linux/platform_data/psp.h |  32 +++++
 10 files changed, 544 insertions(+), 58 deletions(-)
 create mode 100644 arch/x86/kernel/psp.c
 create mode 100644 drivers/acpi/aspt.c
 create mode 100644 include/linux/platform_data/psp.h

-- 
2.25.1

[PATCH v1 1/8] include/acpi: add definition of ASPT table

[Jeremi Piotrowski]

The AMD Secure Processor ACPI Table provides the memory location of the
register window and register offsets necessary to communicate with AMD's
PSP (Platform Security Processor). This table is exposed on Hyper-V VMs
configured with support for AMD's SNP isolation technology.

Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
---
 include/acpi/actbl1.h | 46 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index 15c78678c5d3..00d40373df37 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -26,6 +26,7 @@
  */
 #define ACPI_SIG_AEST           "AEST"	/* Arm Error Source Table */
 #define ACPI_SIG_ASF            "ASF!"	/* Alert Standard Format table */
+#define ACPI_SIG_ASPT           "ASPT"	/* AMD Secure Processor Table */
 #define ACPI_SIG_BERT           "BERT"	/* Boot Error Record Table */
 #define ACPI_SIG_BGRT           "BGRT"	/* Boot Graphics Resource Table */
 #define ACPI_SIG_BOOT           "BOOT"	/* Simple Boot Flag Table */
@@ -106,6 +107,51 @@ struct acpi_whea_header {
 	u64 mask;		/* Bitmask required for this register instruction */
 };
 
+/* https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/acpitabl/ns-acpitabl-aspt_table */
+#define ASPT_REVISION_ID 0x01
+struct acpi_table_aspt {
+	struct acpi_table_header header;
+	u32 num_entries;
+};
+
+struct acpi_aspt_header {
+	u16 type;
+	u16 length;
+};
+
+enum acpi_aspt_type {
+	ACPI_ASPT_TYPE_GLOBAL_REGS = 0,
+	ACPI_ASPT_TYPE_SEV_MBOX_REGS = 1,
+	ACPI_ASPT_TYPE_ACPI_MBOX_REGS = 2,
+};
+
+/* 0: ASPT Global Registers */
+struct acpi_aspt_global_regs {
+	struct acpi_aspt_header header;
+	u32 reserved;
+	u64 feature_reg_addr;
+	u64 irq_en_reg_addr;
+	u64 irq_st_reg_addr;
+};
+
+/* 1: ASPT SEV Mailbox Registers */
+struct acpi_aspt_sev_mbox_regs {
+	struct acpi_aspt_header header;
+	u8 mbox_irq_id;
+	u8 reserved[3];
+	u64 cmd_resp_reg_addr;
+	u64 cmd_buf_lo_reg_addr;
+	u64 cmd_buf_hi_reg_addr;
+};
+
+/* 2: ASPT ACPI Mailbox Registers */
+struct acpi_aspt_acpi_mbox_regs {
+	struct acpi_aspt_header header;
+	u32 reserved1;
+	u64 cmd_resp_reg_addr;
+	u64 reserved2[2];
+};
+
 /*******************************************************************************
  *
  * ASF - Alert Standard Format table (Signature "ASF!")
-- 
2.25.1

[PATCH v1 2/8] ACPI: ASPT: Add helper to parse table

[Jeremi Piotrowski]

The ASP table indicates the presence of a Platform Security Processor
with a register window and registers to configure interrupt delivery.
The helper checks for the presence of the table and returns a resource
and struct with register offsets.

Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
---
 drivers/acpi/Makefile             |   1 +
 drivers/acpi/aspt.c               | 104 ++++++++++++++++++++++++++++++
 include/linux/platform_data/psp.h |  32 +++++++++
 3 files changed, 137 insertions(+)
 create mode 100644 drivers/acpi/aspt.c
 create mode 100644 include/linux/platform_data/psp.h

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 0002eecbf870..9621c90e0221 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -57,6 +57,7 @@ acpi-y				+= evged.o
 acpi-y				+= sysfs.o
 acpi-y				+= property.o
 acpi-$(CONFIG_X86)		+= acpi_cmos_rtc.o
+acpi-$(CONFIG_X86)		+= aspt.o
 acpi-$(CONFIG_X86)		+= x86/apple.o
 acpi-$(CONFIG_X86)		+= x86/utils.o
 acpi-$(CONFIG_X86)		+= x86/s2idle.o
diff --git a/drivers/acpi/aspt.c b/drivers/acpi/aspt.c
new file mode 100644
index 000000000000..cf629db35036
--- /dev/null
+++ b/drivers/acpi/aspt.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define pr_fmt(fmt) "ACPI: ASPT: " fmt
+#include <linux/acpi.h>
+#include <linux/kernel.h>
+#include <linux/platform_data/psp.h>
+
+static int __init psp_validate_regs(const struct acpi_aspt_global_regs *gregs,
+	const struct acpi_aspt_sev_mbox_regs *sevregs,
+	const struct acpi_aspt_acpi_mbox_regs *acpiregs)
+{
+	u64 pfn;
+	int idx;
+	u64 regs[] = {
+		gregs->feature_reg_addr,
+		gregs->irq_en_reg_addr,
+		gregs->irq_st_reg_addr,
+		sevregs->cmd_resp_reg_addr,
+		sevregs->cmd_buf_lo_reg_addr,
+		sevregs->cmd_buf_hi_reg_addr,
+		acpiregs->cmd_resp_reg_addr
+	};
+	pfn = regs[0] >> PAGE_SHIFT;
+	for (idx = 1; idx < ARRAY_SIZE(regs); idx++) {
+		if (regs[idx] >> PAGE_SHIFT != pfn)
+			return -EINVAL;
+	}
+	return 0;
+}
+
+/**
+ * acpi_parse_aspt - Parse ASPT table and return contained information
+ * @res: will be filled with the address and size of the ASP register window
+ * @pdata: will be filled with the register offsets parsed from the ASPT table
+ */
+int __init acpi_parse_aspt(struct resource *res, struct psp_platform_data *pdata)
+{
+	struct acpi_aspt_acpi_mbox_regs acpiregs = {};
+	struct acpi_aspt_sev_mbox_regs sevregs = {};
+	struct acpi_aspt_global_regs gregs = {};
+	struct acpi_aspt_header *entry, *end;
+	struct acpi_table_aspt *aspt;
+	unsigned long base;
+	acpi_status status;
+	int err = 0;
+
+	status = acpi_get_table(ACPI_SIG_ASPT, 0, (struct acpi_table_header **)&aspt);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+	if (aspt->header.revision != ASPT_REVISION_ID) {
+		pr_err("unsupported table revision: %d\n", (int)aspt->header.revision);
+		err = -ENODEV;
+		goto exit;
+	}
+	entry = (struct acpi_aspt_header *)(aspt + 1);
+	end = (struct acpi_aspt_header *)((void *)aspt + aspt->header.length);
+	while (entry < end) {
+		if (((void *)entry + entry->length) > (void *)end) {
+			pr_err("error during parsing\n");
+			err = -EINVAL;
+			goto exit;
+		}
+		switch (entry->type) {
+		case ACPI_ASPT_TYPE_GLOBAL_REGS:
+			memcpy(&gregs, entry, entry->length);
+			break;
+		case ACPI_ASPT_TYPE_SEV_MBOX_REGS:
+			memcpy(&sevregs, entry, entry->length);
+			break;
+		case ACPI_ASPT_TYPE_ACPI_MBOX_REGS:
+			memcpy(&acpiregs, entry, entry->length);
+			break;
+		}
+		entry = (struct acpi_aspt_header *)((void *)entry + entry->length);
+	}
+	if (!gregs.header.length || !sevregs.header.length || !acpiregs.header.length) {
+		pr_err("missing ASPT table entry: %u %u %u\n", gregs.header.length,
+			sevregs.header.length,
+			acpiregs.header.length);
+		err = -EINVAL;
+		goto exit;
+	}
+	/* All registers are expected to be within the same page */
+	err = psp_validate_regs(&gregs, &sevregs, &acpiregs);
+	if (err) {
+		pr_err("ASPT registers span multiple pages\n");
+		goto exit;
+	}
+
+	base = ALIGN_DOWN(gregs.feature_reg_addr, PAGE_SIZE);
+	*res = (struct resource)DEFINE_RES_MEM(base, PAGE_SIZE);
+
+	pdata->sev_cmd_resp_reg = sevregs.cmd_resp_reg_addr & ~PAGE_MASK;
+	pdata->sev_cmd_buf_lo_reg = sevregs.cmd_buf_lo_reg_addr & ~PAGE_MASK;
+	pdata->sev_cmd_buf_hi_reg = sevregs.cmd_buf_hi_reg_addr & ~PAGE_MASK;
+	pdata->feature_reg = gregs.feature_reg_addr & ~PAGE_MASK;
+	pdata->irq_en_reg = gregs.irq_en_reg_addr & ~PAGE_MASK;
+	pdata->irq_st_reg = gregs.irq_st_reg_addr & ~PAGE_MASK;
+	pdata->mbox_irq_id = sevregs.mbox_irq_id;
+	pdata->acpi_cmd_resp_reg = acpiregs.cmd_resp_reg_addr & ~PAGE_MASK;
+
+exit:
+	acpi_put_table((struct acpi_table_header *)aspt);
+	return err;
+}
diff --git a/include/linux/platform_data/psp.h b/include/linux/platform_data/psp.h
new file mode 100644
index 000000000000..b761f72168d6
--- /dev/null
+++ b/include/linux/platform_data/psp.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * psp.h - PSP register offsets parsed from ASPT ACPI table
+ */
+
+#ifndef __LINUX_PSP_H
+#define __LINUX_PSP_H
+
+#include <linux/types.h>
+#include <linux/ioport.h>
+
+struct psp_platform_data {
+	int sev_cmd_resp_reg;
+	int sev_cmd_buf_lo_reg;
+	int sev_cmd_buf_hi_reg;
+	int feature_reg;
+	int irq_en_reg;
+	int irq_st_reg;
+	int mbox_irq_id;
+	int acpi_cmd_resp_reg;
+};
+
+#if IS_ENABLED(CONFIG_ACPI)
+int acpi_parse_aspt(struct resource *res, struct psp_platform_data *pdata);
+#else
+static inline acpi_parse_aspt(struct resource *res, struct psp_platform_data *pdata)
+{
+	return -ENODEV;
+}
+#endif
+
+#endif /* __LINUX_PSP_H */
-- 
2.25.1

[PATCH v1 3/8] x86/psp: Register PSP platform device when ASP table is present

[Jeremi Piotrowski]

The ASP table contains the memory location of the register window for
communication with the Platform Security Processor. The device is not
exposed as an acpi node, so it is necessary to probe for the table and
register a platform_device to represent it in the kernel.

At least conceptually, the same PSP may be exposed on the PCIe bus as
well, in which case it would be necessary to choose whether to use a PCI
BAR or the register window defined in ASPT for communication. There is
no advantage to using the ACPI and there are no known bare-metal systems
that expose the ASP table, so device registration is restricted to the
only systems known to provide an ASPT: Hyper-V VMs. Hyper-V VMs also do
not expose the PSP over PCIe.

This is a skeleton device at this point, as the ccp driver is not yet
prepared to correctly probe it. Interrupt configuration will come later
on as well.

Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
---
 arch/x86/kernel/Makefile |  2 +-
 arch/x86/kernel/psp.c    | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/kernel/psp.c

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index f901658d9f7c..e2e19f2d08a7 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -139,7 +139,7 @@ obj-$(CONFIG_UNWINDER_ORC)		+= unwind_orc.o
 obj-$(CONFIG_UNWINDER_FRAME_POINTER)	+= unwind_frame.o
 obj-$(CONFIG_UNWINDER_GUESS)		+= unwind_guess.o
 
-obj-$(CONFIG_AMD_MEM_ENCRYPT)		+= sev.o
+obj-$(CONFIG_AMD_MEM_ENCRYPT)		+= psp.o sev.o
 
 obj-$(CONFIG_CFI_CLANG)			+= cfi.o
 
diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c
new file mode 100644
index 000000000000..d404df47cc04
--- /dev/null
+++ b/arch/x86/kernel/psp.c
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/platform_data/psp.h>
+#include <linux/platform_device.h>
+#include <asm/hypervisor.h>
+
+static struct platform_device psp_device = {
+	.name           = "psp",
+	.id             = PLATFORM_DEVID_NONE,
+};
+
+static int __init psp_init_platform_device(void)
+{
+	struct psp_platform_data pdata = {};
+	struct resource res[1];
+	int err;
+
+	/*
+	 * The ACPI PSP interface is mutually exclusive with the PCIe interface,
+	 * but there is no reason to use the ACPI interface over the PCIe one.
+	 * Restrict probing ACPI PSP to platforms known to only expose the ACPI
+	 * interface, which at this time is SNP-host capable Hyper-V VMs.
+	 */
+	if (!hypervisor_is_type(X86_HYPER_MS_HYPERV))
+		return -ENODEV;
+
+	err = acpi_parse_aspt(res, &pdata);
+	if (err)
+		return err;
+	err = platform_device_add_resources(&psp_device, res, 1);
+	if (err)
+		return err;
+
+	err = platform_device_register(&psp_device);
+	if (err)
+		return err;
+	return 0;
+}
+device_initcall(psp_init_platform_device);
-- 
2.25.1

[PATCH v1 4/8] x86/psp: Add IRQ support

[Jeremi Piotrowski]

The ACPI PSP device provides a mailbox irq that needs to be configured
through the ACPI mailbox register first. This requires passing a CPU
vector and physical CPU id and then enabling interrupt delivery.
Allocate the irq directly from the default irq domain
(x86_vector_domain) to get access to the required information. By
passing a cpumask through irq_alloc_info the vector is immediately
allocated (and not later during activation) and can be retrieved.

Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
---
 arch/x86/kernel/psp.c | 179 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 175 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c
index d404df47cc04..24181d132bae 100644
--- a/arch/x86/kernel/psp.c
+++ b/arch/x86/kernel/psp.c
@@ -1,8 +1,176 @@
 // SPDX-License-Identifier: GPL-2.0-only
-
+#define pr_fmt(fmt) "psp: " fmt
 #include <linux/platform_data/psp.h>
 #include <linux/platform_device.h>
+#include <linux/iopoll.h>
+#include <linux/irq.h>
 #include <asm/hypervisor.h>
+#include <asm/irqdomain.h>
+
+#define PSP_ACPI_DATA_SHIFT 0
+#define PSP_ACPI_DATA_MASK GENMASK(15, 0)
+#define PSP_ACPI_CMDID_SHIFT 16
+#define PSP_ACPI_CMDID_MASK GENMASK(25, 16)
+#define PSP_ACPI_STATUS_SHIFT 26
+#define PSP_ACPI_STATUS_MASK GENMASK(30, 26)
+#define PSP_ACPI_RESPONSE_BIT BIT(31)
+
+#define PSP_ACPI_VECTOR_SHIFT 0
+#define PSP_ACPI_VECTOR_MASK GENMASK(7, 0)
+#define PSP_ACPI_MBOX_IRQID_SHIFT 10
+#define PSP_ACPI_MBOX_IRQID_MASK GENMASK(15, 10)
+
+#define PSP_ACPI_IRQ_EN_BIT BIT(0)
+#define PSP_ACPI_IRQ_EN_MBOX_IRQID_SHIFT 10
+#define PSP_ACPI_IRQ_EN_MBOX_IRQID_MASK GENMASK(15, 10)
+
+// AMD Secure Processor
+enum ASP_CMDID {
+	ASP_CMDID_PART1  = 0x82,
+	ASP_CMDID_PART2  = 0x83,
+	ASP_CMDID_PART3  = 0x84,
+	ASP_CMDID_IRQ_EN = 0x85,
+};
+
+enum ASP_CMD_STATUS {
+	ASP_CMD_STATUS_SUCCESS = 0x0,
+	ASP_CMD_STATUS_INVALID_CMD = 0x1,
+	ASP_CMD_STATUS_INVALID_PARAM = 0x2,
+	ASP_CMD_STATUS_INVALID_FW_STATE = 0x3,
+	ASP_CMD_STATUS_FAILURE = 0x1F,
+};
+
+struct psp_irq_data {
+	void __iomem *base;
+	int mbox_irq_id;
+	int acpi_cmd_resp_reg;
+};
+
+static int psp_sync_cmd(void __iomem *reg, u8 cmd, u16 data)
+{
+	u32 val = 0;
+	int err;
+
+	val |= data & PSP_ACPI_DATA_MASK;
+	val |= (cmd << PSP_ACPI_CMDID_SHIFT) & PSP_ACPI_CMDID_MASK;
+	writel(val, reg);
+	err = readl_poll_timeout_atomic(reg, val, val & PSP_ACPI_RESPONSE_BIT, 2, 10000);
+	if (err < 0)
+		return err;
+	return (val & PSP_ACPI_STATUS_MASK) >> PSP_ACPI_STATUS_SHIFT;
+}
+
+static int psp_set_irq_enable(struct psp_irq_data *data, bool irq_en)
+{
+	void __iomem *reg = data->base + data->acpi_cmd_resp_reg;
+	u8 mbox_irq_id = data->mbox_irq_id;
+	u16 val = 0;
+	int err;
+
+	val |= irq_en ? PSP_ACPI_IRQ_EN_BIT : 0;
+	val |= (mbox_irq_id << PSP_ACPI_IRQ_EN_MBOX_IRQID_SHIFT) & PSP_ACPI_IRQ_EN_MBOX_IRQID_MASK;
+	err = psp_sync_cmd(reg, ASP_CMDID_IRQ_EN, val);
+	if (err != ASP_CMD_STATUS_SUCCESS) {
+		pr_err("ASP_CMDID_IRQ_EN failed: %d\n", err);
+		return -EIO;
+	}
+	return 0;
+}
+
+static int psp_configure_irq(struct psp_irq_data *data, int vector, unsigned int cpu)
+{
+	void __iomem *reg = data->base + data->acpi_cmd_resp_reg;
+	unsigned int dest_cpu = cpu_physical_id(cpu);
+	u8 mbox_irq_id = data->mbox_irq_id;
+	u16 part1, part2, part3;
+	int err;
+
+	part1 = dest_cpu;
+	part2 = dest_cpu >> 16;
+	part3 = vector & PSP_ACPI_VECTOR_MASK;
+	part3 |= (mbox_irq_id << PSP_ACPI_MBOX_IRQID_SHIFT) & PSP_ACPI_MBOX_IRQID_MASK;
+
+	err = psp_sync_cmd(reg, ASP_CMDID_PART1, part1);
+	if (err != ASP_CMD_STATUS_SUCCESS) {
+		pr_err("ASP_CMDID_PART1 failed: %d\n", err);
+		return -EIO;
+	}
+	err = psp_sync_cmd(reg, ASP_CMDID_PART2, part2);
+	if (err != ASP_CMD_STATUS_SUCCESS) {
+		pr_err("ASP_CMDID_PART2 failed: %d\n", err);
+		return -EIO;
+	}
+	err = psp_sync_cmd(reg, ASP_CMDID_PART3, part3);
+	if (err != ASP_CMD_STATUS_SUCCESS) {
+		pr_err("ASP_CMDID_PART3 failed: %d\n", err);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+
+static int psp_init_irq(const struct psp_platform_data *pdata,
+	const struct resource *reg, struct resource *irq)
+{
+	struct psp_irq_data pspirqd;
+	struct irq_alloc_info info;
+	struct irq_data *data = NULL;
+	struct irq_cfg *cfg = NULL;
+	void __iomem *base = NULL;
+	int virq;
+	int err;
+
+	base = ioremap(reg->start, resource_size(reg));
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+	if (!base)
+		return -ENOMEM;
+	pspirqd.mbox_irq_id = pdata->mbox_irq_id;
+	pspirqd.acpi_cmd_resp_reg = pdata->acpi_cmd_resp_reg;
+	pspirqd.base = base;
+	init_irq_alloc_info(&info, cpumask_of(0));
+	virq = irq_domain_alloc_irqs(NULL, 1, NUMA_NO_NODE, &info);
+	if (virq <= 0) {
+		pr_err("failed to allocate vector: %d\n", virq);
+		err = -ENOMEM;
+		goto unmap;
+	}
+	irq_set_handler(virq, handle_edge_irq);
+	data = irq_get_irq_data(virq);
+	if (!data) {
+		pr_err("no irq data\n");
+		err = -ENODEV;
+		goto freeirq;
+
+	}
+	cfg = irqd_cfg(data);
+	if (!data) {
+		pr_err("no irq cfg\n");
+		err = -ENODEV;
+		goto freeirq;
+	}
+	err = psp_configure_irq(&pspirqd, cfg->vector, 0);
+	if (err) {
+		pr_err("failed to configure irq: %d\n", err);
+		goto freeirq;
+	}
+	err = psp_set_irq_enable(&pspirqd, true);
+	if (err) {
+		pr_err("failed to enable irq: %d\n", err);
+		goto freeirq;
+	}
+
+	*irq = (struct resource)DEFINE_RES_IRQ(virq);
+	iounmap(base);
+	return 0;
+
+freeirq:
+	irq_domain_free_irqs(virq, 1);
+unmap:
+	iounmap(base);
+	return err;
+}
 
 static struct platform_device psp_device = {
 	.name           = "psp",
@@ -12,7 +180,7 @@ static struct platform_device psp_device = {
 static int __init psp_init_platform_device(void)
 {
 	struct psp_platform_data pdata = {};
-	struct resource res[1];
+	struct resource res[2];
 	int err;
 
 	/*
@@ -24,10 +192,13 @@ static int __init psp_init_platform_device(void)
 	if (!hypervisor_is_type(X86_HYPER_MS_HYPERV))
 		return -ENODEV;
 
-	err = acpi_parse_aspt(res, &pdata);
+	err = acpi_parse_aspt(&res[0], &pdata);
+	if (err)
+		return err;
+	err = psp_init_irq(&pdata, &res[0], &res[1]);
 	if (err)
 		return err;
-	err = platform_device_add_resources(&psp_device, res, 1);
+	err = platform_device_add_resources(&psp_device, res, 2);
 	if (err)
 		return err;
 
-- 
2.25.1

[PATCH v1 5/8] crypto: cpp - Bind to psp platform device on x86

[Jeremi Piotrowski]

The PSP in Hyper-V VMs is exposed through the ASP ACPI table and is
represented as a platform_device. Allow the ccp driver to bind to it by
adding an id_table and initing the platform_driver also on x86. At this
point probe is called for the psp device but init fails due to missing
driver data.

Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
---
 drivers/crypto/ccp/sp-dev.c      | 8 ++++++--
 drivers/crypto/ccp/sp-platform.c | 7 +++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c
index 7eb3e4668286..52b8d957d0f6 100644
--- a/drivers/crypto/ccp/sp-dev.c
+++ b/drivers/crypto/ccp/sp-dev.c
@@ -258,7 +258,11 @@ static int __init sp_mod_init(void)
 	ret = sp_pci_init();
 	if (ret)
 		return ret;
-
+	ret = sp_platform_init();
+	if (ret) {
+		sp_pci_exit();
+		return ret;
+	}
 #ifdef CONFIG_CRYPTO_DEV_SP_PSP
 	psp_pci_init();
 #endif
@@ -286,7 +290,7 @@ static void __exit sp_mod_exit(void)
 #ifdef CONFIG_CRYPTO_DEV_SP_PSP
 	psp_pci_exit();
 #endif
-
+	sp_platform_exit();
 	sp_pci_exit();
 #endif
 
diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
index 7d79a8744f9a..ea8926e87981 100644
--- a/drivers/crypto/ccp/sp-platform.c
+++ b/drivers/crypto/ccp/sp-platform.c
@@ -56,6 +56,12 @@ static const struct of_device_id sp_of_match[] = {
 MODULE_DEVICE_TABLE(of, sp_of_match);
 #endif
 
+static const struct platform_device_id sp_plat_match[] = {
+	{ "psp" },
+	{ },
+};
+MODULE_DEVICE_TABLE(platform, sp_plat_match);
+
 static struct sp_dev_vdata *sp_get_of_version(struct platform_device *pdev)
 {
 #ifdef CONFIG_OF
@@ -212,6 +218,7 @@ static int sp_platform_resume(struct platform_device *pdev)
 #endif
 
 static struct platform_driver sp_platform_driver = {
+	.id_table = sp_plat_match,
 	.driver = {
 		.name = "ccp",
 #ifdef CONFIG_ACPI
-- 
2.25.1

[PATCH v1 6/8] crypto: ccp - Add vdata for platform device

[Jeremi Piotrowski]

When matching the "psp" platform_device, determine the register offsets
at runtime from the ASP ACPI table. Pass the parsed register offsets
from the ASPT through platdata.

To support this scenario, mark the members of 'struct sev_vdata' and
'struct psp_vdata' non-const so that the probe function can write the
values. This does not affect the other users of sev_vdata/psp_vdata as
they define the whole struct const and the pointer in struct
sp_dev_vdata stays const too.

Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
---
 arch/x86/kernel/psp.c            |  3 ++
 drivers/crypto/ccp/sp-dev.h      | 12 +++----
 drivers/crypto/ccp/sp-platform.c | 57 +++++++++++++++++++++++++++++++-
 3 files changed, 65 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c
index 24181d132bae..68511a14df63 100644
--- a/arch/x86/kernel/psp.c
+++ b/arch/x86/kernel/psp.c
@@ -199,6 +199,9 @@ static int __init psp_init_platform_device(void)
 	if (err)
 		return err;
 	err = platform_device_add_resources(&psp_device, res, 2);
+	if (err)
+		return err;
+	err = platform_device_add_data(&psp_device, &pdata, sizeof(pdata));
 	if (err)
 		return err;
 
diff --git a/drivers/crypto/ccp/sp-dev.h b/drivers/crypto/ccp/sp-dev.h
index 20377e67f65d..aaa651364425 100644
--- a/drivers/crypto/ccp/sp-dev.h
+++ b/drivers/crypto/ccp/sp-dev.h
@@ -40,9 +40,9 @@ struct ccp_vdata {
 };
 
 struct sev_vdata {
-	const unsigned int cmdresp_reg;
-	const unsigned int cmdbuff_addr_lo_reg;
-	const unsigned int cmdbuff_addr_hi_reg;
+	unsigned int cmdresp_reg;
+	unsigned int cmdbuff_addr_lo_reg;
+	unsigned int cmdbuff_addr_hi_reg;
 };
 
 struct tee_vdata {
@@ -56,9 +56,9 @@ struct tee_vdata {
 struct psp_vdata {
 	const struct sev_vdata *sev;
 	const struct tee_vdata *tee;
-	const unsigned int feature_reg;
-	const unsigned int inten_reg;
-	const unsigned int intsts_reg;
+	unsigned int feature_reg;
+	unsigned int inten_reg;
+	unsigned int intsts_reg;
 };
 
 /* Structure to hold SP device data */
diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
index ea8926e87981..281dbf6b150c 100644
--- a/drivers/crypto/ccp/sp-platform.c
+++ b/drivers/crypto/ccp/sp-platform.c
@@ -22,6 +22,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/acpi.h>
+#include <linux/platform_data/psp.h>
 
 #include "ccp-dev.h"
 
@@ -30,11 +31,31 @@ struct sp_platform {
 	unsigned int irq_count;
 };
 
+#ifdef CONFIG_CRYPTO_DEV_SP_PSP
+static struct sev_vdata sev_platform = {
+	.cmdresp_reg = -1,
+	.cmdbuff_addr_lo_reg = -1,
+	.cmdbuff_addr_hi_reg = -1,
+};
+static struct psp_vdata psp_platform = {
+	.sev = &sev_platform,
+	.feature_reg = -1,
+	.inten_reg = -1,
+	.intsts_reg = -1,
+};
+#endif
+
 static const struct sp_dev_vdata dev_vdata[] = {
 	{
 		.bar = 0,
 #ifdef CONFIG_CRYPTO_DEV_SP_CCP
 		.ccp_vdata = &ccpv3_platform,
+#endif
+	},
+	{
+		.bar = 0,
+#ifdef CONFIG_CRYPTO_DEV_SP_PSP
+		.psp_vdata = &psp_platform,
 #endif
 	},
 };
@@ -57,7 +78,7 @@ MODULE_DEVICE_TABLE(of, sp_of_match);
 #endif
 
 static const struct platform_device_id sp_plat_match[] = {
-	{ "psp" },
+	{ "psp", (kernel_ulong_t)&dev_vdata[1] },
 	{ },
 };
 MODULE_DEVICE_TABLE(platform, sp_plat_match);
@@ -86,6 +107,38 @@ static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev)
 	return NULL;
 }
 
+static struct sp_dev_vdata *sp_get_plat_version(struct platform_device *pdev)
+{
+	struct sp_dev_vdata *drvdata = (struct sp_dev_vdata *)pdev->id_entry->driver_data;
+	struct device *dev = &pdev->dev;
+
+	if (drvdata == &dev_vdata[1]) {
+		struct psp_platform_data *pdata = dev_get_platdata(dev);
+
+		if (!pdata) {
+			dev_err(dev, "missing platform data\n");
+			return NULL;
+		}
+#ifdef CONFIG_CRYPTO_DEV_SP_PSP
+		psp_platform.feature_reg = pdata->feature_reg;
+		psp_platform.inten_reg = pdata->irq_en_reg;
+		psp_platform.intsts_reg = pdata->irq_st_reg;
+		sev_platform.cmdresp_reg = pdata->sev_cmd_resp_reg;
+		sev_platform.cmdbuff_addr_lo_reg = pdata->sev_cmd_buf_lo_reg;
+		sev_platform.cmdbuff_addr_hi_reg = pdata->sev_cmd_buf_hi_reg;
+		dev_dbg(dev, "GLBL feature:\t%x\n", pdata->feature_reg);
+		dev_dbg(dev, "GLBL irq en:\t%x\n", pdata->irq_en_reg);
+		dev_dbg(dev, "GLBL irq st:\t%x\n", pdata->irq_st_reg);
+		dev_dbg(dev, "SEV cmdresp:\t%x\n", pdata->sev_cmd_resp_reg);
+		dev_dbg(dev, "SEV cmdbuf lo:\t%x\n", pdata->sev_cmd_buf_lo_reg);
+		dev_dbg(dev, "SEV cmdbuf hi:\t%x\n", pdata->sev_cmd_buf_hi_reg);
+		dev_dbg(dev, "SEV mbox:\t%x\n", pdata->mbox_irq_id);
+		dev_dbg(dev, "ACPI cmdresp:\t%x\n", pdata->acpi_cmd_resp_reg);
+#endif
+	}
+	return drvdata;
+}
+
 static int sp_get_irqs(struct sp_device *sp)
 {
 	struct sp_platform *sp_platform = sp->dev_specific;
@@ -137,6 +190,8 @@ static int sp_platform_probe(struct platform_device *pdev)
 	sp->dev_specific = sp_platform;
 	sp->dev_vdata = pdev->dev.of_node ? sp_get_of_version(pdev)
 					 : sp_get_acpi_version(pdev);
+	if (!sp->dev_vdata && pdev->id_entry)
+		sp->dev_vdata = sp_get_plat_version(pdev);
 	if (!sp->dev_vdata) {
 		ret = -ENODEV;
 		dev_err(dev, "missing driver data\n");
-- 
2.25.1

[PATCH v1 7/8] crypto: ccp - Skip DMA coherency check for platform psp

[Jeremi Piotrowski]

The value of device_get_dma_attr() is only relevenat for ARM64 and CCP
devices to configure the value of the axcache attribute used to access
memory by the coprocessor. None of this applies to the platform psp so
skip it.

Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
---
 drivers/crypto/ccp/sp-platform.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
index 281dbf6b150c..b74f16e0e963 100644
--- a/drivers/crypto/ccp/sp-platform.c
+++ b/drivers/crypto/ccp/sp-platform.c
@@ -29,6 +29,7 @@
 struct sp_platform {
 	int coherent;
 	unsigned int irq_count;
+	bool is_platform;
 };
 
 #ifdef CONFIG_CRYPTO_DEV_SP_PSP
@@ -190,8 +191,10 @@ static int sp_platform_probe(struct platform_device *pdev)
 	sp->dev_specific = sp_platform;
 	sp->dev_vdata = pdev->dev.of_node ? sp_get_of_version(pdev)
 					 : sp_get_acpi_version(pdev);
-	if (!sp->dev_vdata && pdev->id_entry)
+	if (!sp->dev_vdata && pdev->id_entry) {
+		sp_platform->is_platform = true;
 		sp->dev_vdata = sp_get_plat_version(pdev);
+	}
 	if (!sp->dev_vdata) {
 		ret = -ENODEV;
 		dev_err(dev, "missing driver data\n");
@@ -205,7 +208,7 @@ static int sp_platform_probe(struct platform_device *pdev)
 	}
 
 	attr = device_get_dma_attr(dev);
-	if (attr == DEV_DMA_NOT_SUPPORTED) {
+	if (!sp_platform->is_platform && attr == DEV_DMA_NOT_SUPPORTED) {
 		dev_err(dev, "DMA is not supported");
 		goto e_err;
 	}
-- 
2.25.1

[PATCH v1 8/8] crypto: ccp - Allow platform device to be psp master device

[Jeremi Piotrowski]

Move the getters/setters to sp-dev.c, so that they can be accessed from
sp-pci.c and sp-platform.c. This makes it possible for the psp
platform_device to set the function pointers and be assigned the role of
master device by psp_dev_init().

While the case of a system having both a PCI and ACPI PSP is not
supported (and not known to occur in the wild), it makes sense to have a
single static global to assign to. Should such a system occur, the logic
in psp_set_master() is that the pci device is preferred.

Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
---
 drivers/crypto/ccp/sp-dev.c      | 59 ++++++++++++++++++++++++++++++++
 drivers/crypto/ccp/sp-dev.h      |  4 +++
 drivers/crypto/ccp/sp-pci.c      | 48 --------------------------
 drivers/crypto/ccp/sp-platform.c |  6 ++++
 4 files changed, 69 insertions(+), 48 deletions(-)

diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c
index 52b8d957d0f6..04b77d640f62 100644
--- a/drivers/crypto/ccp/sp-dev.c
+++ b/drivers/crypto/ccp/sp-dev.c
@@ -14,6 +14,8 @@
 #include <linux/kthread.h>
 #include <linux/sched.h>
 #include <linux/interrupt.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
 #include <linux/spinlock.h>
 #include <linux/spinlock_types.h>
 #include <linux/types.h>
@@ -39,6 +41,8 @@ static LIST_HEAD(sp_units);
 /* Ever-increasing value to produce unique unit numbers */
 static atomic_t sp_ordinal;
 
+static struct sp_device *sp_dev_master;
+
 static void sp_add_device(struct sp_device *sp)
 {
 	unsigned long flags;
@@ -250,6 +254,61 @@ struct sp_device *sp_get_psp_master_device(void)
 	return ret;
 }
 
+static bool sp_pci_is_master(struct sp_device *sp)
+{
+	struct device *dev_cur, *dev_new;
+	struct pci_dev *pdev_cur, *pdev_new;
+
+	dev_new = sp->dev;
+	dev_cur = sp_dev_master->dev;
+
+	pdev_new = to_pci_dev(dev_new);
+	pdev_cur = to_pci_dev(dev_cur);
+
+	if (pdev_new->bus->number < pdev_cur->bus->number)
+		return true;
+
+	if (PCI_SLOT(pdev_new->devfn) < PCI_SLOT(pdev_cur->devfn))
+		return true;
+
+	if (PCI_FUNC(pdev_new->devfn) < PCI_FUNC(pdev_cur->devfn))
+		return true;
+
+	return false;
+}
+
+void psp_set_master(struct sp_device *sp)
+{
+	struct device *dev_cur, *dev_new;
+
+	if (!sp_dev_master) {
+		sp_dev_master = sp;
+		return;
+	}
+
+	dev_new = sp->dev;
+	dev_cur = sp_dev_master->dev;
+
+	if (dev_is_pci(dev_new) && dev_is_pci(dev_cur) && sp_pci_is_master(sp))
+		sp_dev_master = sp;
+	if (dev_is_pci(dev_new) && dev_is_platform(dev_cur))
+		sp_dev_master = sp;
+}
+
+struct sp_device *psp_get_master(void)
+{
+	return sp_dev_master;
+}
+
+void psp_clear_master(struct sp_device *sp)
+{
+	if (sp == sp_dev_master) {
+		sp_dev_master = NULL;
+		dev_dbg(sp->dev, "Cleared sp_dev_master\n");
+	}
+}
+
+
 static int __init sp_mod_init(void)
 {
 #ifdef CONFIG_X86
diff --git a/drivers/crypto/ccp/sp-dev.h b/drivers/crypto/ccp/sp-dev.h
index aaa651364425..083e57652c7b 100644
--- a/drivers/crypto/ccp/sp-dev.h
+++ b/drivers/crypto/ccp/sp-dev.h
@@ -129,6 +129,10 @@ int sp_request_psp_irq(struct sp_device *sp, irq_handler_t handler,
 void sp_free_psp_irq(struct sp_device *sp, void *data);
 struct sp_device *sp_get_psp_master_device(void);
 
+void psp_set_master(struct sp_device *sp);
+struct sp_device *psp_get_master(void);
+void psp_clear_master(struct sp_device *sp);
+
 #ifdef CONFIG_CRYPTO_DEV_SP_CCP
 
 int ccp_dev_init(struct sp_device *sp);
diff --git a/drivers/crypto/ccp/sp-pci.c b/drivers/crypto/ccp/sp-pci.c
index 792d6da7f0c0..f9be8aba0acf 100644
--- a/drivers/crypto/ccp/sp-pci.c
+++ b/drivers/crypto/ccp/sp-pci.c
@@ -30,7 +30,6 @@ struct sp_pci {
 	int msix_count;
 	struct msix_entry msix_entry[MSIX_VECTORS];
 };
-static struct sp_device *sp_dev_master;
 
 #define attribute_show(name, def)						\
 static ssize_t name##_show(struct device *d, struct device_attribute *attr,	\
@@ -168,53 +167,6 @@ static void sp_free_irqs(struct sp_device *sp)
 	sp->psp_irq = 0;
 }
 
-static bool sp_pci_is_master(struct sp_device *sp)
-{
-	struct device *dev_cur, *dev_new;
-	struct pci_dev *pdev_cur, *pdev_new;
-
-	dev_new = sp->dev;
-	dev_cur = sp_dev_master->dev;
-
-	pdev_new = to_pci_dev(dev_new);
-	pdev_cur = to_pci_dev(dev_cur);
-
-	if (pdev_new->bus->number < pdev_cur->bus->number)
-		return true;
-
-	if (PCI_SLOT(pdev_new->devfn) < PCI_SLOT(pdev_cur->devfn))
-		return true;
-
-	if (PCI_FUNC(pdev_new->devfn) < PCI_FUNC(pdev_cur->devfn))
-		return true;
-
-	return false;
-}
-
-static void psp_set_master(struct sp_device *sp)
-{
-	if (!sp_dev_master) {
-		sp_dev_master = sp;
-		return;
-	}
-
-	if (sp_pci_is_master(sp))
-		sp_dev_master = sp;
-}
-
-static struct sp_device *psp_get_master(void)
-{
-	return sp_dev_master;
-}
-
-static void psp_clear_master(struct sp_device *sp)
-{
-	if (sp == sp_dev_master) {
-		sp_dev_master = NULL;
-		dev_dbg(sp->dev, "Cleared sp_dev_master\n");
-	}
-}
-
 static int sp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct sp_device *sp;
diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
index b74f16e0e963..d56b34255b97 100644
--- a/drivers/crypto/ccp/sp-platform.c
+++ b/drivers/crypto/ccp/sp-platform.c
@@ -231,6 +231,12 @@ static int sp_platform_probe(struct platform_device *pdev)
 
 	dev_set_drvdata(dev, sp);
 
+	if (sp_platform->is_platform) {
+		sp->set_psp_master_device = psp_set_master;
+		sp->get_psp_master_device = psp_get_master;
+		sp->clear_psp_master_device = psp_clear_master;
+	}
+
 	ret = sp_init(sp);
 	if (ret)
 		goto e_err;
-- 
2.25.1

Re: [PATCH v1 1/8] include/acpi: add definition of ASPT table

[Rafael J. Wysocki]

On Mon, Jan 23, 2023 at 4:23 PM Jeremi Piotrowski
<jpiotrowski@linux.microsoft.com> wrote:
>
> The AMD Secure Processor ACPI Table provides the memory location of the
> register window and register offsets necessary to communicate with AMD's
> PSP (Platform Security Processor). This table is exposed on Hyper-V VMs
> configured with support for AMD's SNP isolation technology.
>
> Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>

This modifies the ACPICA code, so it should at least be submitted as a
pull request to the upstream ACPICA project on GitHub.

Thanks!

> ---
>  include/acpi/actbl1.h | 46 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> index 15c78678c5d3..00d40373df37 100644
> --- a/include/acpi/actbl1.h
> +++ b/include/acpi/actbl1.h
> @@ -26,6 +26,7 @@
>   */
>  #define ACPI_SIG_AEST           "AEST" /* Arm Error Source Table */
>  #define ACPI_SIG_ASF            "ASF!" /* Alert Standard Format table */
> +#define ACPI_SIG_ASPT           "ASPT" /* AMD Secure Processor Table */
>  #define ACPI_SIG_BERT           "BERT" /* Boot Error Record Table */
>  #define ACPI_SIG_BGRT           "BGRT" /* Boot Graphics Resource Table */
>  #define ACPI_SIG_BOOT           "BOOT" /* Simple Boot Flag Table */
> @@ -106,6 +107,51 @@ struct acpi_whea_header {
>         u64 mask;               /* Bitmask required for this register instruction */
>  };
>
> +/* https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/acpitabl/ns-acpitabl-aspt_table */
> +#define ASPT_REVISION_ID 0x01
> +struct acpi_table_aspt {
> +       struct acpi_table_header header;
> +       u32 num_entries;
> +};
> +
> +struct acpi_aspt_header {
> +       u16 type;
> +       u16 length;
> +};
> +
> +enum acpi_aspt_type {
> +       ACPI_ASPT_TYPE_GLOBAL_REGS = 0,
> +       ACPI_ASPT_TYPE_SEV_MBOX_REGS = 1,
> +       ACPI_ASPT_TYPE_ACPI_MBOX_REGS = 2,
> +};
> +
> +/* 0: ASPT Global Registers */
> +struct acpi_aspt_global_regs {
> +       struct acpi_aspt_header header;
> +       u32 reserved;
> +       u64 feature_reg_addr;
> +       u64 irq_en_reg_addr;
> +       u64 irq_st_reg_addr;
> +};
> +
> +/* 1: ASPT SEV Mailbox Registers */
> +struct acpi_aspt_sev_mbox_regs {
> +       struct acpi_aspt_header header;
> +       u8 mbox_irq_id;
> +       u8 reserved[3];
> +       u64 cmd_resp_reg_addr;
> +       u64 cmd_buf_lo_reg_addr;
> +       u64 cmd_buf_hi_reg_addr;
> +};
> +
> +/* 2: ASPT ACPI Mailbox Registers */
> +struct acpi_aspt_acpi_mbox_regs {
> +       struct acpi_aspt_header header;
> +       u32 reserved1;
> +       u64 cmd_resp_reg_addr;
> +       u64 reserved2[2];
> +};
> +
>  /*******************************************************************************
>   *
>   * ASF - Alert Standard Format table (Signature "ASF!")
> --
> 2.25.1
>

Re: [PATCH v1 1/8] include/acpi: add definition of ASPT table

[Jeremi Piotrowski]

On Mon, Jan 23, 2023 at 08:56:32PM +0100, Rafael J. Wysocki wrote:
> On Mon, Jan 23, 2023 at 4:23 PM Jeremi Piotrowski
> <jpiotrowski@linux.microsoft.com> wrote:
> >
> > The AMD Secure Processor ACPI Table provides the memory location of the
> > register window and register offsets necessary to communicate with AMD's
> > PSP (Platform Security Processor). This table is exposed on Hyper-V VMs
> > configured with support for AMD's SNP isolation technology.
> >
> > Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
> 
> This modifies the ACPICA code, so it should at least be submitted as a
> pull request to the upstream ACPICA project on GitHub.
> 
> Thanks!

Hi Rafael,

Sorry, missed that part of the documentation. Here's the PR:
https://github.com/acpica/acpica/pull/829

Thanks,
Jeremi

> 
> > ---
> >  include/acpi/actbl1.h | 46 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> >
> > diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> > index 15c78678c5d3..00d40373df37 100644
> > --- a/include/acpi/actbl1.h
> > +++ b/include/acpi/actbl1.h
> > @@ -26,6 +26,7 @@
> >   */
> >  #define ACPI_SIG_AEST           "AEST" /* Arm Error Source Table */
> >  #define ACPI_SIG_ASF            "ASF!" /* Alert Standard Format table */
> > +#define ACPI_SIG_ASPT           "ASPT" /* AMD Secure Processor Table */
> >  #define ACPI_SIG_BERT           "BERT" /* Boot Error Record Table */
> >  #define ACPI_SIG_BGRT           "BGRT" /* Boot Graphics Resource Table */
> >  #define ACPI_SIG_BOOT           "BOOT" /* Simple Boot Flag Table */
> > @@ -106,6 +107,51 @@ struct acpi_whea_header {
> >         u64 mask;               /* Bitmask required for this register instruction */
> >  };
> >
> > +/* https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/acpitabl/ns-acpitabl-aspt_table */
> > +#define ASPT_REVISION_ID 0x01
> > +struct acpi_table_aspt {
> > +       struct acpi_table_header header;
> > +       u32 num_entries;
> > +};
> > +
> > +struct acpi_aspt_header {
> > +       u16 type;
> > +       u16 length;
> > +};
> > +
> > +enum acpi_aspt_type {
> > +       ACPI_ASPT_TYPE_GLOBAL_REGS = 0,
> > +       ACPI_ASPT_TYPE_SEV_MBOX_REGS = 1,
> > +       ACPI_ASPT_TYPE_ACPI_MBOX_REGS = 2,
> > +};
> > +
> > +/* 0: ASPT Global Registers */
> > +struct acpi_aspt_global_regs {
> > +       struct acpi_aspt_header header;
> > +       u32 reserved;
> > +       u64 feature_reg_addr;
> > +       u64 irq_en_reg_addr;
> > +       u64 irq_st_reg_addr;
> > +};
> > +
> > +/* 1: ASPT SEV Mailbox Registers */
> > +struct acpi_aspt_sev_mbox_regs {
> > +       struct acpi_aspt_header header;
> > +       u8 mbox_irq_id;
> > +       u8 reserved[3];
> > +       u64 cmd_resp_reg_addr;
> > +       u64 cmd_buf_lo_reg_addr;
> > +       u64 cmd_buf_hi_reg_addr;
> > +};
> > +
> > +/* 2: ASPT ACPI Mailbox Registers */
> > +struct acpi_aspt_acpi_mbox_regs {
> > +       struct acpi_aspt_header header;
> > +       u32 reserved1;
> > +       u64 cmd_resp_reg_addr;
> > +       u64 reserved2[2];
> > +};
> > +
> >  /*******************************************************************************
> >   *
> >   * ASF - Alert Standard Format table (Signature "ASF!")
> > --
> > 2.25.1
> >

Re: [PATCH v1 3/8] x86/psp: Register PSP platform device when ASP table is present

[Tom Lendacky]

On 1/23/23 09:22, Jeremi Piotrowski wrote:
> The ASP table contains the memory location of the register window for
> communication with the Platform Security Processor. The device is not
> exposed as an acpi node, so it is necessary to probe for the table and
> register a platform_device to represent it in the kernel.
> 
> At least conceptually, the same PSP may be exposed on the PCIe bus as
> well, in which case it would be necessary to choose whether to use a PCI
> BAR or the register window defined in ASPT for communication. There is
> no advantage to using the ACPI and there are no known bare-metal systems
> that expose the ASP table, so device registration is restricted to the
> only systems known to provide an ASPT: Hyper-V VMs. Hyper-V VMs also do
> not expose the PSP over PCIe.
> 
> This is a skeleton device at this point, as the ccp driver is not yet
> prepared to correctly probe it. Interrupt configuration will come later
> on as well.
> 
> Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
> ---
>   arch/x86/kernel/Makefile |  2 +-
>   arch/x86/kernel/psp.c    | 39 +++++++++++++++++++++++++++++++++++++++

Based on comments about other SEV related items, this should probably be 
moved into the arch/x86/coco/sev/ directory.

Thanks,
Tom

>   2 files changed, 40 insertions(+), 1 deletion(-)
>   create mode 100644 arch/x86/kernel/psp.c
> 
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index f901658d9f7c..e2e19f2d08a7 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -139,7 +139,7 @@ obj-$(CONFIG_UNWINDER_ORC)		+= unwind_orc.o
>   obj-$(CONFIG_UNWINDER_FRAME_POINTER)	+= unwind_frame.o
>   obj-$(CONFIG_UNWINDER_GUESS)		+= unwind_guess.o
>   
> -obj-$(CONFIG_AMD_MEM_ENCRYPT)		+= sev.o
> +obj-$(CONFIG_AMD_MEM_ENCRYPT)		+= psp.o sev.o
>   
>   obj-$(CONFIG_CFI_CLANG)			+= cfi.o
>   
> diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c
> new file mode 100644
> index 000000000000..d404df47cc04
> --- /dev/null
> +++ b/arch/x86/kernel/psp.c
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/platform_data/psp.h>
> +#include <linux/platform_device.h>
> +#include <asm/hypervisor.h>
> +
> +static struct platform_device psp_device = {
> +	.name           = "psp",
> +	.id             = PLATFORM_DEVID_NONE,
> +};
> +
> +static int __init psp_init_platform_device(void)
> +{
> +	struct psp_platform_data pdata = {};
> +	struct resource res[1];
> +	int err;
> +
> +	/*
> +	 * The ACPI PSP interface is mutually exclusive with the PCIe interface,
> +	 * but there is no reason to use the ACPI interface over the PCIe one.
> +	 * Restrict probing ACPI PSP to platforms known to only expose the ACPI
> +	 * interface, which at this time is SNP-host capable Hyper-V VMs.
> +	 */
> +	if (!hypervisor_is_type(X86_HYPER_MS_HYPERV))
> +		return -ENODEV;
> +
> +	err = acpi_parse_aspt(res, &pdata);
> +	if (err)
> +		return err;
> +	err = platform_device_add_resources(&psp_device, res, 1);
> +	if (err)
> +		return err;
> +
> +	err = platform_device_register(&psp_device);
> +	if (err)
> +		return err;
> +	return 0;
> +}
> +device_initcall(psp_init_platform_device);

Re: [PATCH v1 4/8] x86/psp: Add IRQ support

[Tom Lendacky]

On 1/23/23 09:22, Jeremi Piotrowski wrote:
> The ACPI PSP device provides a mailbox irq that needs to be configured
> through the ACPI mailbox register first. This requires passing a CPU
> vector and physical CPU id and then enabling interrupt delivery.
> Allocate the irq directly from the default irq domain
> (x86_vector_domain) to get access to the required information. By
> passing a cpumask through irq_alloc_info the vector is immediately
> allocated (and not later during activation) and can be retrieved.
> 
> Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
> ---
>   arch/x86/kernel/psp.c | 179 +++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 175 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c
> index d404df47cc04..24181d132bae 100644
> --- a/arch/x86/kernel/psp.c
> +++ b/arch/x86/kernel/psp.c
> @@ -1,8 +1,176 @@
>   // SPDX-License-Identifier: GPL-2.0-only
> -
> +#define pr_fmt(fmt) "psp: " fmt
>   #include <linux/platform_data/psp.h>
>   #include <linux/platform_device.h>
> +#include <linux/iopoll.h>
> +#include <linux/irq.h>
>   #include <asm/hypervisor.h>
> +#include <asm/irqdomain.h>
> +
> +#define PSP_ACPI_DATA_SHIFT 0
> +#define PSP_ACPI_DATA_MASK GENMASK(15, 0)
> +#define PSP_ACPI_CMDID_SHIFT 16
> +#define PSP_ACPI_CMDID_MASK GENMASK(25, 16)
> +#define PSP_ACPI_STATUS_SHIFT 26
> +#define PSP_ACPI_STATUS_MASK GENMASK(30, 26)
> +#define PSP_ACPI_RESPONSE_BIT BIT(31)
> +
> +#define PSP_ACPI_VECTOR_SHIFT 0
> +#define PSP_ACPI_VECTOR_MASK GENMASK(7, 0)
> +#define PSP_ACPI_MBOX_IRQID_SHIFT 10
> +#define PSP_ACPI_MBOX_IRQID_MASK GENMASK(15, 10)
> +
> +#define PSP_ACPI_IRQ_EN_BIT BIT(0)
> +#define PSP_ACPI_IRQ_EN_MBOX_IRQID_SHIFT 10
> +#define PSP_ACPI_IRQ_EN_MBOX_IRQID_MASK GENMASK(15, 10)
> +
> +// AMD Secure Processor
> +enum ASP_CMDID {
> +	ASP_CMDID_PART1  = 0x82,
> +	ASP_CMDID_PART2  = 0x83,
> +	ASP_CMDID_PART3  = 0x84,
> +	ASP_CMDID_IRQ_EN = 0x85,
> +};
> +
> +enum ASP_CMD_STATUS {
> +	ASP_CMD_STATUS_SUCCESS = 0x0,
> +	ASP_CMD_STATUS_INVALID_CMD = 0x1,
> +	ASP_CMD_STATUS_INVALID_PARAM = 0x2,
> +	ASP_CMD_STATUS_INVALID_FW_STATE = 0x3,
> +	ASP_CMD_STATUS_FAILURE = 0x1F,
> +};
> +
> +struct psp_irq_data {
> +	void __iomem *base;
> +	int mbox_irq_id;
> +	int acpi_cmd_resp_reg;
> +};
> +
> +static int psp_sync_cmd(void __iomem *reg, u8 cmd, u16 data)
> +{
> +	u32 val = 0;

Remove this initialization and ...

> +	int err;
> +
> +	val |= data & PSP_ACPI_DATA_MASK;

... just make this "val = " which sets the initial value. And if the mask 
is 16 bits and the data is 16 bits, there's no need for the ANDing

> +	val |= (cmd << PSP_ACPI_CMDID_SHIFT) & PSP_ACPI_CMDID_MASK;

Same here, the mask is 8 bits and the cmd is 8 bits...

> +	writel(val, reg);
> +	err = readl_poll_timeout_atomic(reg, val, val & PSP_ACPI_RESPONSE_BIT, 2, 10000);

Use some #defines with informative names for the 2 and 10000.

> +	if (err < 0)

This can just be "if (err)" since readl_poll_timeout_atomic() returns 0 or 
-ETIMEDOUT.

> +		return err;

Blank line...

> +	return (val & PSP_ACPI_STATUS_MASK) >> PSP_ACPI_STATUS_SHIFT;
> +}
> +
> +static int psp_set_irq_enable(struct psp_irq_data *data, bool irq_en)
> +{
> +	void __iomem *reg = data->base + data->acpi_cmd_resp_reg;
> +	u8 mbox_irq_id = data->mbox_irq_id;

Why not just make the mbox_irq_id value a u8 in the struct and eliminate 
this variable (here and in psp_configure_irq()).

> +	u16 val = 0;

Remove this initialization and ...
> +	int err;
> +
> +	val |= irq_en ? PSP_ACPI_IRQ_EN_BIT : 0;

... just make this "val = " which sets the initial value.

> +	val |= (mbox_irq_id << PSP_ACPI_IRQ_EN_MBOX_IRQID_SHIFT) & PSP_ACPI_IRQ_EN_MBOX_IRQID_MASK;

This could possibly truncate the value specified. It would be better to 
check for the max value that mbox_irq_id should be (6-bits == 63) and if 
greater, return an error. Then you don't need to do the ANDing.

> +	err = psp_sync_cmd(reg, ASP_CMDID_IRQ_EN, val);
> +	if (err != ASP_CMD_STATUS_SUCCESS) {
> +		pr_err("ASP_CMDID_IRQ_EN failed: %d\n", err);
> +		return -EIO;
> +	}

Blank line...

> +	return 0;
> +}
> +
> +static int psp_configure_irq(struct psp_irq_data *data, int vector, unsigned int cpu)
> +{
> +	void __iomem *reg = data->base + data->acpi_cmd_resp_reg;
> +	unsigned int dest_cpu = cpu_physical_id(cpu);
> +	u8 mbox_irq_id = data->mbox_irq_id;
> +	u16 part1, part2, part3;
> +	int err;
> +
> +	part1 = dest_cpu;
> +	part2 = dest_cpu >> 16;
> +	part3 = vector & PSP_ACPI_VECTOR_MASK;
> +	part3 |= (mbox_irq_id << PSP_ACPI_MBOX_IRQID_SHIFT) & PSP_ACPI_MBOX_IRQID_MASK;

Same comment about mbox_irq_id as above.

> +
> +	err = psp_sync_cmd(reg, ASP_CMDID_PART1, part1);
> +	if (err != ASP_CMD_STATUS_SUCCESS) {
> +		pr_err("ASP_CMDID_PART1 failed: %d\n", err);
> +		return -EIO;
> +	}
> +	err = psp_sync_cmd(reg, ASP_CMDID_PART2, part2);
> +	if (err != ASP_CMD_STATUS_SUCCESS) {
> +		pr_err("ASP_CMDID_PART2 failed: %d\n", err);
> +		return -EIO;
> +	}
> +	err = psp_sync_cmd(reg, ASP_CMDID_PART3, part3);
> +	if (err != ASP_CMD_STATUS_SUCCESS) {
> +		pr_err("ASP_CMDID_PART3 failed: %d\n", err);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +
> +static int psp_init_irq(const struct psp_platform_data *pdata,
> +	const struct resource *reg, struct resource *irq)

This needs to be aligned under the first parameter of the line above.

> +{
> +	struct psp_irq_data pspirqd;
> +	struct irq_alloc_info info;
> +	struct irq_data *data = NULL;
> +	struct irq_cfg *cfg = NULL;
> +	void __iomem *base = NULL;

Why are these being initialized to NULL, they aren't check or returned 
before being set.

> +	int virq;
> +	int err;
> +
> +	base = ioremap(reg->start, resource_size(reg));
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);

ioremap() returns NULL or a valid address, so the above check isn't necessary.

> +	if (!base)
> +		return -ENOMEM;

Add a blank line here.

> +	pspirqd.mbox_irq_id = pdata->mbox_irq_id;
> +	pspirqd.acpi_cmd_resp_reg = pdata->acpi_cmd_resp_reg;
> +	pspirqd.base = base;

Add a blank line here.

> +	init_irq_alloc_info(&info, cpumask_of(0));
> +	virq = irq_domain_alloc_irqs(NULL, 1, NUMA_NO_NODE, &info);
> +	if (virq <= 0) {
> +		pr_err("failed to allocate vector: %d\n", virq);
> +		err = -ENOMEM;
> +		goto unmap;
> +	}
> +	irq_set_handler(virq, handle_edge_irq);

Blank line...

> +	data = irq_get_irq_data(virq);
> +	if (!data) {
> +		pr_err("no irq data\n");
> +		err = -ENODEV;
> +		goto freeirq;
> +
> +	}

Blank line...

> +	cfg = irqd_cfg(data);
> +	if (!data) {

s/data/cfg/

> +		pr_err("no irq cfg\n");
> +		err = -ENODEV;
> +		goto freeirq;
> +	}

Blank line...

> +	err = psp_configure_irq(&pspirqd, cfg->vector, 0);
> +	if (err) {
> +		pr_err("failed to configure irq: %d\n", err);
> +		goto freeirq;
> +	}

Blank line...

> +	err = psp_set_irq_enable(&pspirqd, true);
> +	if (err) {
> +		pr_err("failed to enable irq: %d\n", err);
> +		goto freeirq;
> +	}
> +
> +	*irq = (struct resource)DEFINE_RES_IRQ(virq);

Blank line...

> +	iounmap(base);

Blank line...

> +	return 0;
> +
> +freeirq:
> +	irq_domain_free_irqs(virq, 1);

Blank line...

> +unmap:
> +	iounmap(base);

Blank line...

Thanks,
Tom

> +	return err;
> +}
>   
>   static struct platform_device psp_device = {
>   	.name           = "psp",
> @@ -12,7 +180,7 @@ static struct platform_device psp_device = {
>   static int __init psp_init_platform_device(void)
>   {
>   	struct psp_platform_data pdata = {};
> -	struct resource res[1];
> +	struct resource res[2];
>   	int err;
>   
>   	/*
> @@ -24,10 +192,13 @@ static int __init psp_init_platform_device(void)
>   	if (!hypervisor_is_type(X86_HYPER_MS_HYPERV))
>   		return -ENODEV;
>   
> -	err = acpi_parse_aspt(res, &pdata);
> +	err = acpi_parse_aspt(&res[0], &pdata);
> +	if (err)
> +		return err;
> +	err = psp_init_irq(&pdata, &res[0], &res[1]);
>   	if (err)
>   		return err;
> -	err = platform_device_add_resources(&psp_device, res, 1);
> +	err = platform_device_add_resources(&psp_device, res, 2);
>   	if (err)
>   		return err;
>   

Re: [PATCH v1 5/8] crypto: cpp - Bind to psp platform device on x86

[Tom Lendacky]

On 1/23/23 09:22, Jeremi Piotrowski wrote:
> The PSP in Hyper-V VMs is exposed through the ASP ACPI table and is
> represented as a platform_device. Allow the ccp driver to bind to it by
> adding an id_table and initing the platform_driver also on x86. At this
> point probe is called for the psp device but init fails due to missing
> driver data.
> 
> Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
> ---
>   drivers/crypto/ccp/sp-dev.c      | 8 ++++++--
>   drivers/crypto/ccp/sp-platform.c | 7 +++++++
>   2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c
> index 7eb3e4668286..52b8d957d0f6 100644
> --- a/drivers/crypto/ccp/sp-dev.c
> +++ b/drivers/crypto/ccp/sp-dev.c
> @@ -258,7 +258,11 @@ static int __init sp_mod_init(void)
>   	ret = sp_pci_init();
>   	if (ret)
>   		return ret;
> -
Please keep the blank line here.

> +	ret = sp_platform_init();
> +	if (ret) {
> +		sp_pci_exit();
> +		return ret;
> +	}

Add a blank line here.

>   #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>   	psp_pci_init();
>   #endif
> @@ -286,7 +290,7 @@ static void __exit sp_mod_exit(void)
>   #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>   	psp_pci_exit();
>   #endif
> -

Please keep the blank line here.

> +	sp_platform_exit(); >   	sp_pci_exit();
>   #endif
>   
> diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
> index 7d79a8744f9a..ea8926e87981 100644
> --- a/drivers/crypto/ccp/sp-platform.c
> +++ b/drivers/crypto/ccp/sp-platform.c
> @@ -56,6 +56,12 @@ static const struct of_device_id sp_of_match[] = {
>   MODULE_DEVICE_TABLE(of, sp_of_match);
>   #endif
>   
> +static const struct platform_device_id sp_plat_match[] = {

s/plat/platform/

Thanks,
Tom

> +	{ "psp" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(platform, sp_plat_match);
> +
>   static struct sp_dev_vdata *sp_get_of_version(struct platform_device *pdev)
>   {
>   #ifdef CONFIG_OF
> @@ -212,6 +218,7 @@ static int sp_platform_resume(struct platform_device *pdev)
>   #endif
>   
>   static struct platform_driver sp_platform_driver = {
> +	.id_table = sp_plat_match,
>   	.driver = {
>   		.name = "ccp",
>   #ifdef CONFIG_ACPI

Re: [PATCH v1 6/8] crypto: ccp - Add vdata for platform device

[Tom Lendacky]

On 1/23/23 09:22, Jeremi Piotrowski wrote:
> When matching the "psp" platform_device, determine the register offsets
> at runtime from the ASP ACPI table. Pass the parsed register offsets
> from the ASPT through platdata.
> 
> To support this scenario, mark the members of 'struct sev_vdata' and
> 'struct psp_vdata' non-const so that the probe function can write the
> values. This does not affect the other users of sev_vdata/psp_vdata as
> they define the whole struct const and the pointer in struct
> sp_dev_vdata stays const too.
> 
> Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
> ---
>   arch/x86/kernel/psp.c            |  3 ++
>   drivers/crypto/ccp/sp-dev.h      | 12 +++----
>   drivers/crypto/ccp/sp-platform.c | 57 +++++++++++++++++++++++++++++++-
>   3 files changed, 65 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c
> index 24181d132bae..68511a14df63 100644
> --- a/arch/x86/kernel/psp.c
> +++ b/arch/x86/kernel/psp.c
> @@ -199,6 +199,9 @@ static int __init psp_init_platform_device(void)
>   	if (err)
>   		return err;
>   	err = platform_device_add_resources(&psp_device, res, 2);
> +	if (err)
> +		return err;
> +	err = platform_device_add_data(&psp_device, &pdata, sizeof(pdata));
>   	if (err)
>   		return err;
>   
> diff --git a/drivers/crypto/ccp/sp-dev.h b/drivers/crypto/ccp/sp-dev.h
> index 20377e67f65d..aaa651364425 100644
> --- a/drivers/crypto/ccp/sp-dev.h
> +++ b/drivers/crypto/ccp/sp-dev.h
> @@ -40,9 +40,9 @@ struct ccp_vdata {
>   };
>   
>   struct sev_vdata {
> -	const unsigned int cmdresp_reg;
> -	const unsigned int cmdbuff_addr_lo_reg;
> -	const unsigned int cmdbuff_addr_hi_reg;
> +	unsigned int cmdresp_reg;
> +	unsigned int cmdbuff_addr_lo_reg;
> +	unsigned int cmdbuff_addr_hi_reg;
>   };
>   
>   struct tee_vdata {
> @@ -56,9 +56,9 @@ struct tee_vdata {
>   struct psp_vdata {
>   	const struct sev_vdata *sev;
>   	const struct tee_vdata *tee;
> -	const unsigned int feature_reg;
> -	const unsigned int inten_reg;
> -	const unsigned int intsts_reg;
> +	unsigned int feature_reg;
> +	unsigned int inten_reg;
> +	unsigned int intsts_reg;
>   };
>   
>   /* Structure to hold SP device data */
> diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
> index ea8926e87981..281dbf6b150c 100644
> --- a/drivers/crypto/ccp/sp-platform.c
> +++ b/drivers/crypto/ccp/sp-platform.c
> @@ -22,6 +22,7 @@
>   #include <linux/of.h>
>   #include <linux/of_address.h>
>   #include <linux/acpi.h>
> +#include <linux/platform_data/psp.h>
>   
>   #include "ccp-dev.h"
>   
> @@ -30,11 +31,31 @@ struct sp_platform {
>   	unsigned int irq_count;
>   };
>   
> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
> +static struct sev_vdata sev_platform = {
> +	.cmdresp_reg = -1,
> +	.cmdbuff_addr_lo_reg = -1,
> +	.cmdbuff_addr_hi_reg = -1,
> +};
> +static struct psp_vdata psp_platform = {
> +	.sev = &sev_platform,
> +	.feature_reg = -1,
> +	.inten_reg = -1,
> +	.intsts_reg = -1,
> +};
> +#endif
> +
>   static const struct sp_dev_vdata dev_vdata[] = {
>   	{
>   		.bar = 0,
>   #ifdef CONFIG_CRYPTO_DEV_SP_CCP
>   		.ccp_vdata = &ccpv3_platform,
> +#endif
> +	},
> +	{
> +		.bar = 0,
> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
> +		.psp_vdata = &psp_platform,
>   #endif
>   	},
>   };
> @@ -57,7 +78,7 @@ MODULE_DEVICE_TABLE(of, sp_of_match);
>   #endif
>   
>   static const struct platform_device_id sp_plat_match[] = {
> -	{ "psp" },
> +	{ "psp", (kernel_ulong_t)&dev_vdata[1] },
>   	{ },
>   };
>   MODULE_DEVICE_TABLE(platform, sp_plat_match);
> @@ -86,6 +107,38 @@ static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev)
>   	return NULL;
>   }
>   
> +static struct sp_dev_vdata *sp_get_plat_version(struct platform_device *pdev)
> +{
> +	struct sp_dev_vdata *drvdata = (struct sp_dev_vdata *)pdev->id_entry->driver_data;

s/drvdata/vdata/

> +	struct device *dev = &pdev->dev;
> +

Should check for null vdata and return NULL, e.g.:

	if (!vdata)
		return NULL;

> +	if (drvdata == &dev_vdata[1]) {

This should be a check for vdata->psp_vdata being non-NULL and 
vdata->psp_vdata->sev being non-NULL, e.g.:

	if (vdata->psp_vdata && vdata->psp_vdata->sev) {

> +		struct psp_platform_data *pdata = dev_get_platdata(dev);
> +
> +		if (!pdata) {
> +			dev_err(dev, "missing platform data\n");
> +			return NULL;
> +		}
> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP

No need for this with the above checks

> +		psp_platform.feature_reg = pdata->feature_reg;

These should then be:

		vdata->psp_vdata->inten_reg = pdata->feature_reg;
		...

> +		psp_platform.inten_reg = pdata->irq_en_reg;
> +		psp_platform.intsts_reg = pdata->irq_st_reg;
> +		sev_platform.cmdresp_reg = pdata->sev_cmd_resp_reg;

And this should be:

		vdata->psp_vdata->sev->cmdbuff_addr_lo = ...

> +		sev_platform.cmdbuff_addr_lo_reg = pdata->sev_cmd_buf_lo_reg;
> +		sev_platform.cmdbuff_addr_hi_reg = pdata->sev_cmd_buf_hi_reg;
> +		dev_dbg(dev, "GLBL feature:\t%x\n", pdata->feature_reg);

s/GLBL feature/PSP feature register/

> +		dev_dbg(dev, "GLBL irq en:\t%x\n", pdata->irq_en_reg);

s/GLBL irq en/PSP IRQ enable register/

> +		dev_dbg(dev, "GLBL irq st:\t%x\n", pdata->irq_st_reg);

s/GLBL irq st/PSP IRQ status register/

> +		dev_dbg(dev, "SEV cmdresp:\t%x\n", pdata->sev_cmd_resp_reg);

s/SEV cmdresp/SEV cmdresp register/

> +		dev_dbg(dev, "SEV cmdbuf lo:\t%x\n", pdata->sev_cmd_buf_lo_reg);

s/SEV cmdbuf lo/SEV cmdbuf lo register/

> +		dev_dbg(dev, "SEV cmdbuf hi:\t%x\n", pdata->sev_cmd_buf_hi_reg);

s/SEV cmdbuf hi/SEV cmdbuf hi register/

> +		dev_dbg(dev, "SEV mbox:\t%x\n", pdata->mbox_irq_id);

s/SEV mbox/SEV cmdresp IRQ/


> +		dev_dbg(dev, "ACPI cmdresp:\t%x\n", pdata->acpi_cmd_resp_reg);

Duplicate entry

> +#endif
> +	}
> +	return drvdata;
> +}
> +
>   static int sp_get_irqs(struct sp_device *sp)
>   {
>   	struct sp_platform *sp_platform = sp->dev_specific;
> @@ -137,6 +190,8 @@ static int sp_platform_probe(struct platform_device *pdev)
>   	sp->dev_specific = sp_platform;
>   	sp->dev_vdata = pdev->dev.of_node ? sp_get_of_version(pdev)
>   					 : sp_get_acpi_version(pdev);
> +	if (!sp->dev_vdata && pdev->id_entry)

Move this pdev->id_entry check into sp_get_plat_version(), returning NULL 
if not set.

Thanks,
Tom

> +		sp->dev_vdata = sp_get_plat_version(pdev);
>   	if (!sp->dev_vdata) {
>   		ret = -ENODEV;
>   		dev_err(dev, "missing driver data\n");

Re: [PATCH v1 7/8] crypto: ccp - Skip DMA coherency check for platform psp

[Tom Lendacky]

On 1/23/23 09:22, Jeremi Piotrowski wrote:
> The value of device_get_dma_attr() is only relevenat for ARM64 and CCP
> devices to configure the value of the axcache attribute used to access
> memory by the coprocessor. None of this applies to the platform psp so
> skip it.
> 
> Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
> ---
>   drivers/crypto/ccp/sp-platform.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
> index 281dbf6b150c..b74f16e0e963 100644
> --- a/drivers/crypto/ccp/sp-platform.c
> +++ b/drivers/crypto/ccp/sp-platform.c
> @@ -29,6 +29,7 @@
>   struct sp_platform {
>   	int coherent;
>   	unsigned int irq_count;
> +	bool is_platform;

s/is_platform/is_platform_device/

>   };
>   
>   #ifdef CONFIG_CRYPTO_DEV_SP_PSP
> @@ -190,8 +191,10 @@ static int sp_platform_probe(struct platform_device *pdev)
>   	sp->dev_specific = sp_platform;
>   	sp->dev_vdata = pdev->dev.of_node ? sp_get_of_version(pdev)
>   					 : sp_get_acpi_version(pdev);
> -	if (!sp->dev_vdata && pdev->id_entry)
> +	if (!sp->dev_vdata && pdev->id_entry) {
> +		sp_platform->is_platform = true;

Move this into the sp_get_plat_version() function.

>   		sp->dev_vdata = sp_get_plat_version(pdev);

And I probably should have made this comment in the previous patch, but 
you should probably spell out platform here.

> +	}
>   	if (!sp->dev_vdata) {
>   		ret = -ENODEV;
>   		dev_err(dev, "missing driver data\n");
> @@ -205,7 +208,7 @@ static int sp_platform_probe(struct platform_device *pdev)
>   	}
>   
>   	attr = device_get_dma_attr(dev);
> -	if (attr == DEV_DMA_NOT_SUPPORTED) {
> +	if (!sp_platform->is_platform && attr == DEV_DMA_NOT_SUPPORTED) {

Just a nit but I'd prefer to see this as:

	if (attr == DEV_DMA_NOT_SUPPORTED && !sp_platform->is_platform) {

The diff is easier to see that way.

Thanks,
Tom

>   		dev_err(dev, "DMA is not supported");
>   		goto e_err;
>   	}

Re: [PATCH v1 3/8] x86/psp: Register PSP platform device when ASP table is present

[Jeremi Piotrowski]

On Tue, Jan 31, 2023 at 12:49:54PM -0600, Tom Lendacky wrote:
> On 1/23/23 09:22, Jeremi Piotrowski wrote:
> >The ASP table contains the memory location of the register window for
> >communication with the Platform Security Processor. The device is not
> >exposed as an acpi node, so it is necessary to probe for the table and
> >register a platform_device to represent it in the kernel.
> >
> >At least conceptually, the same PSP may be exposed on the PCIe bus as
> >well, in which case it would be necessary to choose whether to use a PCI
> >BAR or the register window defined in ASPT for communication. There is
> >no advantage to using the ACPI and there are no known bare-metal systems
> >that expose the ASP table, so device registration is restricted to the
> >only systems known to provide an ASPT: Hyper-V VMs. Hyper-V VMs also do
> >not expose the PSP over PCIe.
> >
> >This is a skeleton device at this point, as the ccp driver is not yet
> >prepared to correctly probe it. Interrupt configuration will come later
> >on as well.
> >
> >Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
> >---
> >  arch/x86/kernel/Makefile |  2 +-
> >  arch/x86/kernel/psp.c    | 39 +++++++++++++++++++++++++++++++++++++++
> 
> Based on comments about other SEV related items, this should
> probably be moved into the arch/x86/coco/sev/ directory.
> 
> Thanks,
> Tom

I'll do that. This will make the code depend on CONFIG_ARCH_HAS_CC_PLATFORM
and CONFIG_AMD_MEM_ENCRYPT, the latter selects the former. This will work
as long as CONFIG_AMD_MEM_ENCRYPT continues to be needed for both SNP guest
and host sides.

Jeremi

> 
> >  2 files changed, 40 insertions(+), 1 deletion(-)
> >  create mode 100644 arch/x86/kernel/psp.c
> >
> >diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> >index f901658d9f7c..e2e19f2d08a7 100644
> >--- a/arch/x86/kernel/Makefile
> >+++ b/arch/x86/kernel/Makefile
> >@@ -139,7 +139,7 @@ obj-$(CONFIG_UNWINDER_ORC)		+= unwind_orc.o
> >  obj-$(CONFIG_UNWINDER_FRAME_POINTER)	+= unwind_frame.o
> >  obj-$(CONFIG_UNWINDER_GUESS)		+= unwind_guess.o
> >-obj-$(CONFIG_AMD_MEM_ENCRYPT)		+= sev.o
> >+obj-$(CONFIG_AMD_MEM_ENCRYPT)		+= psp.o sev.o
> >  obj-$(CONFIG_CFI_CLANG)			+= cfi.o
> >diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c
> >new file mode 100644
> >index 000000000000..d404df47cc04
> >--- /dev/null
> >+++ b/arch/x86/kernel/psp.c
> >@@ -0,0 +1,39 @@
> >+// SPDX-License-Identifier: GPL-2.0-only
> >+
> >+#include <linux/platform_data/psp.h>
> >+#include <linux/platform_device.h>
> >+#include <asm/hypervisor.h>
> >+
> >+static struct platform_device psp_device = {
> >+	.name           = "psp",
> >+	.id             = PLATFORM_DEVID_NONE,
> >+};
> >+
> >+static int __init psp_init_platform_device(void)
> >+{
> >+	struct psp_platform_data pdata = {};
> >+	struct resource res[1];
> >+	int err;
> >+
> >+	/*
> >+	 * The ACPI PSP interface is mutually exclusive with the PCIe interface,
> >+	 * but there is no reason to use the ACPI interface over the PCIe one.
> >+	 * Restrict probing ACPI PSP to platforms known to only expose the ACPI
> >+	 * interface, which at this time is SNP-host capable Hyper-V VMs.
> >+	 */
> >+	if (!hypervisor_is_type(X86_HYPER_MS_HYPERV))
> >+		return -ENODEV;
> >+
> >+	err = acpi_parse_aspt(res, &pdata);
> >+	if (err)
> >+		return err;
> >+	err = platform_device_add_resources(&psp_device, res, 1);
> >+	if (err)
> >+		return err;
> >+
> >+	err = platform_device_register(&psp_device);
> >+	if (err)
> >+		return err;
> >+	return 0;
> >+}
> >+device_initcall(psp_init_platform_device);

Re: [PATCH v1 3/8] x86/psp: Register PSP platform device when ASP table is present

[Tom Lendacky]

On 2/1/23 08:09, Jeremi Piotrowski wrote:
> On Tue, Jan 31, 2023 at 12:49:54PM -0600, Tom Lendacky wrote:
>> On 1/23/23 09:22, Jeremi Piotrowski wrote:
>>> The ASP table contains the memory location of the register window for
>>> communication with the Platform Security Processor. The device is not
>>> exposed as an acpi node, so it is necessary to probe for the table and
>>> register a platform_device to represent it in the kernel.
>>>
>>> At least conceptually, the same PSP may be exposed on the PCIe bus as
>>> well, in which case it would be necessary to choose whether to use a PCI
>>> BAR or the register window defined in ASPT for communication. There is
>>> no advantage to using the ACPI and there are no known bare-metal systems
>>> that expose the ASP table, so device registration is restricted to the
>>> only systems known to provide an ASPT: Hyper-V VMs. Hyper-V VMs also do
>>> not expose the PSP over PCIe.
>>>
>>> This is a skeleton device at this point, as the ccp driver is not yet
>>> prepared to correctly probe it. Interrupt configuration will come later
>>> on as well.
>>>
>>> Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
>>> ---
>>>   arch/x86/kernel/Makefile |  2 +-
>>>   arch/x86/kernel/psp.c    | 39 +++++++++++++++++++++++++++++++++++++++
>>
>> Based on comments about other SEV related items, this should
>> probably be moved into the arch/x86/coco/sev/ directory.
>>
>> Thanks,
>> Tom
> 
> I'll do that. This will make the code depend on CONFIG_ARCH_HAS_CC_PLATFORM
> and CONFIG_AMD_MEM_ENCRYPT, the latter selects the former. This will work
> as long as CONFIG_AMD_MEM_ENCRYPT continues to be needed for both SNP guest
> and host sides.

CONFIG_AMD_MEM_ENCRYPT is only required on the guest side. It is not 
needed to launch an SEV guest of any type. I believe the latest SNP 
hypervisor patches are being updated to remove that dependency and replace 
it with CONFIG_KVM_AMD_SEV. So you'll have to figure out what you want for 
your CONFIG requirement.

Thanks,
Tom

> 
> Jeremi
> 
>>
>>>   2 files changed, 40 insertions(+), 1 deletion(-)
>>>   create mode 100644 arch/x86/kernel/psp.c
>>>
>>> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
>>> index f901658d9f7c..e2e19f2d08a7 100644
>>> --- a/arch/x86/kernel/Makefile
>>> +++ b/arch/x86/kernel/Makefile
>>> @@ -139,7 +139,7 @@ obj-$(CONFIG_UNWINDER_ORC)		+= unwind_orc.o
>>>   obj-$(CONFIG_UNWINDER_FRAME_POINTER)	+= unwind_frame.o
>>>   obj-$(CONFIG_UNWINDER_GUESS)		+= unwind_guess.o
>>> -obj-$(CONFIG_AMD_MEM_ENCRYPT)		+= sev.o
>>> +obj-$(CONFIG_AMD_MEM_ENCRYPT)		+= psp.o sev.o
>>>   obj-$(CONFIG_CFI_CLANG)			+= cfi.o
>>> diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c
>>> new file mode 100644
>>> index 000000000000..d404df47cc04
>>> --- /dev/null
>>> +++ b/arch/x86/kernel/psp.c
>>> @@ -0,0 +1,39 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +#include <linux/platform_data/psp.h>
>>> +#include <linux/platform_device.h>
>>> +#include <asm/hypervisor.h>
>>> +
>>> +static struct platform_device psp_device = {
>>> +	.name           = "psp",
>>> +	.id             = PLATFORM_DEVID_NONE,
>>> +};
>>> +
>>> +static int __init psp_init_platform_device(void)
>>> +{
>>> +	struct psp_platform_data pdata = {};
>>> +	struct resource res[1];
>>> +	int err;
>>> +
>>> +	/*
>>> +	 * The ACPI PSP interface is mutually exclusive with the PCIe interface,
>>> +	 * but there is no reason to use the ACPI interface over the PCIe one.
>>> +	 * Restrict probing ACPI PSP to platforms known to only expose the ACPI
>>> +	 * interface, which at this time is SNP-host capable Hyper-V VMs.
>>> +	 */
>>> +	if (!hypervisor_is_type(X86_HYPER_MS_HYPERV))
>>> +		return -ENODEV;
>>> +
>>> +	err = acpi_parse_aspt(res, &pdata);
>>> +	if (err)
>>> +		return err;
>>> +	err = platform_device_add_resources(&psp_device, res, 1);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	err = platform_device_register(&psp_device);
>>> +	if (err)
>>> +		return err;
>>> +	return 0;
>>> +}
>>> +device_initcall(psp_init_platform_device);

Re: [PATCH v1 6/8] crypto: ccp - Add vdata for platform device

[Jeremi Piotrowski]

On Tue, Jan 31, 2023 at 02:36:01PM -0600, Tom Lendacky wrote:
> On 1/23/23 09:22, Jeremi Piotrowski wrote:
> >When matching the "psp" platform_device, determine the register offsets
> >at runtime from the ASP ACPI table. Pass the parsed register offsets
> >from the ASPT through platdata.
> >
> >To support this scenario, mark the members of 'struct sev_vdata' and
> >'struct psp_vdata' non-const so that the probe function can write the
> >values. This does not affect the other users of sev_vdata/psp_vdata as
> >they define the whole struct const and the pointer in struct
> >sp_dev_vdata stays const too.
> >
> >Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
> >---
> >  arch/x86/kernel/psp.c            |  3 ++
> >  drivers/crypto/ccp/sp-dev.h      | 12 +++----
> >  drivers/crypto/ccp/sp-platform.c | 57 +++++++++++++++++++++++++++++++-
> >  3 files changed, 65 insertions(+), 7 deletions(-)
> >
> >diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c
> >index 24181d132bae..68511a14df63 100644
> >--- a/arch/x86/kernel/psp.c
> >+++ b/arch/x86/kernel/psp.c
> >@@ -199,6 +199,9 @@ static int __init psp_init_platform_device(void)
> >  	if (err)
> >  		return err;
> >  	err = platform_device_add_resources(&psp_device, res, 2);
> >+	if (err)
> >+		return err;
> >+	err = platform_device_add_data(&psp_device, &pdata, sizeof(pdata));
> >  	if (err)
> >  		return err;
> >diff --git a/drivers/crypto/ccp/sp-dev.h b/drivers/crypto/ccp/sp-dev.h
> >index 20377e67f65d..aaa651364425 100644
> >--- a/drivers/crypto/ccp/sp-dev.h
> >+++ b/drivers/crypto/ccp/sp-dev.h
> >@@ -40,9 +40,9 @@ struct ccp_vdata {
> >  };
> >  struct sev_vdata {
> >-	const unsigned int cmdresp_reg;
> >-	const unsigned int cmdbuff_addr_lo_reg;
> >-	const unsigned int cmdbuff_addr_hi_reg;
> >+	unsigned int cmdresp_reg;
> >+	unsigned int cmdbuff_addr_lo_reg;
> >+	unsigned int cmdbuff_addr_hi_reg;
> >  };
> >  struct tee_vdata {
> >@@ -56,9 +56,9 @@ struct tee_vdata {
> >  struct psp_vdata {
> >  	const struct sev_vdata *sev;
> >  	const struct tee_vdata *tee;
> >-	const unsigned int feature_reg;
> >-	const unsigned int inten_reg;
> >-	const unsigned int intsts_reg;
> >+	unsigned int feature_reg;
> >+	unsigned int inten_reg;
> >+	unsigned int intsts_reg;
> >  };
> >  /* Structure to hold SP device data */
> >diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
> >index ea8926e87981..281dbf6b150c 100644
> >--- a/drivers/crypto/ccp/sp-platform.c
> >+++ b/drivers/crypto/ccp/sp-platform.c
> >@@ -22,6 +22,7 @@
> >  #include <linux/of.h>
> >  #include <linux/of_address.h>
> >  #include <linux/acpi.h>
> >+#include <linux/platform_data/psp.h>
> >  #include "ccp-dev.h"
> >@@ -30,11 +31,31 @@ struct sp_platform {
> >  	unsigned int irq_count;
> >  };
> >+#ifdef CONFIG_CRYPTO_DEV_SP_PSP
> >+static struct sev_vdata sev_platform = {
> >+	.cmdresp_reg = -1,
> >+	.cmdbuff_addr_lo_reg = -1,
> >+	.cmdbuff_addr_hi_reg = -1,
> >+};
> >+static struct psp_vdata psp_platform = {
> >+	.sev = &sev_platform,
> >+	.feature_reg = -1,
> >+	.inten_reg = -1,
> >+	.intsts_reg = -1,
> >+};
> >+#endif
> >+
> >  static const struct sp_dev_vdata dev_vdata[] = {
> >  	{
> >  		.bar = 0,
> >  #ifdef CONFIG_CRYPTO_DEV_SP_CCP
> >  		.ccp_vdata = &ccpv3_platform,
> >+#endif
> >+	},
> >+	{
> >+		.bar = 0,
> >+#ifdef CONFIG_CRYPTO_DEV_SP_PSP
> >+		.psp_vdata = &psp_platform,
> >  #endif
> >  	},
> >  };
> >@@ -57,7 +78,7 @@ MODULE_DEVICE_TABLE(of, sp_of_match);
> >  #endif
> >  static const struct platform_device_id sp_plat_match[] = {
> >-	{ "psp" },
> >+	{ "psp", (kernel_ulong_t)&dev_vdata[1] },
> >  	{ },
> >  };
> >  MODULE_DEVICE_TABLE(platform, sp_plat_match);
> >@@ -86,6 +107,38 @@ static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev)
> >  	return NULL;
> >  }
> >+static struct sp_dev_vdata *sp_get_plat_version(struct platform_device *pdev)
> >+{
> >+	struct sp_dev_vdata *drvdata = (struct sp_dev_vdata *)pdev->id_entry->driver_data;
> 
> s/drvdata/vdata/
> 

ok

> >+	struct device *dev = &pdev->dev;
> >+
> 
> Should check for null vdata and return NULL, e.g.:
> 
> 	if (!vdata)
> 		return NULL;
> 

ok

> >+	if (drvdata == &dev_vdata[1]) {
> 
> This should be a check for vdata->psp_vdata being non-NULL and
> vdata->psp_vdata->sev being non-NULL, e.g.:
> 
> 	if (vdata->psp_vdata && vdata->psp_vdata->sev) {
> 
> >+		struct psp_platform_data *pdata = dev_get_platdata(dev);
> >+
> >+		if (!pdata) {
> >+			dev_err(dev, "missing platform data\n");
> >+			return NULL;
> >+		}
> >+#ifdef CONFIG_CRYPTO_DEV_SP_PSP
> 
> No need for this with the above checks
> 
> >+		psp_platform.feature_reg = pdata->feature_reg;
> 
> These should then be:
> 
> 		vdata->psp_vdata->inten_reg = pdata->feature_reg;
> 		...

I see where you're going with this and the above suggestions, but
the psp_vdata pointer is const in struct sp_dev_vdata and so is the
sev pointer in struct psp_vdata. I find these consts to be important
and doing it this way would require casting away the const. I don't
think that's worth doing.

> 
> >+		psp_platform.inten_reg = pdata->irq_en_reg;
> >+		psp_platform.intsts_reg = pdata->irq_st_reg;
> >+		sev_platform.cmdresp_reg = pdata->sev_cmd_resp_reg;
> 
> And this should be:
> 
> 		vdata->psp_vdata->sev->cmdbuff_addr_lo = ...
> 
> >+		sev_platform.cmdbuff_addr_lo_reg = pdata->sev_cmd_buf_lo_reg;
> >+		sev_platform.cmdbuff_addr_hi_reg = pdata->sev_cmd_buf_hi_reg;
> >+		dev_dbg(dev, "GLBL feature:\t%x\n", pdata->feature_reg);
> 
> s/GLBL feature/PSP feature register/
> 
> >+		dev_dbg(dev, "GLBL irq en:\t%x\n", pdata->irq_en_reg);
> 
> s/GLBL irq en/PSP IRQ enable register/
> 
> >+		dev_dbg(dev, "GLBL irq st:\t%x\n", pdata->irq_st_reg);
> 
> s/GLBL irq st/PSP IRQ status register/
> 
> >+		dev_dbg(dev, "SEV cmdresp:\t%x\n", pdata->sev_cmd_resp_reg);
> 
> s/SEV cmdresp/SEV cmdresp register/
> 
> >+		dev_dbg(dev, "SEV cmdbuf lo:\t%x\n", pdata->sev_cmd_buf_lo_reg);
> 
> s/SEV cmdbuf lo/SEV cmdbuf lo register/
> 
> >+		dev_dbg(dev, "SEV cmdbuf hi:\t%x\n", pdata->sev_cmd_buf_hi_reg);
> 
> s/SEV cmdbuf hi/SEV cmdbuf hi register/
> 
> >+		dev_dbg(dev, "SEV mbox:\t%x\n", pdata->mbox_irq_id);
> 
> s/SEV mbox/SEV cmdresp IRQ/
> 

ok to all the above rewordings

> 
> >+		dev_dbg(dev, "ACPI cmdresp:\t%x\n", pdata->acpi_cmd_resp_reg);
> 
> Duplicate entry

I don't see it. This is the ACPI register (the one used for the IRQ config).
Here's how these prints look when the module is loaded with dyndbg=+p:

  ccp psp: GLBL feature:  0
  ccp psp: GLBL irq en:   4
  ccp psp: GLBL irq st:   8
  ccp psp: SEV cmdresp:   10
  ccp psp: SEV cmdbuf lo: 14
  ccp psp: SEV cmdbuf hi: 18
  ccp psp: SEV mbox:      1
  ccp psp: ACPI cmdresp:  20

> 
> >+#endif
> >+	}
> >+	return drvdata;
> >+}
> >+
> >  static int sp_get_irqs(struct sp_device *sp)
> >  {
> >  	struct sp_platform *sp_platform = sp->dev_specific;
> >@@ -137,6 +190,8 @@ static int sp_platform_probe(struct platform_device *pdev)
> >  	sp->dev_specific = sp_platform;
> >  	sp->dev_vdata = pdev->dev.of_node ? sp_get_of_version(pdev)
> >  					 : sp_get_acpi_version(pdev);
> >+	if (!sp->dev_vdata && pdev->id_entry)
> 
> Move this pdev->id_entry check into sp_get_plat_version(), returning
> NULL if not set.
> 

ok

> Thanks,
> Tom
> 
> >+		sp->dev_vdata = sp_get_plat_version(pdev);
> >  	if (!sp->dev_vdata) {
> >  		ret = -ENODEV;
> >  		dev_err(dev, "missing driver data\n");

Re: [PATCH v1 6/8] crypto: ccp - Add vdata for platform device

[Tom Lendacky]

On 2/1/23 13:24, Jeremi Piotrowski wrote:
> On Tue, Jan 31, 2023 at 02:36:01PM -0600, Tom Lendacky wrote:
>> On 1/23/23 09:22, Jeremi Piotrowski wrote:
>>> When matching the "psp" platform_device, determine the register offsets
>>> at runtime from the ASP ACPI table. Pass the parsed register offsets
>> >from the ASPT through platdata.
>>>
>>> To support this scenario, mark the members of 'struct sev_vdata' and
>>> 'struct psp_vdata' non-const so that the probe function can write the
>>> values. This does not affect the other users of sev_vdata/psp_vdata as
>>> they define the whole struct const and the pointer in struct
>>> sp_dev_vdata stays const too.
>>>
>>> Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
>>> ---
>>>   arch/x86/kernel/psp.c            |  3 ++
>>>   drivers/crypto/ccp/sp-dev.h      | 12 +++----
>>>   drivers/crypto/ccp/sp-platform.c | 57 +++++++++++++++++++++++++++++++-
>>>   3 files changed, 65 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c
>>> index 24181d132bae..68511a14df63 100644
>>> --- a/arch/x86/kernel/psp.c
>>> +++ b/arch/x86/kernel/psp.c
>>> @@ -199,6 +199,9 @@ static int __init psp_init_platform_device(void)
>>>   	if (err)
>>>   		return err;
>>>   	err = platform_device_add_resources(&psp_device, res, 2);
>>> +	if (err)
>>> +		return err;
>>> +	err = platform_device_add_data(&psp_device, &pdata, sizeof(pdata));
>>>   	if (err)
>>>   		return err;
>>> diff --git a/drivers/crypto/ccp/sp-dev.h b/drivers/crypto/ccp/sp-dev.h
>>> index 20377e67f65d..aaa651364425 100644
>>> --- a/drivers/crypto/ccp/sp-dev.h
>>> +++ b/drivers/crypto/ccp/sp-dev.h
>>> @@ -40,9 +40,9 @@ struct ccp_vdata {
>>>   };
>>>   struct sev_vdata {
>>> -	const unsigned int cmdresp_reg;
>>> -	const unsigned int cmdbuff_addr_lo_reg;
>>> -	const unsigned int cmdbuff_addr_hi_reg;
>>> +	unsigned int cmdresp_reg;
>>> +	unsigned int cmdbuff_addr_lo_reg;
>>> +	unsigned int cmdbuff_addr_hi_reg;
>>>   };
>>>   struct tee_vdata {
>>> @@ -56,9 +56,9 @@ struct tee_vdata {
>>>   struct psp_vdata {
>>>   	const struct sev_vdata *sev;
>>>   	const struct tee_vdata *tee;
>>> -	const unsigned int feature_reg;
>>> -	const unsigned int inten_reg;
>>> -	const unsigned int intsts_reg;
>>> +	unsigned int feature_reg;
>>> +	unsigned int inten_reg;
>>> +	unsigned int intsts_reg;
>>>   };
>>>   /* Structure to hold SP device data */
>>> diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
>>> index ea8926e87981..281dbf6b150c 100644
>>> --- a/drivers/crypto/ccp/sp-platform.c
>>> +++ b/drivers/crypto/ccp/sp-platform.c
>>> @@ -22,6 +22,7 @@
>>>   #include <linux/of.h>
>>>   #include <linux/of_address.h>
>>>   #include <linux/acpi.h>
>>> +#include <linux/platform_data/psp.h>
>>>   #include "ccp-dev.h"
>>> @@ -30,11 +31,31 @@ struct sp_platform {
>>>   	unsigned int irq_count;
>>>   };
>>> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>> +static struct sev_vdata sev_platform = {
>>> +	.cmdresp_reg = -1,
>>> +	.cmdbuff_addr_lo_reg = -1,
>>> +	.cmdbuff_addr_hi_reg = -1,
>>> +};
>>> +static struct psp_vdata psp_platform = {
>>> +	.sev = &sev_platform,
>>> +	.feature_reg = -1,
>>> +	.inten_reg = -1,
>>> +	.intsts_reg = -1,
>>> +};
>>> +#endif
>>> +
>>>   static const struct sp_dev_vdata dev_vdata[] = {
>>>   	{
>>>   		.bar = 0,
>>>   #ifdef CONFIG_CRYPTO_DEV_SP_CCP
>>>   		.ccp_vdata = &ccpv3_platform,
>>> +#endif
>>> +	},
>>> +	{
>>> +		.bar = 0,
>>> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>> +		.psp_vdata = &psp_platform,
>>>   #endif
>>>   	},
>>>   };
>>> @@ -57,7 +78,7 @@ MODULE_DEVICE_TABLE(of, sp_of_match);
>>>   #endif
>>>   static const struct platform_device_id sp_plat_match[] = {
>>> -	{ "psp" },
>>> +	{ "psp", (kernel_ulong_t)&dev_vdata[1] },
>>>   	{ },
>>>   };
>>>   MODULE_DEVICE_TABLE(platform, sp_plat_match);
>>> @@ -86,6 +107,38 @@ static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev)
>>>   	return NULL;
>>>   }
>>> +static struct sp_dev_vdata *sp_get_plat_version(struct platform_device *pdev)
>>> +{
>>> +	struct sp_dev_vdata *drvdata = (struct sp_dev_vdata *)pdev->id_entry->driver_data;
>>
>> s/drvdata/vdata/
>>
> 
> ok
> 
>>> +	struct device *dev = &pdev->dev;
>>> +
>>
>> Should check for null vdata and return NULL, e.g.:
>>
>> 	if (!vdata)
>> 		return NULL;
>>
> 
> ok
> 
>>> +	if (drvdata == &dev_vdata[1]) {
>>
>> This should be a check for vdata->psp_vdata being non-NULL and
>> vdata->psp_vdata->sev being non-NULL, e.g.:
>>
>> 	if (vdata->psp_vdata && vdata->psp_vdata->sev) {
>>
>>> +		struct psp_platform_data *pdata = dev_get_platdata(dev);
>>> +
>>> +		if (!pdata) {
>>> +			dev_err(dev, "missing platform data\n");
>>> +			return NULL;
>>> +		}
>>> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>
>> No need for this with the above checks
>>
>>> +		psp_platform.feature_reg = pdata->feature_reg;
>>
>> These should then be:
>>
>> 		vdata->psp_vdata->inten_reg = pdata->feature_reg;
>> 		...
> 
> I see where you're going with this and the above suggestions, but
> the psp_vdata pointer is const in struct sp_dev_vdata and so is the
> sev pointer in struct psp_vdata. I find these consts to be important
> and doing it this way would require casting away the const. I don't
> think that's worth doing.

Ok, then maybe it would be better to kmalloc a vdata structure that you 
fill in and then assign that to dev_vdata field for use. That could 
eliminate the removal of the "const" notations in one of the previous 
patches. I just don't think you should be changing the underlying module 
data that isn't expected to be changed.

> 
>>
>>> +		psp_platform.inten_reg = pdata->irq_en_reg;
>>> +		psp_platform.intsts_reg = pdata->irq_st_reg;
>>> +		sev_platform.cmdresp_reg = pdata->sev_cmd_resp_reg;
>>
>> And this should be:
>>
>> 		vdata->psp_vdata->sev->cmdbuff_addr_lo = ...
>>
>>> +		sev_platform.cmdbuff_addr_lo_reg = pdata->sev_cmd_buf_lo_reg;
>>> +		sev_platform.cmdbuff_addr_hi_reg = pdata->sev_cmd_buf_hi_reg;
>>> +		dev_dbg(dev, "GLBL feature:\t%x\n", pdata->feature_reg);
>>
>> s/GLBL feature/PSP feature register/
>>
>>> +		dev_dbg(dev, "GLBL irq en:\t%x\n", pdata->irq_en_reg);
>>
>> s/GLBL irq en/PSP IRQ enable register/
>>
>>> +		dev_dbg(dev, "GLBL irq st:\t%x\n", pdata->irq_st_reg);
>>
>> s/GLBL irq st/PSP IRQ status register/
>>
>>> +		dev_dbg(dev, "SEV cmdresp:\t%x\n", pdata->sev_cmd_resp_reg);
>>
>> s/SEV cmdresp/SEV cmdresp register/
>>
>>> +		dev_dbg(dev, "SEV cmdbuf lo:\t%x\n", pdata->sev_cmd_buf_lo_reg);
>>
>> s/SEV cmdbuf lo/SEV cmdbuf lo register/
>>
>>> +		dev_dbg(dev, "SEV cmdbuf hi:\t%x\n", pdata->sev_cmd_buf_hi_reg);
>>
>> s/SEV cmdbuf hi/SEV cmdbuf hi register/
>>
>>> +		dev_dbg(dev, "SEV mbox:\t%x\n", pdata->mbox_irq_id);
>>
>> s/SEV mbox/SEV cmdresp IRQ/
>>
> 
> ok to all the above rewordings
> 
>>
>>> +		dev_dbg(dev, "ACPI cmdresp:\t%x\n", pdata->acpi_cmd_resp_reg);
>>
>> Duplicate entry
> 
> I don't see it. This is the ACPI register (the one used for the IRQ config).
> Here's how these prints look when the module is loaded with dyndbg=+p:

My bad, misread it as SEV cmdresp.

Thanks,
Tom

> 
>    ccp psp: GLBL feature:  0
>    ccp psp: GLBL irq en:   4
>    ccp psp: GLBL irq st:   8
>    ccp psp: SEV cmdresp:   10
>    ccp psp: SEV cmdbuf lo: 14
>    ccp psp: SEV cmdbuf hi: 18
>    ccp psp: SEV mbox:      1
>    ccp psp: ACPI cmdresp:  20
> 
>>
>>> +#endif
>>> +	}
>>> +	return drvdata;
>>> +}
>>> +
>>>   static int sp_get_irqs(struct sp_device *sp)
>>>   {
>>>   	struct sp_platform *sp_platform = sp->dev_specific;
>>> @@ -137,6 +190,8 @@ static int sp_platform_probe(struct platform_device *pdev)
>>>   	sp->dev_specific = sp_platform;
>>>   	sp->dev_vdata = pdev->dev.of_node ? sp_get_of_version(pdev)
>>>   					 : sp_get_acpi_version(pdev);
>>> +	if (!sp->dev_vdata && pdev->id_entry)
>>
>> Move this pdev->id_entry check into sp_get_plat_version(), returning
>> NULL if not set.
>>
> 
> ok
> 
>> Thanks,
>> Tom
>>
>>> +		sp->dev_vdata = sp_get_plat_version(pdev);
>>>   	if (!sp->dev_vdata) {
>>>   		ret = -ENODEV;
>>>   		dev_err(dev, "missing driver data\n");

Re: [PATCH v1 6/8] crypto: ccp - Add vdata for platform device

[Jeremi Piotrowski]

On 06/02/2023 20:13, Tom Lendacky wrote:
> On 2/1/23 13:24, Jeremi Piotrowski wrote:
>> On Tue, Jan 31, 2023 at 02:36:01PM -0600, Tom Lendacky wrote:
>>> On 1/23/23 09:22, Jeremi Piotrowski wrote:
>>>> When matching the "psp" platform_device, determine the register offsets
>>>> at runtime from the ASP ACPI table. Pass the parsed register offsets
>>> >from the ASPT through platdata.
>>>>
>>>> To support this scenario, mark the members of 'struct sev_vdata' and
>>>> 'struct psp_vdata' non-const so that the probe function can write the
>>>> values. This does not affect the other users of sev_vdata/psp_vdata as
>>>> they define the whole struct const and the pointer in struct
>>>> sp_dev_vdata stays const too.
>>>>
>>>> Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
>>>> ---
>>>>   arch/x86/kernel/psp.c            |  3 ++
>>>>   drivers/crypto/ccp/sp-dev.h      | 12 +++----
>>>>   drivers/crypto/ccp/sp-platform.c | 57 +++++++++++++++++++++++++++++++-
>>>>   3 files changed, 65 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c
>>>> index 24181d132bae..68511a14df63 100644
>>>> --- a/arch/x86/kernel/psp.c
>>>> +++ b/arch/x86/kernel/psp.c
>>>> @@ -199,6 +199,9 @@ static int __init psp_init_platform_device(void)
>>>>       if (err)
>>>>           return err;
>>>>       err = platform_device_add_resources(&psp_device, res, 2);
>>>> +    if (err)
>>>> +        return err;
>>>> +    err = platform_device_add_data(&psp_device, &pdata, sizeof(pdata));
>>>>       if (err)
>>>>           return err;
>>>> diff --git a/drivers/crypto/ccp/sp-dev.h b/drivers/crypto/ccp/sp-dev.h
>>>> index 20377e67f65d..aaa651364425 100644
>>>> --- a/drivers/crypto/ccp/sp-dev.h
>>>> +++ b/drivers/crypto/ccp/sp-dev.h
>>>> @@ -40,9 +40,9 @@ struct ccp_vdata {
>>>>   };
>>>>   struct sev_vdata {
>>>> -    const unsigned int cmdresp_reg;
>>>> -    const unsigned int cmdbuff_addr_lo_reg;
>>>> -    const unsigned int cmdbuff_addr_hi_reg;
>>>> +    unsigned int cmdresp_reg;
>>>> +    unsigned int cmdbuff_addr_lo_reg;
>>>> +    unsigned int cmdbuff_addr_hi_reg;
>>>>   };
>>>>   struct tee_vdata {
>>>> @@ -56,9 +56,9 @@ struct tee_vdata {
>>>>   struct psp_vdata {
>>>>       const struct sev_vdata *sev;
>>>>       const struct tee_vdata *tee;
>>>> -    const unsigned int feature_reg;
>>>> -    const unsigned int inten_reg;
>>>> -    const unsigned int intsts_reg;
>>>> +    unsigned int feature_reg;
>>>> +    unsigned int inten_reg;
>>>> +    unsigned int intsts_reg;
>>>>   };
>>>>   /* Structure to hold SP device data */
>>>> diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
>>>> index ea8926e87981..281dbf6b150c 100644
>>>> --- a/drivers/crypto/ccp/sp-platform.c
>>>> +++ b/drivers/crypto/ccp/sp-platform.c
>>>> @@ -22,6 +22,7 @@
>>>>   #include <linux/of.h>
>>>>   #include <linux/of_address.h>
>>>>   #include <linux/acpi.h>
>>>> +#include <linux/platform_data/psp.h>
>>>>   #include "ccp-dev.h"
>>>> @@ -30,11 +31,31 @@ struct sp_platform {
>>>>       unsigned int irq_count;
>>>>   };
>>>> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>>> +static struct sev_vdata sev_platform = {
>>>> +    .cmdresp_reg = -1,
>>>> +    .cmdbuff_addr_lo_reg = -1,
>>>> +    .cmdbuff_addr_hi_reg = -1,
>>>> +};
>>>> +static struct psp_vdata psp_platform = {
>>>> +    .sev = &sev_platform,
>>>> +    .feature_reg = -1,
>>>> +    .inten_reg = -1,
>>>> +    .intsts_reg = -1,
>>>> +};
>>>> +#endif
>>>> +
>>>>   static const struct sp_dev_vdata dev_vdata[] = {
>>>>       {
>>>>           .bar = 0,
>>>>   #ifdef CONFIG_CRYPTO_DEV_SP_CCP
>>>>           .ccp_vdata = &ccpv3_platform,
>>>> +#endif
>>>> +    },
>>>> +    {
>>>> +        .bar = 0,
>>>> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>>> +        .psp_vdata = &psp_platform,
>>>>   #endif
>>>>       },
>>>>   };
>>>> @@ -57,7 +78,7 @@ MODULE_DEVICE_TABLE(of, sp_of_match);
>>>>   #endif
>>>>   static const struct platform_device_id sp_plat_match[] = {
>>>> -    { "psp" },
>>>> +    { "psp", (kernel_ulong_t)&dev_vdata[1] },
>>>>       { },
>>>>   };
>>>>   MODULE_DEVICE_TABLE(platform, sp_plat_match);
>>>> @@ -86,6 +107,38 @@ static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev)
>>>>       return NULL;
>>>>   }
>>>> +static struct sp_dev_vdata *sp_get_plat_version(struct platform_device *pdev)
>>>> +{
>>>> +    struct sp_dev_vdata *drvdata = (struct sp_dev_vdata *)pdev->id_entry->driver_data;
>>>
>>> s/drvdata/vdata/
>>>
>>
>> ok
>>
>>>> +    struct device *dev = &pdev->dev;
>>>> +
>>>
>>> Should check for null vdata and return NULL, e.g.:
>>>
>>>     if (!vdata)
>>>         return NULL;
>>>
>>
>> ok
>>
>>>> +    if (drvdata == &dev_vdata[1]) {
>>>
>>> This should be a check for vdata->psp_vdata being non-NULL and
>>> vdata->psp_vdata->sev being non-NULL, e.g.:
>>>
>>>     if (vdata->psp_vdata && vdata->psp_vdata->sev) {
>>>
>>>> +        struct psp_platform_data *pdata = dev_get_platdata(dev);
>>>> +
>>>> +        if (!pdata) {
>>>> +            dev_err(dev, "missing platform data\n");
>>>> +            return NULL;
>>>> +        }
>>>> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>>
>>> No need for this with the above checks
>>>
>>>> +        psp_platform.feature_reg = pdata->feature_reg;
>>>
>>> These should then be:
>>>
>>>         vdata->psp_vdata->inten_reg = pdata->feature_reg;
>>>         ...
>>
>> I see where you're going with this and the above suggestions, but
>> the psp_vdata pointer is const in struct sp_dev_vdata and so is the
>> sev pointer in struct psp_vdata. I find these consts to be important
>> and doing it this way would require casting away the const. I don't
>> think that's worth doing.
> 
> Ok, then maybe it would be better to kmalloc a vdata structure that you fill in and then assign that to dev_vdata field for use. That could eliminate the removal of the "const" notations in one of the previous patches. I just don't think you should be changing the underlying module data that isn't expected to be changed.
> 

I can do that and undo the removal of consts from the
struct (sev|psp)_vdata members, but the outcome would look something
like this:

static void sp_platform_fill_vdata(struct sp_dev_vdata *vdata,
					struct psp_vdata *psp,
					struct sev_vdata *sev,
					const struct psp_platform_data *pdata)
{
	struct sev_vdata sevtmp = {
		.cmdbuff_addr_hi_reg = pdata->sev_cmd_buf_hi_reg,
		.cmdbuff_addr_lo_reg = pdata->sev_cmd_buf_lo_reg,
		.cmdresp_reg = pdata->sev_cmd_resp_reg,
	};
	struct psp_vdata psptmp = {
		.feature_reg = pdata->feature_reg,
		.inten_reg = pdata->irq_en_reg,
		.intsts_reg = pdata->irq_st_reg,
		.sev = sev,
	};

	memcpy(sev, &sevtmp, sizeof(*sev));
	memcpy(psp, &psptmp, sizeof(*psp));
	vdata->psp_vdata = psp;
}

static struct sp_dev_vdata *sp_get_platform_version(struct sp_device *sp)
{
	struct sp_platform *sp_platform = sp->dev_specific;
	struct psp_platform_data *pdata;
	struct device *dev = sp->dev;
	struct sp_dev_vdata *vdata;
	struct psp_vdata *psp;
	struct sev_vdata *sev;

	pdata = dev_get_platdata(dev);
	if (!pdata) {
		dev_err(dev, "missing platform data\n");
		return NULL;
	}

	sp_platform->is_platform_device = true;

	vdata = devm_kzalloc(dev, sizeof(*vdata) + sizeof(*psp) + sizeof(*sev), GFP_KERNEL);
	if (!vdata)
		return NULL;

	psp = (void *)vdata + sizeof(*vdata);
	sev = (void *)psp + sizeof(*psp);
	sp_platform_fill_vdata(vdata, psp, sev, pdata);

	/* elided debug print */
	...

	return vdata;
}

with the const fields in the struct it's not possible to assign in any
other way than on initialization, so I need to use the helper function,
tmp structs and memcpy.

Could you ack that you like this approach before I post a v2?

Re: [PATCH v1 5/8] crypto: cpp - Bind to psp platform device on x86

[Jeremi Piotrowski]

On 31/01/2023 20:51, Tom Lendacky wrote:
> On 1/23/23 09:22, Jeremi Piotrowski wrote:
>> The PSP in Hyper-V VMs is exposed through the ASP ACPI table and is
>> represented as a platform_device. Allow the ccp driver to bind to it by
>> adding an id_table and initing the platform_driver also on x86. At this
>> point probe is called for the psp device but init fails due to missing
>> driver data.
>>
>> Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
>> ---
>>   drivers/crypto/ccp/sp-dev.c      | 8 ++++++--
>>   drivers/crypto/ccp/sp-platform.c | 7 +++++++
>>   2 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c
>> index 7eb3e4668286..52b8d957d0f6 100644
>> --- a/drivers/crypto/ccp/sp-dev.c
>> +++ b/drivers/crypto/ccp/sp-dev.c
>> @@ -258,7 +258,11 @@ static int __init sp_mod_init(void)
>>       ret = sp_pci_init();
>>       if (ret)
>>           return ret;
>> -
> Please keep the blank line here.>

ok
 
>> +    ret = sp_platform_init();
>> +    if (ret) {
>> +        sp_pci_exit();
>> +        return ret;
>> +    }
> 
> Add a blank line here.
>

ok

>>   #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>       psp_pci_init();
>>   #endif
>> @@ -286,7 +290,7 @@ static void __exit sp_mod_exit(void)
>>   #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>       psp_pci_exit();
>>   #endif
>> -
> 
> Please keep the blank line here.
>

ok

>> +    sp_platform_exit(); >       sp_pci_exit();
>>   #endif
>>   diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
>> index 7d79a8744f9a..ea8926e87981 100644
>> --- a/drivers/crypto/ccp/sp-platform.c
>> +++ b/drivers/crypto/ccp/sp-platform.c
>> @@ -56,6 +56,12 @@ static const struct of_device_id sp_of_match[] = {
>>   MODULE_DEVICE_TABLE(of, sp_of_match);
>>   #endif
>>   +static const struct platform_device_id sp_plat_match[] = {
> 
> s/plat/platform/
> 

ok

> Thanks,
> Tom
> 
>> +    { "psp" },
>> +    { },
>> +};
>> +MODULE_DEVICE_TABLE(platform, sp_plat_match);
>> +
>>   static struct sp_dev_vdata *sp_get_of_version(struct platform_device *pdev)
>>   {
>>   #ifdef CONFIG_OF
>> @@ -212,6 +218,7 @@ static int sp_platform_resume(struct platform_device *pdev)
>>   #endif
>>     static struct platform_driver sp_platform_driver = {
>> +    .id_table = sp_plat_match,
>>       .driver = {
>>           .name = "ccp",
>>   #ifdef CONFIG_ACPI

Re: [PATCH v1 7/8] crypto: ccp - Skip DMA coherency check for platform psp

[Jeremi Piotrowski]

On 31/01/2023 21:42, Tom Lendacky wrote:
> On 1/23/23 09:22, Jeremi Piotrowski wrote:
>> The value of device_get_dma_attr() is only relevenat for ARM64 and CCP
>> devices to configure the value of the axcache attribute used to access
>> memory by the coprocessor. None of this applies to the platform psp so
>> skip it.
>>
>> Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
>> ---
>>   drivers/crypto/ccp/sp-platform.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
>> index 281dbf6b150c..b74f16e0e963 100644
>> --- a/drivers/crypto/ccp/sp-platform.c
>> +++ b/drivers/crypto/ccp/sp-platform.c
>> @@ -29,6 +29,7 @@
>>   struct sp_platform {
>>       int coherent;
>>       unsigned int irq_count;
>> +    bool is_platform;
> 
> s/is_platform/is_platform_device/
> 

ok

>>   };
>>     #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>> @@ -190,8 +191,10 @@ static int sp_platform_probe(struct platform_device *pdev)
>>       sp->dev_specific = sp_platform;
>>       sp->dev_vdata = pdev->dev.of_node ? sp_get_of_version(pdev)
>>                        : sp_get_acpi_version(pdev);
>> -    if (!sp->dev_vdata && pdev->id_entry)
>> +    if (!sp->dev_vdata && pdev->id_entry) {
>> +        sp_platform->is_platform = true;
> 
> Move this into the sp_get_plat_version() function.
> 
>>           sp->dev_vdata = sp_get_plat_version(pdev);
> 
> And I probably should have made this comment in the previous patch, but you should probably spell out platform here.
>

ok (i had done that before i got to this comment for consistency)

>> +    }
>>       if (!sp->dev_vdata) {
>>           ret = -ENODEV;
>>           dev_err(dev, "missing driver data\n");
>> @@ -205,7 +208,7 @@ static int sp_platform_probe(struct platform_device *pdev)
>>       }
>>         attr = device_get_dma_attr(dev);
>> -    if (attr == DEV_DMA_NOT_SUPPORTED) {
>> +    if (!sp_platform->is_platform && attr == DEV_DMA_NOT_SUPPORTED) {
> 
> Just a nit but I'd prefer to see this as:
> 
>     if (attr == DEV_DMA_NOT_SUPPORTED && !sp_platform->is_platform) {
> 
> The diff is easier to see that way.

makes sense

> 
> Thanks,
> Tom
> 
>>           dev_err(dev, "DMA is not supported");
>>           goto e_err;
>>       }

Re: [PATCH v1 6/8] crypto: ccp - Add vdata for platform device

[Tom Lendacky]

On 2/8/23 06:45, Jeremi Piotrowski wrote:
> On 06/02/2023 20:13, Tom Lendacky wrote:
>> On 2/1/23 13:24, Jeremi Piotrowski wrote:
>>> On Tue, Jan 31, 2023 at 02:36:01PM -0600, Tom Lendacky wrote:
>>>> On 1/23/23 09:22, Jeremi Piotrowski wrote:
>>>>> When matching the "psp" platform_device, determine the register offsets
>>>>> at runtime from the ASP ACPI table. Pass the parsed register offsets
>>>> >from the ASPT through platdata.
>>>>>
>>>>> To support this scenario, mark the members of 'struct sev_vdata' and
>>>>> 'struct psp_vdata' non-const so that the probe function can write the
>>>>> values. This does not affect the other users of sev_vdata/psp_vdata as
>>>>> they define the whole struct const and the pointer in struct
>>>>> sp_dev_vdata stays const too.
>>>>>
>>>>> Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
>>>>> ---
>>>>>    arch/x86/kernel/psp.c            |  3 ++
>>>>>    drivers/crypto/ccp/sp-dev.h      | 12 +++----
>>>>>    drivers/crypto/ccp/sp-platform.c | 57 +++++++++++++++++++++++++++++++-
>>>>>    3 files changed, 65 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c
>>>>> index 24181d132bae..68511a14df63 100644
>>>>> --- a/arch/x86/kernel/psp.c
>>>>> +++ b/arch/x86/kernel/psp.c
>>>>> @@ -199,6 +199,9 @@ static int __init psp_init_platform_device(void)
>>>>>        if (err)
>>>>>            return err;
>>>>>        err = platform_device_add_resources(&psp_device, res, 2);
>>>>> +    if (err)
>>>>> +        return err;
>>>>> +    err = platform_device_add_data(&psp_device, &pdata, sizeof(pdata));
>>>>>        if (err)
>>>>>            return err;
>>>>> diff --git a/drivers/crypto/ccp/sp-dev.h b/drivers/crypto/ccp/sp-dev.h
>>>>> index 20377e67f65d..aaa651364425 100644
>>>>> --- a/drivers/crypto/ccp/sp-dev.h
>>>>> +++ b/drivers/crypto/ccp/sp-dev.h
>>>>> @@ -40,9 +40,9 @@ struct ccp_vdata {
>>>>>    };
>>>>>    struct sev_vdata {
>>>>> -    const unsigned int cmdresp_reg;
>>>>> -    const unsigned int cmdbuff_addr_lo_reg;
>>>>> -    const unsigned int cmdbuff_addr_hi_reg;
>>>>> +    unsigned int cmdresp_reg;
>>>>> +    unsigned int cmdbuff_addr_lo_reg;
>>>>> +    unsigned int cmdbuff_addr_hi_reg;
>>>>>    };
>>>>>    struct tee_vdata {
>>>>> @@ -56,9 +56,9 @@ struct tee_vdata {
>>>>>    struct psp_vdata {
>>>>>        const struct sev_vdata *sev;
>>>>>        const struct tee_vdata *tee;
>>>>> -    const unsigned int feature_reg;
>>>>> -    const unsigned int inten_reg;
>>>>> -    const unsigned int intsts_reg;
>>>>> +    unsigned int feature_reg;
>>>>> +    unsigned int inten_reg;
>>>>> +    unsigned int intsts_reg;
>>>>>    };
>>>>>    /* Structure to hold SP device data */
>>>>> diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
>>>>> index ea8926e87981..281dbf6b150c 100644
>>>>> --- a/drivers/crypto/ccp/sp-platform.c
>>>>> +++ b/drivers/crypto/ccp/sp-platform.c
>>>>> @@ -22,6 +22,7 @@
>>>>>    #include <linux/of.h>
>>>>>    #include <linux/of_address.h>
>>>>>    #include <linux/acpi.h>
>>>>> +#include <linux/platform_data/psp.h>
>>>>>    #include "ccp-dev.h"
>>>>> @@ -30,11 +31,31 @@ struct sp_platform {
>>>>>        unsigned int irq_count;
>>>>>    };
>>>>> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>>>> +static struct sev_vdata sev_platform = {
>>>>> +    .cmdresp_reg = -1,
>>>>> +    .cmdbuff_addr_lo_reg = -1,
>>>>> +    .cmdbuff_addr_hi_reg = -1,
>>>>> +};
>>>>> +static struct psp_vdata psp_platform = {
>>>>> +    .sev = &sev_platform,
>>>>> +    .feature_reg = -1,
>>>>> +    .inten_reg = -1,
>>>>> +    .intsts_reg = -1,
>>>>> +};
>>>>> +#endif
>>>>> +
>>>>>    static const struct sp_dev_vdata dev_vdata[] = {
>>>>>        {
>>>>>            .bar = 0,
>>>>>    #ifdef CONFIG_CRYPTO_DEV_SP_CCP
>>>>>            .ccp_vdata = &ccpv3_platform,
>>>>> +#endif
>>>>> +    },
>>>>> +    {
>>>>> +        .bar = 0,
>>>>> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>>>> +        .psp_vdata = &psp_platform,
>>>>>    #endif
>>>>>        },
>>>>>    };
>>>>> @@ -57,7 +78,7 @@ MODULE_DEVICE_TABLE(of, sp_of_match);
>>>>>    #endif
>>>>>    static const struct platform_device_id sp_plat_match[] = {
>>>>> -    { "psp" },
>>>>> +    { "psp", (kernel_ulong_t)&dev_vdata[1] },
>>>>>        { },
>>>>>    };
>>>>>    MODULE_DEVICE_TABLE(platform, sp_plat_match);
>>>>> @@ -86,6 +107,38 @@ static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev)
>>>>>        return NULL;
>>>>>    }
>>>>> +static struct sp_dev_vdata *sp_get_plat_version(struct platform_device *pdev)
>>>>> +{
>>>>> +    struct sp_dev_vdata *drvdata = (struct sp_dev_vdata *)pdev->id_entry->driver_data;
>>>>
>>>> s/drvdata/vdata/
>>>>
>>>
>>> ok
>>>
>>>>> +    struct device *dev = &pdev->dev;
>>>>> +
>>>>
>>>> Should check for null vdata and return NULL, e.g.:
>>>>
>>>>      if (!vdata)
>>>>          return NULL;
>>>>
>>>
>>> ok
>>>
>>>>> +    if (drvdata == &dev_vdata[1]) {
>>>>
>>>> This should be a check for vdata->psp_vdata being non-NULL and
>>>> vdata->psp_vdata->sev being non-NULL, e.g.:
>>>>
>>>>      if (vdata->psp_vdata && vdata->psp_vdata->sev) {
>>>>
>>>>> +        struct psp_platform_data *pdata = dev_get_platdata(dev);
>>>>> +
>>>>> +        if (!pdata) {
>>>>> +            dev_err(dev, "missing platform data\n");
>>>>> +            return NULL;
>>>>> +        }
>>>>> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>>>
>>>> No need for this with the above checks
>>>>
>>>>> +        psp_platform.feature_reg = pdata->feature_reg;
>>>>
>>>> These should then be:
>>>>
>>>>          vdata->psp_vdata->inten_reg = pdata->feature_reg;
>>>>          ...
>>>
>>> I see where you're going with this and the above suggestions, but
>>> the psp_vdata pointer is const in struct sp_dev_vdata and so is the
>>> sev pointer in struct psp_vdata. I find these consts to be important
>>> and doing it this way would require casting away the const. I don't
>>> think that's worth doing.
>>
>> Ok, then maybe it would be better to kmalloc a vdata structure that you fill in and then assign that to dev_vdata field for use. That could eliminate the removal of the "const" notations in one of the previous patches. I just don't think you should be changing the underlying module data that isn't expected to be changed.
>>
> 
> I can do that and undo the removal of consts from the
> struct (sev|psp)_vdata members, but the outcome would look something
> like this:
> 
> static void sp_platform_fill_vdata(struct sp_dev_vdata *vdata,
> 					struct psp_vdata *psp,
> 					struct sev_vdata *sev,
> 					const struct psp_platform_data *pdata)
> {
> 	struct sev_vdata sevtmp = {
> 		.cmdbuff_addr_hi_reg = pdata->sev_cmd_buf_hi_reg,
> 		.cmdbuff_addr_lo_reg = pdata->sev_cmd_buf_lo_reg,
> 		.cmdresp_reg = pdata->sev_cmd_resp_reg,
> 	};
> 	struct psp_vdata psptmp = {
> 		.feature_reg = pdata->feature_reg,
> 		.inten_reg = pdata->irq_en_reg,
> 		.intsts_reg = pdata->irq_st_reg,
> 		.sev = sev,
> 	};
> 
> 	memcpy(sev, &sevtmp, sizeof(*sev));
> 	memcpy(psp, &psptmp, sizeof(*psp));
> 	vdata->psp_vdata = psp;
> }
> 
> static struct sp_dev_vdata *sp_get_platform_version(struct sp_device *sp)
> {
> 	struct sp_platform *sp_platform = sp->dev_specific;
> 	struct psp_platform_data *pdata;
> 	struct device *dev = sp->dev;
> 	struct sp_dev_vdata *vdata;
> 	struct psp_vdata *psp;
> 	struct sev_vdata *sev;
> 
> 	pdata = dev_get_platdata(dev);
> 	if (!pdata) {
> 		dev_err(dev, "missing platform data\n");
> 		return NULL;
> 	}
> 
> 	sp_platform->is_platform_device = true;
> 
> 	vdata = devm_kzalloc(dev, sizeof(*vdata) + sizeof(*psp) + sizeof(*sev), GFP_KERNEL);
> 	if (!vdata)
> 		return NULL;
> 
> 	psp = (void *)vdata + sizeof(*vdata);
> 	sev = (void *)psp + sizeof(*psp);
> 	sp_platform_fill_vdata(vdata, psp, sev, pdata);
> 
> 	/* elided debug print */
> 	...
> 
> 	return vdata;
> }
> 
> with the const fields in the struct it's not possible to assign in any
> other way than on initialization, so I need to use the helper function,
> tmp structs and memcpy.

Yeah, not the prettiest, but I prefer this over altering the static data.

> 
> Could you ack that you like this approach before I post a v2?

Yes, please go with this approach.

Thanks,
Tom