diff mbox series

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

Message ID 20230320191956.1354602-4-jpiotrowski@linux.microsoft.com
State New
Headers
Series Support ACPI PSP on Hyper-V |

Commit Message

Jeremi Piotrowski March 20, 2023, 7:19 p.m. UTC 
  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 <thomas.lendacky@amd.com>
Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
---
 arch/x86/kernel/Makefile |  1 +
 arch/x86/kernel/psp.c    | 42 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)
 create mode 100644 arch/x86/kernel/psp.c

Comments

Jeremi Piotrowski March 20, 2023, 7:37 p.m. UTC | #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 <thomas.lendacky@amd.com>
>> Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
>> ---
>>  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
Borislav Petkov March 20, 2023, 8:03 p.m. UTC | #2 
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.
Jeremi Piotrowski March 20, 2023, 8:18 p.m. UTC | #3 
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.
Borislav Petkov March 20, 2023, 9:03 p.m. UTC | #4 
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?
Jeremi Piotrowski March 21, 2023, 2:15 p.m. UTC | #5 
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.
diff mbox series

Patch

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