Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Tue, 31 Jan 2023 13:45:48 -0600 | Subject | Re: [PATCH v1 4/8] x86/psp: Add IRQ support | From | Tom Lendacky <> |
| |
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@linux.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; >
| ![\](/images/icornerr.gif) |