| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 31 Jan 2023 13:45:48 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>,
linux-kernel@...r.kernel.org
Cc: Brijesh Singh <brijesh.singh@....com>,
"Kalra, Ashish" <ashish.kalra@....com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org
Subject: Re: [PATCH v1 4/8] x86/psp: Add IRQ support
On 1/23/23 09:22, Jeremi Piotrowski wrote:
> The ACPI PSP device provides a mailbox irq that needs to be configured
> through the ACPI mailbox register first. This requires passing a CPU
> vector and physical CPU id and then enabling interrupt delivery.
> Allocate the irq directly from the default irq domain
> (x86_vector_domain) to get access to the required information. By
> passing a cpumask through irq_alloc_info the vector is immediately
> allocated (and not later during activation) and can be retrieved.
>
> Signed-off-by: Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>
> ---
> arch/x86/kernel/psp.c | 179 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 175 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c
> index d404df47cc04..24181d132bae 100644
> --- a/arch/x86/kernel/psp.c
> +++ b/arch/x86/kernel/psp.c
> @@ -1,8 +1,176 @@
> // SPDX-License-Identifier: GPL-2.0-only
> -
> +#define pr_fmt(fmt) "psp: " fmt
> #include <linux/platform_data/psp.h>
> #include <linux/platform_device.h>
> +#include <linux/iopoll.h>
> +#include <linux/irq.h>
> #include <asm/hypervisor.h>
> +#include <asm/irqdomain.h>
> +
> +#define PSP_ACPI_DATA_SHIFT 0
> +#define PSP_ACPI_DATA_MASK GENMASK(15, 0)
> +#define PSP_ACPI_CMDID_SHIFT 16
> +#define PSP_ACPI_CMDID_MASK GENMASK(25, 16)
> +#define PSP_ACPI_STATUS_SHIFT 26
> +#define PSP_ACPI_STATUS_MASK GENMASK(30, 26)
> +#define PSP_ACPI_RESPONSE_BIT BIT(31)
> +
> +#define PSP_ACPI_VECTOR_SHIFT 0
> +#define PSP_ACPI_VECTOR_MASK GENMASK(7, 0)
> +#define PSP_ACPI_MBOX_IRQID_SHIFT 10
> +#define PSP_ACPI_MBOX_IRQID_MASK GENMASK(15, 10)
> +
> +#define PSP_ACPI_IRQ_EN_BIT BIT(0)
> +#define PSP_ACPI_IRQ_EN_MBOX_IRQID_SHIFT 10
> +#define PSP_ACPI_IRQ_EN_MBOX_IRQID_MASK GENMASK(15, 10)
> +
> +// AMD Secure Processor
> +enum ASP_CMDID {
> + ASP_CMDID_PART1 = 0x82,
> + ASP_CMDID_PART2 = 0x83,
> + ASP_CMDID_PART3 = 0x84,
> + ASP_CMDID_IRQ_EN = 0x85,
> +};
> +
> +enum ASP_CMD_STATUS {
> + ASP_CMD_STATUS_SUCCESS = 0x0,
> + ASP_CMD_STATUS_INVALID_CMD = 0x1,
> + ASP_CMD_STATUS_INVALID_PARAM = 0x2,
> + ASP_CMD_STATUS_INVALID_FW_STATE = 0x3,
> + ASP_CMD_STATUS_FAILURE = 0x1F,
> +};
> +
> +struct psp_irq_data {
> + void __iomem *base;
> + int mbox_irq_id;
> + int acpi_cmd_resp_reg;
> +};
> +
> +static int psp_sync_cmd(void __iomem *reg, u8 cmd, u16 data)
> +{
> + u32 val = 0;
Remove this initialization and ...
> + int err;
> +
> + val |= data & PSP_ACPI_DATA_MASK;
... just make this "val = " which sets the initial value. And if the mask
is 16 bits and the data is 16 bits, there's no need for the ANDing
> + val |= (cmd << PSP_ACPI_CMDID_SHIFT) & PSP_ACPI_CMDID_MASK;
Same here, the mask is 8 bits and the cmd is 8 bits...
> + writel(val, reg);
> + err = readl_poll_timeout_atomic(reg, val, val & PSP_ACPI_RESPONSE_BIT, 2, 10000);
Use some #defines with informative names for the 2 and 10000.
> + if (err < 0)
This can just be "if (err)" since readl_poll_timeout_atomic() returns 0 or
-ETIMEDOUT.
> + return err;
Blank line...
> + return (val & PSP_ACPI_STATUS_MASK) >> PSP_ACPI_STATUS_SHIFT;
> +}
> +
> +static int psp_set_irq_enable(struct psp_irq_data *data, bool irq_en)
> +{
> + void __iomem *reg = data->base + data->acpi_cmd_resp_reg;
> + u8 mbox_irq_id = data->mbox_irq_id;
Why not just make the mbox_irq_id value a u8 in the struct and eliminate
this variable (here and in psp_configure_irq()).
> + u16 val = 0;
Remove this initialization and ...
> + int err;
> +
> + val |= irq_en ? PSP_ACPI_IRQ_EN_BIT : 0;
... just make this "val = " which sets the initial value.
> + val |= (mbox_irq_id << PSP_ACPI_IRQ_EN_MBOX_IRQID_SHIFT) & PSP_ACPI_IRQ_EN_MBOX_IRQID_MASK;
This could possibly truncate the value specified. It would be better to
check for the max value that mbox_irq_id should be (6-bits == 63) and if
greater, return an error. Then you don't need to do the ANDing.
> + err = psp_sync_cmd(reg, ASP_CMDID_IRQ_EN, val);
> + if (err != ASP_CMD_STATUS_SUCCESS) {
> + pr_err("ASP_CMDID_IRQ_EN failed: %d\n", err);
> + return -EIO;
> + }
Blank line...
> + return 0;
> +}
> +
> +static int psp_configure_irq(struct psp_irq_data *data, int vector, unsigned int cpu)
> +{
> + void __iomem *reg = data->base + data->acpi_cmd_resp_reg;
> + unsigned int dest_cpu = cpu_physical_id(cpu);
> + u8 mbox_irq_id = data->mbox_irq_id;
> + u16 part1, part2, part3;
> + int err;
> +
> + part1 = dest_cpu;
> + part2 = dest_cpu >> 16;
> + part3 = vector & PSP_ACPI_VECTOR_MASK;
> + part3 |= (mbox_irq_id << PSP_ACPI_MBOX_IRQID_SHIFT) & PSP_ACPI_MBOX_IRQID_MASK;
Same comment about mbox_irq_id as above.
> +
> + err = psp_sync_cmd(reg, ASP_CMDID_PART1, part1);
> + if (err != ASP_CMD_STATUS_SUCCESS) {
> + pr_err("ASP_CMDID_PART1 failed: %d\n", err);
> + return -EIO;
> + }
> + err = psp_sync_cmd(reg, ASP_CMDID_PART2, part2);
> + if (err != ASP_CMD_STATUS_SUCCESS) {
> + pr_err("ASP_CMDID_PART2 failed: %d\n", err);
> + return -EIO;
> + }
> + err = psp_sync_cmd(reg, ASP_CMDID_PART3, part3);
> + if (err != ASP_CMD_STATUS_SUCCESS) {
> + pr_err("ASP_CMDID_PART3 failed: %d\n", err);
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +
> +static int psp_init_irq(const struct psp_platform_data *pdata,
> + const struct resource *reg, struct resource *irq)
This needs to be aligned under the first parameter of the line above.
> +{
> + struct psp_irq_data pspirqd;
> + struct irq_alloc_info info;
> + struct irq_data *data = NULL;
> + struct irq_cfg *cfg = NULL;
> + void __iomem *base = NULL;
Why are these being initialized to NULL, they aren't check or returned
before being set.
> + int virq;
> + int err;
> +
> + base = ioremap(reg->start, resource_size(reg));
> + if (IS_ERR(base))
> + return PTR_ERR(base);
ioremap() returns NULL or a valid address, so the above check isn't necessary.
> + if (!base)
> + return -ENOMEM;
Add a blank line here.
> + pspirqd.mbox_irq_id = pdata->mbox_irq_id;
> + pspirqd.acpi_cmd_resp_reg = pdata->acpi_cmd_resp_reg;
> + pspirqd.base = base;
Add a blank line here.
> + init_irq_alloc_info(&info, cpumask_of(0));
> + virq = irq_domain_alloc_irqs(NULL, 1, NUMA_NO_NODE, &info);
> + if (virq <= 0) {
> + pr_err("failed to allocate vector: %d\n", virq);
> + err = -ENOMEM;
> + goto unmap;
> + }
> + irq_set_handler(virq, handle_edge_irq);
Blank line...
> + data = irq_get_irq_data(virq);
> + if (!data) {
> + pr_err("no irq data\n");
> + err = -ENODEV;
> + goto freeirq;
> +
> + }
Blank line...
> + cfg = irqd_cfg(data);
> + if (!data) {
s/data/cfg/
> + pr_err("no irq cfg\n");
> + err = -ENODEV;
> + goto freeirq;
> + }
Blank line...
> + err = psp_configure_irq(&pspirqd, cfg->vector, 0);
> + if (err) {
> + pr_err("failed to configure irq: %d\n", err);
> + goto freeirq;
> + }
Blank line...
> + err = psp_set_irq_enable(&pspirqd, true);
> + if (err) {
> + pr_err("failed to enable irq: %d\n", err);
> + goto freeirq;
> + }
> +
> + *irq = (struct resource)DEFINE_RES_IRQ(virq);
Blank line...
> + iounmap(base);
Blank line...
> + return 0;
> +
> +freeirq:
> + irq_domain_free_irqs(virq, 1);
Blank line...
> +unmap:
> + iounmap(base);
Blank line...
Thanks,
Tom
> + return err;
> +}
>
> static struct platform_device psp_device = {
> .name = "psp",
> @@ -12,7 +180,7 @@ static struct platform_device psp_device = {
> static int __init psp_init_platform_device(void)
> {
> struct psp_platform_data pdata = {};
> - struct resource res[1];
> + struct resource res[2];
> int err;
>
> /*
> @@ -24,10 +192,13 @@ static int __init psp_init_platform_device(void)
> if (!hypervisor_is_type(X86_HYPER_MS_HYPERV))
> return -ENODEV;
>
> - err = acpi_parse_aspt(res, &pdata);
> + err = acpi_parse_aspt(&res[0], &pdata);
> + if (err)
> + return err;
> + err = psp_init_irq(&pdata, &res[0], &res[1]);
> if (err)
> return err;
> - err = platform_device_add_resources(&psp_device, res, 1);
> + err = platform_device_add_resources(&psp_device, res, 2);
> if (err)
> return err;
>
Powered by blists - more mailing lists