[v3,4/8] x86/psp: Add IRQ support
Commit Message
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 <thomas.lendacky@amd.com>
Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
---
arch/x86/kernel/psp.c | 185 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 181 insertions(+), 4 deletions(-)
Comments
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 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 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
@@ -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));