[33/33] irqchip: Add IDXD Interrupt Message Store driver
Commit Message
Provide a driver for the Intel IDXD IMS implementation. The implementation
uses a large message store array in device memory.
The IMS domain implementation is minimal and just provides the required
irq_chip callbacks and one domain callback which prepares the MSI
descriptor which is allocated by the core for easy usage in the irq_chip
callbacks.
The necessary iobase is stored in the irqdomain and the PASID which is
required for operation is handed in via msi_dev_cookie in the allocation
function.
Not much to see here. A few lines of code and a filled in template is all
what's needed.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
drivers/irqchip/Kconfig | 7 +
drivers/irqchip/Makefile | 1
drivers/irqchip/irq-pci-intel-idxd.c | 143 +++++++++++++++++++++++++++++
include/linux/irqchip/irq-pci-intel-idxd.h | 22 ++++
4 files changed, 173 insertions(+)
Comments
Hi Thomas,
On 11/11/2022 5:59 AM, Thomas Gleixner wrote:
> Provide a driver for the Intel IDXD IMS implementation. The implementation
> uses a large message store array in device memory.
>
> The IMS domain implementation is minimal and just provides the required
> irq_chip callbacks and one domain callback which prepares the MSI
> descriptor which is allocated by the core for easy usage in the irq_chip
> callbacks.
>
> The necessary iobase is stored in the irqdomain and the PASID which is
> required for operation is handed in via msi_dev_cookie in the allocation
> function.
The use of PASID is optional for dedicated workqueues. Could this be
supported to let the irqchip support all scenarios? Since the cookie is
always provided I was wondering if an invalid PASID can be used to let
the driver disable PASID? Please see the delta snippet below in which I
primarily made such a change, but added a few more changes for
consideration.
Summary of changes:
* Use provided invalid PASID to disable PASID for the interrupt.
* Use bitmask to ensure that the cookie only contains a valid PASID.
* Modify header comment to fix typo.
* Modify header comment to reflect driver usage of macro.
With the first change I am able to test IMS on the host using devmsi-v2-part3
of the development branch. I did try to update to the most recent development
to confirm all is well but version devmsi-v3.1-part3 behaves differently
in that pci_ims_alloc_irq() returns successfully but the returned
virq is 0. This triggers a problem when request_threaded_irq() runs and
reports:
genirq: Flags mismatch irq 0. 00000000 (idxd-portal) vs. 00015a00 (timer)
Thank you very much
Reinette
---
drivers/irqchip/irq-pci-intel-idxd.c | 20 ++++++++++++++------
include/linux/irqchip/irq-pci-intel-idxd.h | 4 ++--
2 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/irqchip/irq-pci-intel-idxd.c b/drivers/irqchip/irq-pci-intel-idxd.c
index d33c32787ad5..1b49c884bd85 100644
--- a/drivers/irqchip/irq-pci-intel-idxd.c
+++ b/drivers/irqchip/irq-pci-intel-idxd.c
@@ -4,6 +4,7 @@
* interrupt message store (IMS).
*/
#include <linux/device.h>
+#include <linux/ioasid.h>
#include <linux/irq.h>
#include <linux/irqdomain.h>
#include <linux/msi.h>
@@ -33,6 +34,8 @@ struct ims_slot {
#define CTRL_PASID_ENABLE BIT(3)
/* Position of PASID.LSB in the control word */
#define CTRL_PASID_SHIFT 12
+/* Valid PASID is 20 bits */
+#define CTRL_PASID_VALID GENMASK(19, 0)
static inline void iowrite32_and_flush(u32 value, void __iomem *addr)
{
@@ -93,12 +96,17 @@ static void idxd_prepare_desc(struct irq_domain *domain, msi_alloc_info_t *arg,
/* Mask the interrupt for paranoia sake */
iowrite32_and_flush(CTRL_VECTOR_MASKBIT, &slot->ctrl);
- /*
- * The caller provided PASID. Shift it to the proper position
- * and set the PASID enable bit.
- */
- desc->data.icookie.value <<= CTRL_PASID_SHIFT;
- desc->data.icookie.value |= CTRL_PASID_ENABLE;
+ if (pasid_valid((ioasid_t)desc->data.icookie.value)) {
+ /*
+ * The caller provided PASID. Shift it to the proper position
+ * and set the PASID enable bit.
+ */
+ desc->data.icookie.value &= CTRL_PASID_VALID;
+ desc->data.icookie.value <<= CTRL_PASID_SHIFT;
+ desc->data.icookie.value |= CTRL_PASID_ENABLE;
+ } else {
+ desc->data.icookie.value = 0;
+ }
arg->hwirq = desc->msi_index;
}
diff --git a/include/linux/irqchip/irq-pci-intel-idxd.h b/include/linux/irqchip/irq-pci-intel-idxd.h
index d62ef5b3285c..48c73bffbb5d 100644
--- a/include/linux/irqchip/irq-pci-intel-idxd.h
+++ b/include/linux/irqchip/irq-pci-intel-idxd.h
@@ -9,8 +9,8 @@
#include <linux/types.h>
/*
- * Conveniance macro to wrap the PASID for interrupt allocation
- * via pci_ims_alloc_irq(pdev, INTEL_IDXD_DEV_COOKIE(pasid))
+ * Convenience macro to wrap the PASID for interrupt allocation
+ * via pci_ims_alloc_irq(pdev, &INTEL_IDXD_DEV_COOKIE(pasid))
*/
#define INTEL_IDXD_DEV_COOKIE(pasid) (union msi_instance_cookie) { .value = (pasid), }
---
Reinette!
On Fri, Dec 02 2022 at 09:55, Reinette Chatre wrote:
> On 11/11/2022 5:59 AM, Thomas Gleixner wrote:
>> The necessary iobase is stored in the irqdomain and the PASID which is
>> required for operation is handed in via msi_dev_cookie in the allocation
>> function.
>
> The use of PASID is optional for dedicated workqueues. Could this be
> supported to let the irqchip support all scenarios?
Sure. I wrote this thing mostly out of thin air based on some ancient
PoC code. :)
> Since the cookie is always provided I was wondering if an invalid
> PASID can be used to let the driver disable PASID? Please see the
> delta snippet below in which I primarily made such a change, but added
> a few more changes for consideration.
Let me check.
> With the first change I am able to test IMS on the host using devmsi-v2-part3
> of the development branch. I did try to update to the most recent development
> to confirm all is well but version devmsi-v3.1-part3 behaves differently
> in that pci_ims_alloc_irq() returns successfully but the returned
> virq is 0. This triggers a problem when request_threaded_irq() runs and
> reports:
> genirq: Flags mismatch irq 0. 00000000 (idxd-portal) vs. 00015a00 (timer)
Bah. Let me figure out what I fat-fingered there.
> @@ -33,6 +34,8 @@ struct ims_slot {
> #define CTRL_PASID_ENABLE BIT(3)
> /* Position of PASID.LSB in the control word */
> #define CTRL_PASID_SHIFT 12
> +/* Valid PASID is 20 bits */
> +#define CTRL_PASID_VALID GENMASK(19, 0)
>
> static inline void iowrite32_and_flush(u32 value, void __iomem *addr)
> {
> @@ -93,12 +96,17 @@ static void idxd_prepare_desc(struct irq_domain *domain, msi_alloc_info_t *arg,
> /* Mask the interrupt for paranoia sake */
> iowrite32_and_flush(CTRL_VECTOR_MASKBIT, &slot->ctrl);
>
> - /*
> - * The caller provided PASID. Shift it to the proper position
> - * and set the PASID enable bit.
> - */
> - desc->data.icookie.value <<= CTRL_PASID_SHIFT;
> - desc->data.icookie.value |= CTRL_PASID_ENABLE;
> + if (pasid_valid((ioasid_t)desc->data.icookie.value)) {
> + /*
> + * The caller provided PASID. Shift it to the proper position
> + * and set the PASID enable bit.
> + */
> + desc->data.icookie.value &= CTRL_PASID_VALID;
> + desc->data.icookie.value <<= CTRL_PASID_SHIFT;
> + desc->data.icookie.value |= CTRL_PASID_ENABLE;
> + } else {
> + desc->data.icookie.value = 0;
> + }
Looks about right. But that needs some sanity measures at the call sites
so that we don't end up with an invalid PASID in cases where a valid
PASID is truly required.
Thanks,
tglx
Hi Thomas,
On 12/2/2022 11:51 AM, Thomas Gleixner wrote:
> On Fri, Dec 02 2022 at 09:55, Reinette Chatre wrote:
>> On 11/11/2022 5:59 AM, Thomas Gleixner wrote:
>> @@ -33,6 +34,8 @@ struct ims_slot {
>> #define CTRL_PASID_ENABLE BIT(3)
>> /* Position of PASID.LSB in the control word */
>> #define CTRL_PASID_SHIFT 12
>> +/* Valid PASID is 20 bits */
>> +#define CTRL_PASID_VALID GENMASK(19, 0)
>>
>> static inline void iowrite32_and_flush(u32 value, void __iomem *addr)
>> {
>> @@ -93,12 +96,17 @@ static void idxd_prepare_desc(struct irq_domain *domain, msi_alloc_info_t *arg,
>> /* Mask the interrupt for paranoia sake */
>> iowrite32_and_flush(CTRL_VECTOR_MASKBIT, &slot->ctrl);
>>
>> - /*
>> - * The caller provided PASID. Shift it to the proper position
>> - * and set the PASID enable bit.
>> - */
>> - desc->data.icookie.value <<= CTRL_PASID_SHIFT;
>> - desc->data.icookie.value |= CTRL_PASID_ENABLE;
>> + if (pasid_valid((ioasid_t)desc->data.icookie.value)) {
>> + /*
>> + * The caller provided PASID. Shift it to the proper position
>> + * and set the PASID enable bit.
>> + */
>> + desc->data.icookie.value &= CTRL_PASID_VALID;
>> + desc->data.icookie.value <<= CTRL_PASID_SHIFT;
>> + desc->data.icookie.value |= CTRL_PASID_ENABLE;
>> + } else {
>> + desc->data.icookie.value = 0;
>> + }
>
> Looks about right. But that needs some sanity measures at the call sites
> so that we don't end up with an invalid PASID in cases where a valid
> PASID is truly required.
I will take a closer look at this. Current call site is explicit to
set an invalid PASID when PASID use is disabled. I still need to do testing
with valid PASID to learn those flows.
Reinette
On Fri, Dec 02 2022 at 20:51, Thomas Gleixner wrote:
> On Fri, Dec 02 2022 at 09:55, Reinette Chatre wrote:
>> With the first change I am able to test IMS on the host using devmsi-v2-part3
>> of the development branch. I did try to update to the most recent development
>> to confirm all is well but version devmsi-v3.1-part3 behaves differently
>> in that pci_ims_alloc_irq() returns successfully but the returned
>> virq is 0. This triggers a problem when request_threaded_irq() runs and
>> reports:
>> genirq: Flags mismatch irq 0. 00000000 (idxd-portal) vs. 00015a00 (timer)
>
> Bah. Let me figure out what I fat-fingered there.
tag devmsi-v3.2-part3 works again.
Hi Thomas,
On 12/5/2022 7:20 AM, Thomas Gleixner wrote:
> On Fri, Dec 02 2022 at 20:51, Thomas Gleixner wrote:
>> On Fri, Dec 02 2022 at 09:55, Reinette Chatre wrote:
>>> With the first change I am able to test IMS on the host using devmsi-v2-part3
>>> of the development branch. I did try to update to the most recent development
>>> to confirm all is well but version devmsi-v3.1-part3 behaves differently
>>> in that pci_ims_alloc_irq() returns successfully but the returned
>>> virq is 0. This triggers a problem when request_threaded_irq() runs and
>>> reports:
>>> genirq: Flags mismatch irq 0. 00000000 (idxd-portal) vs. 00015a00 (timer)
>>
>> Bah. Let me figure out what I fat-fingered there.
>
> tag devmsi-v3.2-part3 works again.
Thank you very much.
This tag is not yet available but I can confirm that the current tip of
devmsi, 6bd4ee6cb126 ("irqchip: Add IDXD Interrupt Message Store driver"),
combined with the earlier irqchip driver delta snippet passes the "dedicated
kernel work queue using host IMS" tests.
Reinette
@@ -695,4 +695,11 @@ config SUNPLUS_SP7021_INTC
chained controller, routing all interrupt source in P-Chip to
the primary controller on C-Chip.
+config PCI_INTEL_IDXD_IMS
+ tristate "Intel IDXD Interrupt Message Store controller"
+ depends on PCI_MSI
+ help
+ Support for Intel IDXD IMS Interrupt Message Store controller
+ with IMS slot storage in a slot array in device memory
+
endmenu
@@ -121,3 +121,4 @@ obj-$(CONFIG_IRQ_IDT3243X) += irq-idt32
obj-$(CONFIG_APPLE_AIC) += irq-apple-aic.o
obj-$(CONFIG_MCHP_EIC) += irq-mchp-eic.o
obj-$(CONFIG_SUNPLUS_SP7021_INTC) += irq-sp7021-intc.o
+obj-$(CONFIG_PCI_INTEL_IDXD_IMS) += irq-pci-intel-idxd.o
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Interrupt chip and domain for Intel IDXD with hardware array based
+ * interrupt message store (IMS).
+ */
+#include <linux/device.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/msi.h>
+#include <linux/pci.h>
+
+#include <linux/irqchip/irq-pci-intel-idxd.h>
+
+MODULE_LICENSE("GPL");
+
+/**
+ * struct ims_slot - The hardware layout of a slot in the memory table
+ * @address_lo: Lower 32bit address
+ * @address_hi: Upper 32bit address
+ * @data: Message data
+ * @ctrl: Control word
+ */
+struct ims_slot {
+ u32 address_lo;
+ u32 address_hi;
+ u32 data;
+ u32 ctrl;
+} __packed;
+
+/* Bit to mask the interrupt in the control word */
+#define CTRL_VECTOR_MASKBIT BIT(0)
+/* Bit to enable PASID in the control word */
+#define CTRL_PASID_ENABLE BIT(3)
+/* Position of PASID.LSB in the control word */
+#define CTRL_PASID_SHIFT 12
+
+static inline void iowrite32_and_flush(u32 value, void __iomem *addr)
+{
+ iowrite32(value, addr);
+ ioread32(addr);
+}
+
+static void idxd_mask(struct irq_data *data)
+{
+ struct msi_desc *desc = irq_data_get_msi_desc(data);
+ struct ims_slot __iomem *slot = desc->data.iobase;
+ u32 cval = desc->data.cookie.value;
+
+ iowrite32_and_flush(cval | CTRL_VECTOR_MASKBIT, &slot->ctrl);
+}
+
+static void idxd_unmask(struct irq_data *data)
+{
+ struct msi_desc *desc = irq_data_get_msi_desc(data);
+ struct ims_slot __iomem *slot = desc->data.iobase;
+ u32 cval = desc->data.cookie.value;
+
+ iowrite32_and_flush(cval, &slot->ctrl);
+}
+
+static void idxd_write_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+ struct msi_desc *desc = irq_data_get_msi_desc(data);
+ struct ims_slot __iomem *slot = desc->data.iobase;
+
+ iowrite32(msg->address_lo, &slot->address_lo);
+ iowrite32(msg->address_hi, &slot->address_hi);
+ iowrite32_and_flush(msg->data, &slot->data);
+}
+
+static void idxd_shutdown(struct irq_data *data)
+{
+ struct msi_desc *desc = irq_data_get_msi_desc(data);
+ struct ims_slot __iomem *slot = desc->data.iobase;
+
+ iowrite32(0, &slot->address_lo);
+ iowrite32(0, &slot->address_hi);
+ iowrite32(0, &slot->data);
+ iowrite32_and_flush(CTRL_VECTOR_MASKBIT, &slot->ctrl);
+}
+
+static void idxd_prepare_desc(struct irq_domain *domain, msi_alloc_info_t *arg,
+ struct msi_desc *desc)
+{
+ struct msi_domain_info *info = domain->host_data;
+ struct ims_slot __iomem *slot;
+
+ /* Set up the slot address for the irq_chip callbacks */
+ slot = (__force struct ims_slot __iomem *) info->data;
+ slot += desc->msi_index;
+ desc->data.iobase = slot;
+
+ /* Mask the interrupt for paranoia sake */
+ iowrite32_and_flush(CTRL_VECTOR_MASKBIT, &slot->ctrl);
+
+ /*
+ * The caller provided PASID. Shift it to the proper position
+ * and set the PASID enable bit.
+ */
+ desc->data.cookie.value <<= CTRL_PASID_SHIFT;
+ desc->data.cookie.value |= CTRL_PASID_ENABLE;
+
+ arg->hwirq = desc->msi_index;
+}
+
+static const struct msi_domain_template idxd_ims_template = {
+ .chip = {
+ .name = "PCI-IDXD",
+ .irq_mask = idxd_mask,
+ .irq_unmask = idxd_unmask,
+ .irq_write_msi_msg = idxd_write_msi_msg,
+ .irq_shutdown = idxd_shutdown,
+ .flags = IRQCHIP_ONESHOT_SAFE,
+ },
+
+ .ops = {
+ .prepare_desc = idxd_prepare_desc,
+ },
+
+ .info = {
+ .flags = MSI_FLAG_ALLOC_SIMPLE_MSI_DESCS |
+ MSI_FLAG_FREE_MSI_DESCS |
+ MSI_FLAG_PCI_IMS,
+ .bus_token = DOMAIN_BUS_PCI_DEVICE_IMS,
+ },
+};
+
+/**
+ * pci_intel_idxd_create_ims_domain - Create a IDXD IMS domain
+ * @pdev: IDXD PCI device to operate on
+ * @slots: Pointer to the mapped slot memory arrray
+ * @nr_slots: The number of slots in the array
+ *
+ * Returns: True on success, false otherwise
+ *
+ * The domain is automatically destroyed when the @pdev is destroyed
+ */
+bool pci_intel_idxd_create_ims_domain(struct pci_dev *pdev, void __iomem *slots,
+ unsigned int nr_slots)
+{
+ return pci_create_ims_domain(pdev, &idxd_ims_template, nr_slots, (__force void *)slots);
+}
+EXPORT_SYMBOL_GPL(pci_intel_idxd_create_ims_domain);
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* (C) Copyright 2022 Thomas Gleixner <tglx@linutronix.de> */
+
+#ifndef _LINUX_IRQCHIP_IRQ_PCI_INTEL_IDXD_H
+#define _LINUX_IRQCHIP_IRQ_PCI_INTEL_IDXD_H
+
+#include <linux/msi_api.h>
+#include <linux/bits.h>
+#include <linux/types.h>
+
+/*
+ * Conveniance macro to wrap the PASID for interrupt allocation
+ * via pci_ims_alloc_irq(pdev, INTEL_IDXD_DEV_COOKIE(pasid))
+ */
+#define INTEL_IDXD_DEV_COOKIE(pasid) (union msi_dev_cookie) { .value = (pasid), }
+
+struct pci_dev;
+
+bool pci_intel_idxd_create_ims_domain(struct pci_dev *pdev, void __iomem *slots,
+ unsigned int nr_slots);
+
+#endif