Support ACPI PSP on Hyper-V

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 <[email protected]>

---
 include/acpi/actbl1.h | 46 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

include/acpi/actbl1.h +46 -1

index 15c78678c5d3..00d40373df37 100644
@@ -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

Quoted 6 lines · 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.

Quoted 1 line

Signed-off-by Jeremi Piotrowski <[email protected]>

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

Thanks!

Quoted 72 lines

---
 include/acpi/actbl1.h | 46 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

include/acpi/actbl1.h +46 -1

index 15c78678c5d3..00d40373df37 100644
@@ -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

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 <[email protected]>

---
  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

arch/x86/kernel/Makefile +1 -1

index f901658d9f7c..e2e19f2d08a7 100644
@@ -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
 

arch/x86/kernel/psp.c new file +39 -0

new file mode 100644
index 000000000000..d404df47cc04
@@ -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);

Quoted 13 lines · 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.

Quoted 8 lines

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 <[email protected]>

---
 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

Quoted 59 lines

 2 files changed, 40 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/kernel/psp.c

arch/x86/kernel/Makefile +1 -1

index f901658d9f7c..e2e19f2d08a7 100644
@@ -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

arch/x86/kernel/psp.c new file +39 -0

new file mode 100644
index 000000000000..d404df47cc04
@@ -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);

Quoted 26 lines · Tom Lendacky

Quoted 13 lines · 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.

Quoted 8 lines

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 <[email protected]>

---
  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.

Quoted 2 lines

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

Quoted 59 lines

Quoted 59 lines

  2 files changed, 40 insertions(+), 1 deletion(-)

create mode 100644 arch/x86/kernel/psp.c

arch/x86/kernel/Makefile +1 -1

index f901658d9f7c..e2e19f2d08a7 100644
@@ -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

arch/x86/kernel/psp.c new file +39 -0

new file mode 100644
index 000000000000..d404df47cc04
@@ -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);

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 <[email protected]>

---
  arch/x86/kernel/psp.c | 179 +++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 175 insertions(+), 4 deletions(-)

arch/x86/kernel/psp.c +37 -1

index d404df47cc04..24181d132bae 100644
@@ -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,
+};

arch/x86/kernel/psp.c +5 -0

