| Message ID | 20221111135207.141746268@linutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | genirq, PCI/MSI: Support for per device MSI and PCI/IMS - Part 3 implementation | expand |
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
--- 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
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(+)