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 thought about putting psp.c in arch/x86/coco, but that directory is meant for
the (confidential) guest side of CoCo, not the supporting host side code.
It was kept in arch/x86/kernel because configuring the irq for the PSP through
the ACPI interface requires poking at bits from the architectural vector
domain.
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.
Changes since v2:
* Added links to ASPT spec and ACPICA commit
* Added acked-by Tom to all commits
Changes since v1:
* move platform_device_add_data() call to commit that introduces psp device
* change psp dependency from CONFIG_AMD_MEM_ENCRYPT to CONFIG_KVM_AMD_SEV
* add blank lines, s/plat/platform/, remove variable initializers before first
use, remove masking/shifting where not needed
* dynamically allocate sev_vdata/psp_vdata structs instead of overwriting static
variables
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 | 1 +
arch/x86/kernel/psp.c | 219 ++++++++++++++++++++++++++++++
drivers/acpi/Makefile | 1 +
drivers/acpi/aspt.c | 104 ++++++++++++++
drivers/crypto/ccp/sp-dev.c | 65 +++++++++
drivers/crypto/ccp/sp-dev.h | 4 +
drivers/crypto/ccp/sp-pci.c | 48 -------
drivers/crypto/ccp/sp-platform.c | 76 ++++++++++-
include/acpi/actbl1.h | 46 +++++++
include/linux/platform_data/psp.h | 32 +++++
10 files changed, 547 insertions(+), 49 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.34.1
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.
Link: https://github.com/acpica/acpica/commit/15b939b034ab41a864b4e7647b8e2233780bb0c7
Link: https://www.amd.com/system/files/TechDocs/58028_1.00-PUB.pdf
Acked-by: Tom Lendacky <[email protected]>
Signed-off-by: Jeremi Piotrowski <[email protected]>
---
include/acpi/actbl1.h | 46 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index 4175dce3967c..0fa428d335b2 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 */
@@ -107,6 +108,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.34.1
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.
Link: https://github.com/acpica/acpica/commit/15b939b034ab41a864b4e7647b8e2233780bb0c7
Link: https://www.amd.com/system/files/TechDocs/58028_1.00-PUB.pdf
Acked-by: Tom Lendacky <[email protected]>
Signed-off-by: Jeremi Piotrowski <[email protected]>
---
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 feb36c0b9446..831d7a12b522 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.34.1
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.
Acked-by: Tom Lendacky <[email protected]>
Signed-off-by: Jeremi Piotrowski <[email protected]>
---
arch/x86/kernel/psp.c | 185 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 181 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c
index 64f3bfc5c9ff..fc059cf3b25c 100644
--- a/arch/x86/kernel/psp.c
+++ b/arch/x86/kernel/psp.c
@@ -1,8 +1,182 @@
// 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_CMDID_SHIFT 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_MASK GENMASK(7, 0)
+#define PSP_ACPI_MBOX_IRQID_SHIFT 10
+#define PSP_ACPI_IRQ_EN_BIT BIT(0)
+#define PSP_ACPI_IRQ_EN_MBOX_IRQID_SHIFT 10
+
+#define PSP_CMD_DELAY_US 2
+#define PSP_CMD_TIMEOUT_US 10000
+
+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;
+ u8 mbox_irq_id;
+ int acpi_cmd_resp_reg;
+};
+
+static int psp_sync_cmd(void __iomem *reg, u8 cmd, u16 data)
+{
+ u32 val;
+ int err;
+
+ val = data;
+ val |= cmd << PSP_ACPI_CMDID_SHIFT;
+ writel(val, reg);
+ err = readl_poll_timeout_atomic(reg, val, val & PSP_ACPI_RESPONSE_BIT, PSP_CMD_DELAY_US,
+ PSP_CMD_TIMEOUT_US);
+ if (err)
+ 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;
+ u16 val = 0;
+ int err;
+
+ if (data->mbox_irq_id > 63)
+ return -EINVAL;
+
+ val = irq_en ? PSP_ACPI_IRQ_EN_BIT : 0;
+ val |= data->mbox_irq_id << PSP_ACPI_IRQ_EN_MBOX_IRQID_SHIFT;
+ 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, unsigned int vector, unsigned int cpu)
+{
+ void __iomem *reg = data->base + data->acpi_cmd_resp_reg;
+ unsigned int dest_cpu = cpu_physical_id(cpu);
+ u16 part1, part2, part3;
+ int err;
+
+ if (data->mbox_irq_id > 63)
+ return -EINVAL;
+
+ part1 = dest_cpu;
+ part2 = dest_cpu >> 16;
+ part3 = vector & PSP_ACPI_VECTOR_MASK;
+ part3 |= data->mbox_irq_id << PSP_ACPI_MBOX_IRQID_SHIFT;
+
+ 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;
+ struct irq_cfg *cfg;
+ void __iomem *base;
+ int virq;
+ int err;
+
+ base = ioremap(reg->start, resource_size(reg));
+ 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 (!cfg) {
+ 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 +186,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 +198,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;
err = platform_device_add_data(&psp_device, &pdata, sizeof(pdata));
--
2.34.1
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.
Acked-by: Tom Lendacky <[email protected]>
Signed-off-by: Jeremi Piotrowski <[email protected]>
---
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/psp.c | 42 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 43 insertions(+)
create mode 100644 arch/x86/kernel/psp.c
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 96d51bbc2bd4..6fe52328bc28 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -139,6 +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_KVM_AMD_SEV) += psp.o
obj-$(CONFIG_AMD_MEM_ENCRYPT) += 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..64f3bfc5c9ff
--- /dev/null
+++ b/arch/x86/kernel/psp.c
@@ -0,0 +1,42 @@
+// 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_add_data(&psp_device, &pdata, sizeof(pdata));
+ if (err)
+ return err;
+
+ err = platform_device_register(&psp_device);
+ if (err)
+ return err;
+ return 0;
+}
+device_initcall(psp_init_platform_device);
--
2.34.1
On Mon, Mar 20, 2023 at 07:19:51PM +0000, 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.
>
> Acked-by: Tom Lendacky <[email protected]>
> Signed-off-by: Jeremi Piotrowski <[email protected]>
> ---
> arch/x86/kernel/Makefile | 1 +
> arch/x86/kernel/psp.c | 42 ++++++++++++++++++++++++++++++++++++++++
If this is a platform device (driver), why isn't it part of
the drivers/platform/x86/ lineup?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
When matching the "psp" platform_device, the register offsets are
determined at runtime from the ASP ACPI table. The structure containing
the offset is passed through the platform data field provided before
registering the platform device.
To support this scenario, dynamically allocate vdata structs and fill
those in with offsets provided by the platform. Due to the fields of the
structs being const, it was necessary to use temporary structs and
memcpy, as any assignment of the whole struct fails with an 'read-only
location' compiler error.
Acked-by: Tom Lendacky <[email protected]>
Signed-off-by: Jeremi Piotrowski <[email protected]>
---
drivers/crypto/ccp/sp-platform.c | 57 ++++++++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
index 5dcc834deb72..2e57ec15046b 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"
@@ -86,6 +87,60 @@ static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev)
return NULL;
}
+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 = {
+ .cmdresp_reg = pdata->sev_cmd_resp_reg,
+ .cmdbuff_addr_lo_reg = pdata->sev_cmd_buf_lo_reg,
+ .cmdbuff_addr_hi_reg = pdata->sev_cmd_buf_hi_reg,
+ };
+ struct psp_vdata psptmp = {
+ .sev = sev,
+ .feature_reg = pdata->feature_reg,
+ .inten_reg = pdata->irq_en_reg,
+ .intsts_reg = pdata->irq_st_reg,
+ };
+
+ 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 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;
+ }
+
+ 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);
+
+ dev_dbg(dev, "PSP feature register:\t%x\n", psp->feature_reg);
+ dev_dbg(dev, "PSP IRQ enable register:\t%x\n", psp->inten_reg);
+ dev_dbg(dev, "PSP IRQ status register:\t%x\n", psp->intsts_reg);
+ dev_dbg(dev, "SEV cmdresp register:\t%x\n", sev->cmdresp_reg);
+ dev_dbg(dev, "SEV cmdbuf lo register:\t%x\n", sev->cmdbuff_addr_lo_reg);
+ dev_dbg(dev, "SEV cmdbuf hi register:\t%x\n", sev->cmdbuff_addr_hi_reg);
+ dev_dbg(dev, "SEV cmdresp IRQ:\t%x\n", pdata->mbox_irq_id);
+ dev_dbg(dev, "ACPI cmdresp register:\t%x\n", pdata->acpi_cmd_resp_reg);
+
+ return vdata;
+}
+
static int sp_get_irqs(struct sp_device *sp)
{
struct sp_platform *sp_platform = sp->dev_specific;
@@ -137,6 +192,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)
+ sp->dev_vdata = sp_get_platform_version(sp);
if (!sp->dev_vdata) {
ret = -ENODEV;
dev_err(dev, "missing driver data\n");
--
2.34.1
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.
Acked-by: Tom Lendacky <[email protected]>
Signed-off-by: Jeremi Piotrowski <[email protected]>
---
drivers/crypto/ccp/sp-dev.c | 7 +++++++
drivers/crypto/ccp/sp-platform.c | 7 +++++++
2 files changed, 14 insertions(+)
diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c
index 7eb3e4668286..4c9f442b8a11 100644
--- a/drivers/crypto/ccp/sp-dev.c
+++ b/drivers/crypto/ccp/sp-dev.c
@@ -259,6 +259,12 @@ static int __init sp_mod_init(void)
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,6 +292,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..5dcc834deb72 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_platform_match[] = {
+ { "psp" },
+ { },
+};
+MODULE_DEVICE_TABLE(platform, sp_platform_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_platform_match,
.driver = {
.name = "ccp",
#ifdef CONFIG_ACPI
--
2.34.1
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. Skip the dma_attr check by keeping track of the fact that we
are a pure platform device.
Acked-by: Tom Lendacky <[email protected]>
Signed-off-by: Jeremi Piotrowski <[email protected]>
---
drivers/crypto/ccp/sp-platform.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
index 2e57ec15046b..be8306c47196 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_device;
};
static const struct sp_dev_vdata dev_vdata[] = {
@@ -109,6 +110,7 @@ static void sp_platform_fill_vdata(struct sp_dev_vdata *vdata, struct psp_vdata
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;
@@ -129,6 +131,8 @@ static struct sp_dev_vdata *sp_get_platform_version(struct sp_device *sp)
sev = (void *)psp + sizeof(*psp);
sp_platform_fill_vdata(vdata, psp, sev, pdata);
+ sp_platform->is_platform_device = true;
+
dev_dbg(dev, "PSP feature register:\t%x\n", psp->feature_reg);
dev_dbg(dev, "PSP IRQ enable register:\t%x\n", psp->inten_reg);
dev_dbg(dev, "PSP IRQ status register:\t%x\n", psp->intsts_reg);
@@ -207,7 +211,7 @@ static int sp_platform_probe(struct platform_device *pdev)
}
attr = device_get_dma_attr(dev);
- if (attr == DEV_DMA_NOT_SUPPORTED) {
+ if (attr == DEV_DMA_NOT_SUPPORTED && !sp_platform->is_platform_device) {
dev_err(dev, "DMA is not supported");
goto e_err;
}
--
2.34.1
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.
Acked-by: Tom Lendacky <[email protected]>
Signed-off-by: Jeremi Piotrowski <[email protected]>
---
drivers/crypto/ccp/sp-dev.c | 58 ++++++++++++++++++++++++++++++++
drivers/crypto/ccp/sp-dev.h | 4 +++
drivers/crypto/ccp/sp-pci.c | 48 --------------------------
drivers/crypto/ccp/sp-platform.c | 6 ++++
4 files changed, 68 insertions(+), 48 deletions(-)
diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c
index 4c9f442b8a11..0ac1d0eba8bb 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,60 @@ 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 20377e67f65d..c05f1fa82ff4 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 084d052fddcc..af8f0f65c4b5 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 be8306c47196..e22d9fee0956 100644
--- a/drivers/crypto/ccp/sp-platform.c
+++ b/drivers/crypto/ccp/sp-platform.c
@@ -234,6 +234,12 @@ static int sp_platform_probe(struct platform_device *pdev)
dev_set_drvdata(dev, sp);
+ if (sp_platform->is_platform_device) {
+ 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.34.1
On 20/03/2023 20:25, Borislav Petkov wrote:
> On Mon, Mar 20, 2023 at 07:19:51PM +0000, 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.
>>
>> Acked-by: Tom Lendacky <[email protected]>
>> Signed-off-by: Jeremi Piotrowski <[email protected]>
>> ---
>> arch/x86/kernel/Makefile | 1 +
>> arch/x86/kernel/psp.c | 42 ++++++++++++++++++++++++++++++++++++++++
>
> If this is a platform device (driver), why isn't it part of
> the drivers/platform/x86/ lineup?
>
Because of patch 4. My thinking was that the irq setup requires poking
at intimate architectural details (init_irq_alloc_info etc.) so it seems
like it fits in arch/x86.
I also drew inspiration from the sev-guest device in the arch/x86/kernel/sev.c,
which is used in a similar context (the PSP device I am registering here is
for SNP-host support).
Would you prefer it in drivers/platform/x86?
Jeremi
On Mon, Mar 20, 2023 at 08:37:56PM +0100, Jeremi Piotrowski wrote:
> Because of patch 4. My thinking was that the irq setup requires poking
> at intimate architectural details (init_irq_alloc_info etc.) so it seems
> like it fits in arch/x86.
arch/x86/platform/uv/uv_irq.c:193: init_irq_alloc_info(&info, cpumask_of(cpu));
drivers/iommu/amd/init.c:2391: init_irq_alloc_info(&info, NULL);
Also, what patch 4's commit message says, sounds hacky to me. A simple
driver should not need the x86_vector_domain. Especially if it is some
ACPI wrapper around the PSP hw.
But I'd leave that to tglx.
> I also drew inspiration from the sev-guest device in the arch/x86/kernel/sev.c,
Yeah, we've designed another mess there considering we already have
drivers/virt/coco/sev-guest/sev-guest.c
That sev guest thing has no place in sev.c and it should go away from
there.
> which is used in a similar context (the PSP device I am registering here is
> for SNP-host support).
>
> Would you prefer it in drivers/platform/x86?
drivers/hv/?
Seeing how hyperv is the only thing that's going to use it, AFAICT.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 20/03/2023 21:03, Borislav Petkov wrote:
> On Mon, Mar 20, 2023 at 08:37:56PM +0100, Jeremi Piotrowski wrote:
>> Because of patch 4. My thinking was that the irq setup requires poking
>> at intimate architectural details (init_irq_alloc_info etc.) so it seems
>> like it fits in arch/x86.
>
> arch/x86/platform/uv/uv_irq.c:193: init_irq_alloc_info(&info, cpumask_of(cpu));
> drivers/iommu/amd/init.c:2391: init_irq_alloc_info(&info, NULL);
>
> Also, what patch 4's commit message says, sounds hacky to me. A simple
> driver should not need the x86_vector_domain. Especially if it is some
> ACPI wrapper around the PSP hw.
I agree with you here. The irq config of this thing requires specifying
passing a CPU vector, this follows the hardware spec which I linked in the
first 2 commits, pages 13-15 here:
https://www.amd.com/system/files/TechDocs/58028_1.00-PUB.pdf
The only way I found to get this to work was going through x86_vector_domain
or statically defining a system vector (the latter felt worse).
>
> But I'd leave that to tglx.
>>> I also drew inspiration from the sev-guest device in the arch/x86/kernel/sev.c,
>
> Yeah, we've designed another mess there considering we already have
>
> drivers/virt/coco/sev-guest/sev-guest.c
>
> That sev guest thing has no place in sev.c and it should go away from
> there.
>
>> which is used in a similar context (the PSP device I am registering here is
>> for SNP-host support).
>>
>> Would you prefer it in drivers/platform/x86?
>
> drivers/hv/?
>
> Seeing how hyperv is the only thing that's going to use it, AFAICT.
>
That could work, let me try that.
On Mon, Mar 20, 2023 at 09:18:19PM +0100, Jeremi Piotrowski wrote:
> I agree with you here. The irq config of this thing requires specifying
> passing a CPU vector, this follows the hardware spec which I linked in the
> first 2 commits, pages 13-15 here:
You mean the interrupt vector in table 19?
> https://www.amd.com/system/files/TechDocs/58028_1.00-PUB.pdf
>
> The only way I found to get this to work was going through x86_vector_domain
> or statically defining a system vector (the latter felt worse).
Hmm. Why is that thing special and can't use devm_request_irq() like the
rest of the drivers out there?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Mar 20 2023 at 19:19, 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.
Sorry, but this is a horrible hack which violates _all_ design rules
for interrupts in one go.
1) What's so special about this PSP device that it requires a vector
directly from the vector domain and evades interrupt remapping?
2) Why is this interrupt enabled _before_ it is actually requested?
3) Why is this interrupt required to be bound to CPU0 and still exposes
a disfunctional and broken affinity setter interface in /proc?
There is absolutely zero reason and justification to fiddle in the guts
of the x86 vector configuration data just because it's possible.
This is clearly a custom MSI implementation and the obvious solution is
a per device MSI interrupt domain.
Thanks,
tglx
On 20/03/2023 22:03, Borislav Petkov wrote:
> On Mon, Mar 20, 2023 at 09:18:19PM +0100, Jeremi Piotrowski wrote:
>> I agree with you here. The irq config of this thing requires specifying
>> passing a CPU vector, this follows the hardware spec which I linked in the
>> first 2 commits, pages 13-15 here:
>
> You mean the interrupt vector in table 19?
>
Yes - this thing wants to receive an interrupt vector and APIC id which it will
then use to target its interrupt at.
>> https://www.amd.com/system/files/TechDocs/58028_1.00-PUB.pdf
>>
>> The only way I found to get this to work was going through x86_vector_domain
>> or statically defining a system vector (the latter felt worse).
>
> Hmm. Why is that thing special and can't use devm_request_irq() like the
> rest of the drivers out there?
>
Because the device is not exposed through AML (with ACPI managed irq routing)
and needs to be discovered manually and the interrupt programmed by hand.
I don't know the reasoning behind it being specified this way.
But essentially I am doing all this nasty stuff so that I get a simple irq number.
This is then passed to the actual driver that binds to the platform_device
(drivers/crypto/ccp/sp-platform.c) which uses it with devm_request_irq.
On 21/03/2023 11:31, Thomas Gleixner wrote:
> On Mon, Mar 20 2023 at 19:19, 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.
>
> Sorry, but this is a horrible hack which violates _all_ design rules
> for interrupts in one go.
>
Ouch. But I agree and it's not like i was trying to sneak it past you -
I just didn't know how to map the hardware behavior to kernel constructs
any better so was hoping to get some guidance.
> 1) What's so special about this PSP device that it requires a vector
> directly from the vector domain and evades interrupt remapping?
>
The PSP interrupt configuration requires passing an APIC id and interrupt
vector that it will assert. The closest thing I found that provides me with
those was the x86_vector_domain. Here's the link to the ACPI interface, the
relevant info is on pages 13-15 (it's not very detailed, but that's all I
had when implementing this):
https://www.amd.com/system/files/TechDocs/58028_1.00-PUB.pdf
The platform that I have access to for testing does not have interrupt remapping
so it's not something I thought about. I don't know how this thing would behave
with interrupt remapping.
> 2) Why is this interrupt enabled _before_ it is actually requested?
>
In this implementation: so that I can configure everything before setting the
irq number on the platform device. I had a version with an irq domain that I
wasn't happy with, I'll talk about that below.
> 3) Why is this interrupt required to be bound to CPU0 and still exposes
> a disfunctional and broken affinity setter interface in /proc?>
It's not required to be bound to CPU0. But yes, with this implementation
smp_affinity does not work (effective_affinity is correct though).
> There is absolutely zero reason and justification to fiddle in the guts
> of the x86 vector configuration data just because it's possible.
>
> This is clearly a custom MSI implementation and the obvious solution is
> a per device MSI interrupt domain.
>
This is an interesting suggestion, and it wasn't at all obvious to me that
this maps to an MSI interrupt domain. I'd love to get that to work, let me
ask some follow-up questions:
1) do I need both a standard irq domain and an msi domain?
2) what domain do I take as a parent?
3) i will still need to extract apic id + vector from somewhere. irq_compose_msi_msg
or irq_write_msi_msg seem like candidates, but then i'd still be relying on knowledge
about the hierarchy and need to poke at irqd_cfg or read msi_msg.arch_data. Am I missing
something, do you see a better way?
4) will this actually work considering that the PSP will not actually write to an
MSI address, and that I can't use all the data contained in an msi_msg?
My first attempt that "worked" used a plain irq domain and can be found here:
https://github.com/jepio/linux/commit/43c9ed6de82a3ae6c3f2d7894b3da049cb1ea4e4
It had this structure:
static struct irq_chip psp_irq_chip = {
.name = "PSP",
.irq_enable = psp_irq_enable,
.irq_disable = psp_irq_disable,
.irq_set_affinity = psp_irq_set_affinity, // calls psp_configure_irq(data, cpu num)
};
static int psp_irq_domain_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hwirq)
{
struct psp_irq_domain_data *data = d->host_data;
// this used a global system vector instead of x86_vector_domain
// and i needed to program to some cpu otherwise psp_irq_enable fails
psp_configure_irq(data, 0);
irq_set_chip_and_handler(irq, &psp_irq_chip, handle_simple_irq);
irq_set_chip_data(irq, d->host_data);
return 0;
}
static const struct irq_domain_ops psp_irq_domain_ops = {
.map = psp_irq_domain_map,
};
static int psp_init_irq()
{
// eliding lots of things
psp_domain = irq_domain_create_linear(NULL, 1, &psp_irq_domain_ops, &psp_irq_data);
return irq_create_mapping(psp_domain, 0);
}
This version had many flaws: it wasn't hierarchical, it relied on a static
system vector (it was only later that i figured out how to dynamically allocate the
vector), and setting irq affinity didn't actually work (due to lack of mask/unmask
I think).
Jeremi
> Thanks,
>
> tglx
>
On Tue, Mar 21 2023 at 20:16, Jeremi Piotrowski wrote:
> On 21/03/2023 11:31, Thomas Gleixner wrote:
>
>> 1) What's so special about this PSP device that it requires a vector
>> directly from the vector domain and evades interrupt remapping?
>
> The PSP interrupt configuration requires passing an APIC id and interrupt
> vector that it will assert. The closest thing I found that provides me with
> those was the x86_vector_domain. Here's the link to the ACPI interface, the
> relevant info is on pages 13-15 (it's not very detailed, but that's all I
> had when implementing this):
> https://www.amd.com/system/files/TechDocs/58028_1.00-PUB.pdf
That seriously expects an (extended) APIC-ID so that firmware can fiddle
with X2APIC ICR directly.
Why can't firmware people just use something which is properly
manageable by the OS, e.g. a MSI message or something like the ACPI
interrupt? Because that would just be too useful and not require
horrible hacks.
So my initial suggestion to piggy pack that on device MSI is moot. Let
me think about it.
Thanks,
tglx
On 22/03/2023 16:46, Borislav Petkov wrote:
> On Mon, Mar 20, 2023 at 07:19:48PM +0000, Jeremi Piotrowski wrote:
>> This series is a prerequisite for nested SNP-host support on Hyper-V
>
> I'm curious: what in the *world* is a sensible use case for doing this
> thing at all?
>
This is actually not as crazy as it sounds.
What this does is it allows a normal (non-SNP) VM to host confidential (SNP)
VMs. I say "normal" but not every VM is going to be able to do this, it needs
to be running on AMD hardware and configured to have access to
VirtualizationExtensions, a "HardwareIsolation" capability, and given a number
of "hardware isolated guests" that it is allowed to spawn. In practice this
will result in the VM seeing a PSP device, SEV-SNP related CPUID leafs, and
have access to additional memory management instructions (rmpadjust/psmash).
This allows the rest of the of KVM-SNP support to work.
So instead of taking a bare-metal AMD server with 128 CPUs to run confidential
workloads you'll be able to provision an Azure VM with say 8 CPUs and run up to
8 SNP guests nested inside it.
It's also useful for development, I participate in the kata-containers project
where we're doing confidential-containers related work, and having access to
test VMs to run SNP guests is going to make things much easier.
If you're interested, I posted the other half of the patches required some time
back: https://lore.kernel.org/lkml/[email protected]/#t
Jeremi
On Wed, Mar 22, 2023 at 06:33:37PM +0100, Jeremi Piotrowski wrote:
> What this does is it allows a normal (non-SNP) VM to host confidential (SNP)
> VMs. I say "normal" but not every VM is going to be able to do this, it needs
If you say "non-SNP" VM then this sounds like purely for development.
Because I cannot see how you're going to give the confidentiality
guarantee to the SNP guests if the lower level is unencrypted, non-SNP
and so on...
> to be running on AMD hardware and configured to have access to
> VirtualizationExtensions, a "HardwareIsolation" capability, and given a number
> of "hardware isolated guests" that it is allowed to spawn. In practice this
> will result in the VM seeing a PSP device, SEV-SNP related CPUID
> leafs, and have access to additional memory management instructions
> (rmpadjust/psmash). This allows the rest of the of KVM-SNP support to
> work.
So why don't you emulate the PSP in KVM instead of doing some BIOS hack?
And multiplex the access to it between all the parties needing it?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 3/22/2023 7:15 PM, Borislav Petkov wrote:
> On Wed, Mar 22, 2023 at 06:33:37PM +0100, Jeremi Piotrowski wrote:
>> What this does is it allows a normal (non-SNP) VM to host confidential (SNP)
>> VMs. I say "normal" but not every VM is going to be able to do this, it needs
>
> If you say "non-SNP" VM then this sounds like purely for development.
> Because I cannot see how you're going to give the confidentiality
> guarantee to the SNP guests if the lower level is unencrypted, non-SNP
> and so on...
Not at all. Just to be clear: this lights up all the same bits of SNP
as it does on bare-metal, none of it is emulated away. On bare-metal the
hypervisor underneath the SNP guest is unencrypted as well. Here the stack
is: L0 (Hyper-V), L1 (KVM) and L2 (SNP guest).
Starting an SNP guest is the same and involves sending commands to the PSP:
* SNP_GCTX_CREATE
* SNP_LAUNCH_START
* SNP_LAUNCH_UPDATE
* SNP_LAUNCH_FINISH
Pages need to be assigned to a specific L2 SNP guest in the system-wide
"reverse map table", at which point neither L0 nor L1 hypervisor can touch
them. Every L2 SNP guests memory is encrypted with a different key, and the
SNP guest can fetch a hardware signed attestation report from the PSP that
includes a hash of all the pages that were loaded (and encrypted) into the
VM address space at the time the VM was launched. The communication channel
between L2 guest and PSP is secured using keys that the PSP injects into the
SNP guest's address space at launch time.
Honestly, I find it pretty cool that you can stuff a whole extra hypervisor
underneath the SNP guest, and the hardware will still ensure and attest to
the fact that neither hypervisor is able to compromise the integrity and
confidentiality of the VM enclave. And you can verify this claim independently.
>
>> to be running on AMD hardware and configured to have access to
>> VirtualizationExtensions, a "HardwareIsolation" capability, and given a number
>> of "hardware isolated guests" that it is allowed to spawn. In practice this
>> will result in the VM seeing a PSP device, SEV-SNP related CPUID
>> leafs, and have access to additional memory management instructions
>> (rmpadjust/psmash). This allows the rest of the of KVM-SNP support to
>> work.
>
> So why don't you emulate the PSP in KVM instead of doing some BIOS hack?
> And multiplex the access to it between all the parties needing it?
>
Not sure I follow you here. The quoted paragraph talks about what the L1
VM (KVM) sees. The L1 VM needs to issue PSP commands to bring up an L2 SNP
guest, and later the L1 VM relays SNP guest commands to the PSP. The
PSP commands are multiplexed to the physical PSP by the L0 hypervisor
(Hyper-V).
So Hyper-V exposes a PSP to the L1 VM because it is needed and it is
compatible with the existing Linux driver that handles the PSP. The way
it is exposed (ACPI table) follows how it was specified by AMD.
On Thu, Mar 23, 2023 at 05:11:26PM +0100, Jeremi Piotrowski wrote:
> That same interface is exposed by physical hardware+firmware to the underlying
> Hyper-V.
Let me see if I understand it correctly: Hyper-V *baremetal* is using
the same ASPT spec to to talk to the *physical* PSP device?
Is that ASPT interface to talk to the PSP used by the L0 hypervisor?
Or does the L0 HV have a normal driver, similar to the Linux one,
without the functionality this ASPT spec provides?
> So it wasn't a matter of Microsoft architects coming up with a
> guest-host interface but rather exposing the virtual hardware in the same
> way as on a physical server.
So if you want to expose the same interface to the L1 guest, why isn't
Hyper-V emulating an ACPI device just like any other functionality? Why
does it need to reach into the interrupt handling internals?
I'd expect that the L0 HV would emulate a PSP device, the L1 would
simply load the Linux PSP device driver and everything should just work.
What's the point of that alternate access at all?
But I might still be missing something...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Mar 22, 2023 at 11:07:25AM +0100, Thomas Gleixner wrote:
> On Tue, Mar 21 2023 at 20:16, Jeremi Piotrowski wrote:
> > On 21/03/2023 11:31, Thomas Gleixner wrote:
> >
> >> 1) What's so special about this PSP device that it requires a vector
> >> directly from the vector domain and evades interrupt remapping?
> >
> > The PSP interrupt configuration requires passing an APIC id and interrupt
> > vector that it will assert. The closest thing I found that provides me with
> > those was the x86_vector_domain. Here's the link to the ACPI interface, the
> > relevant info is on pages 13-15 (it's not very detailed, but that's all I
> > had when implementing this):
> > https://www.amd.com/system/files/TechDocs/58028_1.00-PUB.pdf
>
> That seriously expects an (extended) APIC-ID so that firmware can fiddle
> with X2APIC ICR directly.
>
> Why can't firmware people just use something which is properly
> manageable by the OS, e.g. a MSI message or something like the ACPI
> interrupt? Because that would just be too useful and not require
> horrible hacks.
>
> So my initial suggestion to piggy pack that on device MSI is moot. Let
> me think about it.
>
> Thanks,
>
> tglx
>
>
So this is what it looks when done properly with an irq_domain. It definitely
feels less like a hack to me now (thank you): setting affinity works and the
irq is enabled only when requested. The lack of support for interrupt remapping
and the need to configure with APIC-ID/vector comes from the fact that for
firmware designers this PSP operates at the same level as the AMD IOMMU (and
the IOMMU is responsible for interrupt remapping).
Let me know what you think.
Jeremi