+	return 0;
+}
+
+static int psp_configure_irq(struct psp_irq_data *data, int vector, unsigned int cpu)
+{

arch/x86/kernel/psp.c +13 -0

+
+	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) {

arch/x86/kernel/psp.c +8 -3

+	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;
 

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 <[email protected]>

---
  drivers/crypto/ccp/sp-dev.c      | 8 ++++++--
  drivers/crypto/ccp/sp-platform.c | 7 +++++++
  2 files changed, 13 insertions(+), 2 deletions(-)

drivers/crypto/ccp/sp-dev.c +0 -0

index 7eb3e4668286..52b8d957d0f6 100644

drivers/crypto/ccp/sp-platform.c +6 -0

+	{ "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

Quoted 12 lines · 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 <[email protected]>

---
  drivers/crypto/ccp/sp-dev.c      | 8 ++++++--
  drivers/crypto/ccp/sp-platform.c | 7 +++++++

  2 files changed, 13 insertions(+), 2 deletions(-)

drivers/crypto/ccp/sp-dev.c static int __init sp_mod_init(void) · line 261

Quoted 9 lines

drivers/crypto/ccp/sp-dev.c +0 -1

index 7eb3e4668286..52b8d957d0f6 100644
@@ -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.>

drivers/crypto/ccp/sp-dev.c static int __init sp_mod_init(void) · line 265

Quoted 5 lines

drivers/crypto/ccp/sp-dev.c +5 -0

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

Add a blank line here.

drivers/crypto/ccp/sp-dev.c static void __exit sp_mod_exit(void) · line 289

Quoted 8 lines

drivers/crypto/ccp/sp-dev.c +0 -1

   #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.

Quoted 4 lines

drivers/crypto/ccp/sp-dev.c +1 -0

+    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

drivers/crypto/ccp/sp-platform.c static const struct of_device_id sp_of_match[] = { · line 58

Quoted 6 lines

drivers/crypto/ccp/sp-platform.c +0 -0

@@ -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

Quoted 15 lines

drivers/crypto/ccp/sp-platform.c +6 -0

+    { "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

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 <[email protected]>

---
  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(-)

arch/x86/kernel/psp.c +3 -0

index 24181d132bae..68511a14df63 100644
@@ -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;
 

drivers/crypto/ccp/sp-dev.h +6 -6

index 20377e67f65d..aaa651364425 100644
@@ -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 */

drivers/crypto/ccp/sp-platform.c +22 -1

index ea8926e87981..281dbf6b150c 100644
@@ -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);

drivers/crypto/ccp/sp-platform.c +1 -0

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

Quoted 106 lines · 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 <[email protected]>

---
 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(-)

arch/x86/kernel/psp.c +3 -0

index 24181d132bae..68511a14df63 100644
@@ -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;

drivers/crypto/ccp/sp-dev.h +6 -6

index 20377e67f65d..aaa651364425 100644
@@ -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 */

drivers/crypto/ccp/sp-platform.c +22 -1

index ea8926e87981..281dbf6b150c 100644
@@ -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);

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 111

Quoted 6 lines

drivers/crypto/ccp/sp-platform.c +3 -0

@@ -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/

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 113

Quoted 2 lines

drivers/crypto/ccp/sp-platform.c +2 -0

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

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

	if (!vdata)
		return NULL;

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 115

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

+	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) {

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 122

Quoted 7 lines

drivers/crypto/ccp/sp-platform.c +7 -0

+		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

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 123

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

+		psp_platform.feature_reg = pdata->feature_reg;

These should then be:

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

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 126

Quoted 4 lines

drivers/crypto/ccp/sp-platform.c +3 -0

+		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 = ...

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 129

Quoted 3 lines

drivers/crypto/ccp/sp-platform.c +3 -0

+		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/

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 130

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

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

s/GLBL irq en/PSP IRQ enable register/

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 131

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

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

s/GLBL irq st/PSP IRQ status register/

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 132

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

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

s/SEV cmdresp/SEV cmdresp register/

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 133

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

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

s/SEV cmdbuf lo/SEV cmdbuf lo register/

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 134

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

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

s/SEV cmdbuf hi/SEV cmdbuf hi register/

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 135

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

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

s/SEV mbox/SEV cmdresp IRQ/

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 136

Quoted 2 lines

drivers/crypto/ccp/sp-platform.c +1 -0

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

Duplicate entry

Quoted 10 lines

drivers/crypto/ccp/sp-platform.c +5 -0

+#endif
+	}
+	return drvdata;
+}
+
 static int sp_get_irqs(struct sp_device *sp)
 {
 	struct sp_platform *sp_platform = sp->dev_specific;

drivers/crypto/ccp/sp-platform.c static int sp_platform_probe(struct platform_device *pdev) · line 193

Quoted 4 lines

drivers/crypto/ccp/sp-platform.c +1 -0

@@ -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

Quoted 4 lines

drivers/crypto/ccp/sp-platform.c +1 -0

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

Quoted 107 lines · Tom Lendacky

Quoted 106 lines · 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 <[email protected]>

---
  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(-)

arch/x86/kernel/psp.c +3 -0

index 24181d132bae..68511a14df63 100644
@@ -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;

drivers/crypto/ccp/sp-dev.h +6 -6

index 20377e67f65d..aaa651364425 100644
@@ -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 */

drivers/crypto/ccp/sp-platform.c +22 -1

index ea8926e87981..281dbf6b150c 100644
@@ -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);

Quoted 9 lines

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 111

Quoted 6 lines

drivers/crypto/ccp/sp-platform.c +3 -0

@@ -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

Quoted 8 lines

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 113

Quoted 2 lines

drivers/crypto/ccp/sp-platform.c +2 -0

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

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

	if (!vdata)
		return NULL;

ok

Quoted 21 lines

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 115

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

+	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) {

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 122

Quoted 7 lines

drivers/crypto/ccp/sp-platform.c +7 -0

+		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

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 123

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

+		psp_platform.feature_reg = pdata->feature_reg;

These should then be:

Quoted 2 lines

		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.

Quoted 32 lines

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 126

Quoted 5 lines

drivers/crypto/ccp/sp-platform.c +3 -0

+		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 = ...

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 129

Quoted 3 lines

drivers/crypto/ccp/sp-platform.c +3 -0

+		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/

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 130

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

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

s/GLBL irq en/PSP IRQ enable register/

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 131

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

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

s/GLBL irq st/PSP IRQ status register/

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 132

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

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

s/SEV cmdresp/SEV cmdresp register/

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 133

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

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

s/SEV cmdbuf lo/SEV cmdbuf lo register/

Quoted 8 lines

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 134

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

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

s/SEV cmdbuf hi/SEV cmdbuf hi register/

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 135

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

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

s/SEV mbox/SEV cmdresp IRQ/

ok to all the above rewordings

Quoted 1 line

Quoted 3 lines

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 136

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

+		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

Quoted 18 lines

Quoted 10 lines

drivers/crypto/ccp/sp-platform.c +5 -0

+#endif
+	}
+	return drvdata;
+}
+
  static int sp_get_irqs(struct sp_device *sp)
  {
  	struct sp_platform *sp_platform = sp->dev_specific;

drivers/crypto/ccp/sp-platform.c static int sp_platform_probe(struct platform_device *pdev) · line 193

Quoted 4 lines

drivers/crypto/ccp/sp-platform.c +1 -0

@@ -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

Quoted 7 lines

Thanks,
Tom

Quoted 4 lines

drivers/crypto/ccp/sp-platform.c +1 -0

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

Quoted 155 lines · Jeremi Piotrowski

Quoted 107 lines · Tom Lendacky

Quoted 106 lines · 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 <[email protected]>

---
  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(-)

arch/x86/kernel/psp.c +3 -0

index 24181d132bae..68511a14df63 100644
@@ -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;

drivers/crypto/ccp/sp-dev.h +6 -6

index 20377e67f65d..aaa651364425 100644
@@ -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 */

drivers/crypto/ccp/sp-platform.c +22 -1

index ea8926e87981..281dbf6b150c 100644
@@ -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);

