[33/33] irqchip: Add IDXD Interrupt Message Store driver

Message ID 20221111135207.141746268@linutronix.de
State New
Headers
Series genirq, PCI/MSI: Support for per device MSI and PCI/IMS - Part 3 implementation |

Commit Message

Thomas Gleixner Nov. 11, 2022, 1:59 p.m. UTC
  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

Reinette Chatre Dec. 2, 2022, 5:55 p.m. UTC | #1
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), }
 
---
  
Thomas Gleixner Dec. 2, 2022, 7:51 p.m. UTC | #2
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
  
Reinette Chatre Dec. 2, 2022, 9:16 p.m. UTC | #3
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
  
Thomas Gleixner Dec. 5, 2022, 3:20 p.m. UTC | #4
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.
  
Reinette Chatre Dec. 5, 2022, 5:19 p.m. UTC | #5
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
  

Patch

--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -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
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -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
--- /dev/null
+++ b/drivers/irqchip/irq-pci-intel-idxd.c
@@ -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);
--- /dev/null
+++ b/include/linux/irqchip/irq-pci-intel-idxd.h
@@ -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