Quoted 9 lines

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 111

Quoted 6 lines

drivers/crypto/ccp/sp-platform.c +3 -0

@@ -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

Quoted 8 lines

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 113

Quoted 2 lines

drivers/crypto/ccp/sp-platform.c +2 -0

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

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

    if (!vdata)
        return NULL;

ok

Quoted 23 lines

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 115

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

+    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) {

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 122

Quoted 7 lines

drivers/crypto/ccp/sp-platform.c +7 -0

+        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

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 123

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

+        psp_platform.feature_reg = pdata->feature_reg;

These should then be:

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

Quoted 5 lines

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.

Quoted 156 lines · Tom Lendacky

Quoted 155 lines · Jeremi Piotrowski

Quoted 107 lines · Tom Lendacky

Quoted 106 lines · 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 <[email protected]>

---
   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(-)

arch/x86/kernel/psp.c +3 -0

index 24181d132bae..68511a14df63 100644
@@ -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;

drivers/crypto/ccp/sp-dev.h +6 -6

index 20377e67f65d..aaa651364425 100644
@@ -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 */

drivers/crypto/ccp/sp-platform.c +22 -1

index ea8926e87981..281dbf6b150c 100644
@@ -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);

Quoted 9 lines

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 111

Quoted 6 lines

drivers/crypto/ccp/sp-platform.c +3 -0

@@ -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

Quoted 8 lines

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 113

Quoted 2 lines

drivers/crypto/ccp/sp-platform.c +2 -0

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

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

    if (!vdata)
        return NULL;

ok

Quoted 23 lines

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 115

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

+    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) {

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 122

Quoted 7 lines

drivers/crypto/ccp/sp-platform.c +7 -0

+        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

drivers/crypto/ccp/sp-platform.c static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) · line 123

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +1 -0

+        psp_platform.feature_reg = pdata->feature_reg;

These should then be:

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

Quoted 8 lines

Quoted 5 lines

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?

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 <[email protected]>

---
  drivers/crypto/ccp/sp-platform.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)

drivers/crypto/ccp/sp-platform.c +0 -0

index 281dbf6b150c..b74f16e0e963 100644

drivers/crypto/ccp/sp-platform.c +0 -0

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

Quoted 10 lines · 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 <[email protected]>

---
  drivers/crypto/ccp/sp-platform.c | 7 +++++--

  1 file changed, 5 insertions(+), 2 deletions(-)

drivers/crypto/ccp/sp-platform.c line 32

Quoted 9 lines

drivers/crypto/ccp/sp-platform.c +1 -0

index 281dbf6b150c..b74f16e0e963 100644
@@ -29,6 +29,7 @@
   struct sp_platform {
       int coherent;
       unsigned int irq_count;
+    bool is_platform;

s/is_platform/is_platform_device/

drivers/crypto/ccp/sp-platform.c static int sp_platform_probe(struct platform_device *pdev) · line 195

Quoted 9 lines

drivers/crypto/ccp/sp-platform.c +2 -1

   };
     #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.

drivers/crypto/ccp/sp-platform.c static int sp_platform_probe(struct platform_device *pdev) · line 196

Quoted 1 line

drivers/crypto/ccp/sp-platform.c +0 -0

           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.

drivers/crypto/ccp/sp-platform.c static int sp_platform_probe(struct platform_device *pdev) · line 210

Quoted 9 lines

drivers/crypto/ccp/sp-platform.c +2 -1

+    }
       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

Quoted 3 lines

drivers/crypto/ccp/sp-platform.c +0 -0

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