genirq/msi: Fix device MSI prepare/alloc sequencing
Subsystems (5)
Marc Zyngier
maintainerv2 cover genirq/msi: Fix device MSI prepare/alloc sequencing
While reviewing (and debugging) the GICv5 code, it became obvious that there was something pretty fishy about the way MSI allocation was taking place in respect to the msi_prepare() callback.
There is a strong (though not 100% explicit) requirement that the .msi_prepare() callback must take place exactly once during the lifetime of an MSI domain, before any MSI is allocated. Sadly, that's not how the core code behaves, leading to all sorts of "interesting" issues with MBIGEN or the GICv5 IWB.
The fix is both simple and surprisingly convoluted. The first course of action would be to hoist the "prepare" action before we allocate any MSI, adding new tracking for the msi_alloc_info_t structure.
But this unveils another issue that the core code has been papering over, which is that there is no pendant to the .msi_prepare() callback that would be called on irqdomain destruction, and that at least the ITS code has been using some crap heuristics to work around it.
So this needs to be addressed first, and that's the point of the first two patches, introducing and implementing the .msi_teardown() callback. The .msi_prepare() call is then moved is the specific case of device-MSI, and the .msi_teardown() callback wired up.
The last patch remove a gross hack in the ITS msi-parent code, which was papering over the broken use of the "prepare" callback.
This has been tested on a GICv4 system with MBIGEN, patches on top of tip/irq/msi (a1d8a83093675).
* From v1 [1]:
- Fixed blunder in the ITS .msi_teardown()
- Fixed spelling, grammar, and other use of unclear terms
- Fixed line splitting, disgraceful underscores
- Split the calling of .msi_teardown() into its own patch
- Rebased on top of tip/irq/msi
[1] https://lore.kernel.org/r/[email protected]
Marc Zyngier (5):
genirq/msi: Add .msi_teardown() callback as the reverse of
.msi_prepare()
irqchip/gic-v3-its: Implement .msi_teardown() callback genirq/msi: Move prepare() call to per-device allocation genirq/msi: Engage the .msi_teardown() callback on domain removal irqchip/gic-v3-its: Use allocation size from the prepare call
drivers/irqchip/irq-gic-v3-its-msi-parent.c | 29 +++++-------- drivers/irqchip/irq-gic-v3-its.c | 46 +++++++++++---------- include/linux/msi.h | 12 +++++- kernel/irq/msi.c | 45 ++++++++++++++++++-- 4 files changed, 86 insertions(+), 46 deletions(-)
Signature · 1 line
2.39.2
Marc Zyngier
maintainerPATCH v2 1/5 genirq/msi: Add .msi_teardown() callback as the reverse of .msi_prepare()
While the MSI ops do have a .msi_prepare() callback that is responsible for setting up the relevant (usually per-device) allocation, there is no callback reversing this setup.
For this purpose, add .msi_teardown() callback.
In order to avoid breaking the ITS driver that suffers from related issues, do not call the call back just yet.
--- include/linux/msi.h | 10 ++++++++-- kernel/irq/msi.c | 8 ++++++++ 2 files changed, 16 insertions(+), 2 deletions(-)
include/linux/msi.h +8 -2
@@ -423,6 +423,7 @@ struct msi_domain_info;* @msi_init: Domain specific init function for MSI interrupts * @msi_free: Domain specific function to free a MSI interrupts * @msi_prepare: Prepare the allocation of the interrupts in the domain +* @msi_teardown: Reverse the effects of @msi_prepare * @prepare_desc: Optional function to prepare the allocated MSI descriptor * in the domain * @set_desc: Set the msi descriptor for an interrupt
@@ -438,8 +439,9 @@ struct msi_domain_info;* @get_hwirq, @msi_init and @msi_free are callbacks used by the underlying * irqdomain. * -* @msi_check, @msi_prepare, @prepare_desc and @set_desc are callbacks used by the -* msi_domain_alloc/free_irqs*() variants. +* @msi_check, @msi_prepare, @msi_teardown, @prepare_desc and +* @set_desc are callbacks used by the msi_domain_alloc/free_irqs*() +* variants. * * @domain_alloc_irqs, @domain_free_irqs can be used to override the * default allocation/free functions (__msi_domain_alloc/free_irqs). This
@@ -461,6 +463,8 @@ struct msi_domain_ops {int (*msi_prepare)(struct irq_domain *domain,struct device *dev,int nvec,msi_alloc_info_t *arg); +void (*msi_teardown)(struct irq_domain *domain, +msi_alloc_info_t *arg);void (*prepare_desc)(struct irq_domain *domain,msi_alloc_info_t *arg,struct msi_desc *desc); void (*set_desc)(msi_alloc_info_t *arg,
@@ -489,6 +493,7 @@ struct msi_domain_ops {* @handler: Optional: associated interrupt flow handler * @handler_data: Optional: associated interrupt flow handler data * @handler_name: Optional: associated interrupt flow handler name +* @alloc_data: Optional: associated interrupt allocation data * @data: Optional: domain specific data */ struct msi_domain_info {
@@ -501,6 +506,7 @@ struct msi_domain_info {irq_flow_handler_t handler;void *handler_data;const char *handler_name; +msi_alloc_info_t *alloc_data;void *data; };
kernel/irq/msi.c +8 -0
@@ -795,6 +795,11 @@ static int msi_domain_ops_prepare(struct irq_domain *domain, struct device *dev,return 0 ; } +static void msi_domain_ops_teardown (struct irq_domain *domain, +msi_alloc_info_t *arg) +{ +} +static void msi_domain_ops_set_desc (msi_alloc_info_t *arg,struct msi_desc *desc) {
@@ -820,6 +825,7 @@ static struct msi_domain_ops msi_domain_ops_default = { .get_hwirq = msi_domain_ops_get_hwirq, .msi_init = msi_domain_ops_init, .msi_prepare = msi_domain_ops_prepare, + .msi_teardown = msi_domain_ops_teardown, .set_desc = msi_domain_ops_set_desc, };
@@ -841,6 +847,8 @@ static void msi_domain_update_dom_ops(struct msi_domain_info *info) ops->msi_init = msi_domain_ops_default.msi_init;if (ops->msi_prepare ==NULL ) ops->msi_prepare = msi_domain_ops_default.msi_prepare; +if (ops->msi_teardown ==NULL ) + ops->msi_teardown = msi_domain_ops_default.msi_teardown;if (ops->set_desc ==NULL ) ops->set_desc = msi_domain_ops_default.set_desc; }
Signature · 1 line
2.39.2
[tip: irq/msi] genirq/msi: Add .msi_teardown() callback as the reverse of .msi_prepare()
The following commit has been merged into the irq/msi branch of tip:
The following commit has been merged into the irq/msi branch of tip:
Commit-ID: 28026cf2dd84d961a62123b1fa941dc3c2c4a132 Gitweb: https://git.kernel.org/tip/28026cf2dd84d961a62123b1fa941dc3c2c4a132 Author: Marc Zyngier <[email protected]> AuthorDate: Tue, 13 May 2025 17:31:40 +01:00 Committer: Thomas Gleixner <[email protected]> CommitterDate: Wed, 14 May 2025 12:36:41 +02:00
genirq/msi: Add .msi_teardown() callback as the reverse of .msi_prepare()
While the MSI ops do have a .msi_prepare() callback that is responsible for setting up the relevant (usually per-device) allocation, there is no callback reversing this setup.
For this purpose, add .msi_teardown() callback.
In order to avoid breaking the ITS driver that suffers from related issues, do not call the callback just yet.
--- include/linux/msi.h | 10 ++++++++-- kernel/irq/msi.c | 8 ++++++++ 2 files changed, 16 insertions(+), 2 deletions(-)
include/linux/msi.h +8 -2
@@ -423,6 +423,7 @@ struct msi_domain_info;* @msi_init: Domain specific init function for MSI interrupts * @msi_free: Domain specific function to free a MSI interrupts * @msi_prepare: Prepare the allocation of the interrupts in the domain +* @msi_teardown: Reverse the effects of @msi_prepare * @prepare_desc: Optional function to prepare the allocated MSI descriptor * in the domain * @set_desc: Set the msi descriptor for an interrupt
@@ -438,8 +439,9 @@ struct msi_domain_info;* @get_hwirq, @msi_init and @msi_free are callbacks used by the underlying * irqdomain. * -* @msi_check, @msi_prepare, @prepare_desc and @set_desc are callbacks used by the -* msi_domain_alloc/free_irqs*() variants. +* @msi_check, @msi_prepare, @msi_teardown, @prepare_desc and +* @set_desc are callbacks used by the msi_domain_alloc/free_irqs*() +* variants. * * @domain_alloc_irqs, @domain_free_irqs can be used to override the * default allocation/free functions (__msi_domain_alloc/free_irqs). This
@@ -461,6 +463,8 @@ struct msi_domain_ops {int (*msi_prepare)(struct irq_domain *domain,struct device *dev,int nvec,msi_alloc_info_t *arg); +void (*msi_teardown)(struct irq_domain *domain, +msi_alloc_info_t *arg);void (*prepare_desc)(struct irq_domain *domain,msi_alloc_info_t *arg,struct msi_desc *desc); void (*set_desc)(msi_alloc_info_t *arg,
@@ -489,6 +493,7 @@ struct msi_domain_ops {* @handler: Optional: associated interrupt flow handler * @handler_data: Optional: associated interrupt flow handler data * @handler_name: Optional: associated interrupt flow handler name +* @alloc_data: Optional: associated interrupt allocation data * @data: Optional: domain specific data */ struct msi_domain_info {
@@ -501,6 +506,7 @@ struct msi_domain_info {irq_flow_handler_t handler;void *handler_data;const char *handler_name; +msi_alloc_info_t *alloc_data;void *data; };
kernel/irq/msi.c +8 -0
@@ -795,6 +795,11 @@ static int msi_domain_ops_prepare(struct irq_domain *domain, struct device *dev,return 0 ; } +static void msi_domain_ops_teardown (struct irq_domain *domain, +msi_alloc_info_t *arg) +{ +} +static void msi_domain_ops_set_desc (msi_alloc_info_t *arg,struct msi_desc *desc) {
@@ -820,6 +825,7 @@ static struct msi_domain_ops msi_domain_ops_default = { .get_hwirq = msi_domain_ops_get_hwirq, .msi_init = msi_domain_ops_init, .msi_prepare = msi_domain_ops_prepare, + .msi_teardown = msi_domain_ops_teardown, .set_desc = msi_domain_ops_set_desc, };
@@ -841,6 +847,8 @@ static void msi_domain_update_dom_ops(struct msi_domain_info *info) ops->msi_init = msi_domain_ops_default.msi_init;if (ops->msi_prepare ==NULL ) ops->msi_prepare = msi_domain_ops_default.msi_prepare; +if (ops->msi_teardown ==NULL ) + ops->msi_teardown = msi_domain_ops_default.msi_teardown;if (ops->set_desc ==NULL ) ops->set_desc = msi_domain_ops_default.set_desc; }
Marc Zyngier
maintainerPATCH v2 2/5 irqchip/gic-v3-its: Implement .msi_teardown() callback
In reply to Marc ZyngierThe ITS driver currently nukes the structure representing an endpoint device translating via an ITS on freeing the last LPI allocated for it.
That's an unfortunate state of affair, as it is pretty common for a driver to allocate a single MSI, do something clever, teardown this MSI, and reallocate a whole bunch of them. The nvme driver does exactly that, amongst others.
What happens in that case is that the core code is buggy enough to issue another .msi_prepare() call, even if it shouldn't. This luckily cancels the above behaviour and hides the problem.
In order to fix the core code, start by implementing the new .msi_teardown() callback. Nothing calls it yet, so a side effect is that the its_dev structure will not be freed and that the DID will stay mapped. Not a big deal, and this will be solved in following patches.
--- drivers/irqchip/irq-gic-v3-its-msi-parent.c | 10 +++++ drivers/irqchip/irq-gic-v3-its.c | 46 +++++++++++---------- kernel/irq/msi.c | 3 +- 3 files changed, 36 insertions(+), 23 deletions(-)
drivers/irqchip/irq-gic-v3-its-msi-parent.c +10 -0
@@ -167,6 +167,14 @@ static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev, dev, nvec, info); } +static void its_msi_teardown (struct irq_domain *domain,msi_alloc_info_t *info) +{ +struct msi_domain_info *msi_info; + + msi_info =msi_get_domain_info (domain->parent); + msi_info->ops->msi_teardown (domain->parent, info); +} +static bool its_init_dev_msi_info (struct device *dev,struct irq_domain *domain,struct irq_domain *real_parent,struct msi_domain_info *info) {
@@ -190,6 +198,7 @@ static bool its_init_dev_msi_info(struct device *dev, struct irq_domain *domain,* %MSI_MAX_INDEX. */ info->ops->msi_prepare = its_pci_msi_prepare; + info->ops->msi_teardown = its_msi_teardown;break ;case DOMAIN_BUS_DEVICE_MSI :case DOMAIN_BUS_WIRED_TO_MSI :
@@ -198,6 +207,7 @@ static bool its_init_dev_msi_info(struct device *dev, struct irq_domain *domain,* size is also known at domain creation time. */ info->ops->msi_prepare = its_pmsi_prepare; + info->ops->msi_teardown = its_msi_teardown;break ;default :/* Confused. How did the lib return true? */
drivers/irqchip/irq-gic-v3-its.c +25 -21
66 lines not on this page · raw message
kernel/irq/msi.c +1 -2
@@ -795,8 +795,7 @@ static int msi_domain_ops_prepare(struct irq_domain *domain, struct device *dev,return 0 ; } -static void msi_domain_ops_teardown (struct irq_domain *domain, -msi_alloc_info_t *arg) +static void msi_domain_ops_teardown (struct irq_domain *domain,msi_alloc_info_t *arg) { }
Signature · 1 line
2.39.2
[tip: irq/msi] irqchip/gic-v3-its: Implement .msi_teardown() callback
The following commit has been merged into the irq/msi branch of tip:
The following commit has been merged into the irq/msi branch of tip:
Commit-ID: 713335b6ee29f0045737a1ddfc685bc6040d4baf Gitweb: https://git.kernel.org/tip/713335b6ee29f0045737a1ddfc685bc6040d4baf Author: Marc Zyngier <[email protected]> AuthorDate: Tue, 13 May 2025 17:31:41 +01:00 Committer: Thomas Gleixner <[email protected]> CommitterDate: Wed, 14 May 2025 12:36:41 +02:00
irqchip/gic-v3-its: Implement .msi_teardown() callback
The ITS driver currently nukes the structure representing an endpoint device translating via an ITS on freeing the last LPI allocated for it.
That's an unfortunate state of affair, as it is pretty common for a driver to allocate a single MSI, do something clever, teardown this MSI, and reallocate a whole bunch of them. The NVME driver does exactly that, amongst others.
What happens in that case is that the core code is accidentaly issuing another .msi_prepare() call, even if it shouldn't. This luckily cancels the above behaviour and hides the problem.
In order to fix the core code, start by implementing the new .msi_teardown() callback. Nothing calls it yet, so a side effect is that the its_dev structure will not be freed and that the DID will stay mapped. Not a big deal, and this will be solved in following patches.
--- drivers/irqchip/irq-gic-v3-its-msi-parent.c | 10 ++++- drivers/irqchip/irq-gic-v3-its.c | 46 ++++++++++---------- kernel/irq/msi.c | 3 +- 3 files changed, 36 insertions(+), 23 deletions(-)
drivers/irqchip/irq-gic-v3-its-msi-parent.c +10 -0
@@ -167,6 +167,14 @@ static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev, dev, nvec, info); } +static void its_msi_teardown (struct irq_domain *domain,msi_alloc_info_t *info) +{ +struct msi_domain_info *msi_info; + + msi_info =msi_get_domain_info (domain->parent); + msi_info->ops->msi_teardown (domain->parent, info); +} +static bool its_init_dev_msi_info (struct device *dev,struct irq_domain *domain,struct irq_domain *real_parent,struct msi_domain_info *info) {
@@ -190,6 +198,7 @@ static bool its_init_dev_msi_info(struct device *dev, struct irq_domain *domain,* %MSI_MAX_INDEX. */ info->ops->msi_prepare = its_pci_msi_prepare; + info->ops->msi_teardown = its_msi_teardown;break ;case DOMAIN_BUS_DEVICE_MSI :case DOMAIN_BUS_WIRED_TO_MSI :
@@ -198,6 +207,7 @@ static bool its_init_dev_msi_info(struct device *dev, struct irq_domain *domain,* size is also known at domain creation time. */ info->ops->msi_prepare = its_pmsi_prepare; + info->ops->msi_teardown = its_msi_teardown;break ;default :/* Confused. How did the lib return true? */
drivers/irqchip/irq-gic-v3-its.c +25 -21
66 lines not on this page · raw message
kernel/irq/msi.c +1 -2
@@ -795,8 +795,7 @@ static int msi_domain_ops_prepare(struct irq_domain *domain, struct device *dev,return 0 ; } -static void msi_domain_ops_teardown (struct irq_domain *domain, -msi_alloc_info_t *arg) +static void msi_domain_ops_teardown (struct irq_domain *domain,msi_alloc_info_t *arg) { }
Marc Zyngier
maintainerPATCH v2 3/5 genirq/msi: Move prepare() call to per-device allocation
In reply to Marc ZyngierThe current device MSI infrastructure is subtly broken, as it will issue an .msi_prepare() callback into the MSI controller driver every time it needs to allocate an MSI. That's pretty wrong, as the contract (or unwarranted assumption, depending who you ask) between the MSI controller and the core code is that .msi_prepare() is called exactly once per device.
This leads to some subtle breakage in said MSI controller drivers, as it gives the impression that there are multiple endpoints sharing a bus identifier (RID in PCI parlance, DID for GICv3+). It implies that whatever allocation the ITS driver (for example) has done on behalf of these devices cannot be undone, as there is no way to track the shared state. This is particularly bad for wire-MSI devices, for which .msi_prepare() is called for. each. input. line.
To address this issue, move the call to .msi_prepare() to take place at the point of irq domain allocation, which is the only place that makes sense. The msi_alloc_info_t structure is made part of the msi_domain_template, so that its life-cycle is that of the domain as well.
Finally, the msi_info::alloc_data field is made to point at this allocation tracking structure, ensuring that it is carried around the block.
This is all pretty straightforward, except for the non-device-MSI leftovers, which still have to call .msi_prepare() at the old spot. One day...
--- include/linux/msi.h | 2 ++ kernel/irq/msi.c | 35 +++++++++++++++++++++++++++++++---- 2 files changed, 33 insertions(+), 4 deletions(-)
include/linux/msi.h +2 -0
@@ -516,12 +516,14 @@ struct msi_domain_info {* @chip: Interrupt chip for this domain * @ops: MSI domain ops * @info: MSI domain info data +* @alloc_info: MSI domain allocation data (arch specific) */ struct msi_domain_template {char name[48 ];struct irq_chip chip;struct msi_domain_ops ops;struct msi_domain_info info; +msi_alloc_info_t alloc_info; };/*
kernel/irq/msi.c +31 -4
@@ -59,7 +59,8 @@ struct msi_ctrl {static void msi_domain_free_locked (struct device *dev,struct msi_ctrl *ctrl);static unsigned int msi_domain_get_hwsize (struct device *dev,unsigned int domid);static inline int msi_sysfs_create_group (struct device *dev); - +static int msi_domain_prepare_irqs (struct irq_domain *domain,struct device *dev, +int nvec,msi_alloc_info_t *arg);/** * msi_alloc_desc - Allocate an initialized msi_desc
@@ -1023,6 +1024,7 @@ bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, bundle->info.ops = &bundle->ops; bundle->info.data = domain_data; bundle->info.chip_data = chip_data; + bundle->info.alloc_data = &bundle->alloc_info; pops = parent->msi_parent_ops; snprintf(bundle->name, sizeof(bundle->name), "%s%s-%s",
@@ -1061,11 +1063,18 @@ bool msi_create_device_irq_domain(struct device *dev, unsigned int domid,if (!domain)return false; + domain->dev = dev; + dev->msi.data->__domains[domid].domain = domain; + +if (msi_domain_prepare_irqs (domain, dev, hwsize, &bundle->alloc_info)) { + dev->msi.data->__domains[domid].domain =NULL ; +irq_domain_remove (domain); +return false; + } +/* @bundle and @fwnode_alloced are now in use. Prevent cleanup */ retain_and_null_ptr (bundle);retain_and_null_ptr (fwnode_alloced); - domain->dev = dev; - dev->msi.data->__domains[domid].domain = domain;return true; }
@@ -1232,6 +1241,24 @@ static int msi_init_virq(struct irq_domain *domain, int virq, unsigned int vflagreturn 0 ; } +static int populate_alloc_info (struct irq_domain *domain,struct device *dev, +unsigned int nirqs,msi_alloc_info_t *arg) +{ +struct msi_domain_info *info = domain->host_data; + +/* +* If the caller has provided a template alloc info, use that. Once +* all users of msi_create_irq_domain() have been eliminated, this +* should be the only source of allocation information, and the +* prepare call below should be finally removed. +*/ +if (!info->alloc_data) +return msi_domain_prepare_irqs (domain, dev, nirqs, arg); + + *arg = *info->alloc_data; +return 0 ; +} +static int __msi_domain_alloc_irqs (struct device *dev,struct irq_domain *domain,struct msi_ctrl *ctrl) {
@@ -1244,7 +1271,7 @@ static int __msi_domain_alloc_irqs(struct device *dev, struct irq_domain *domainunsigned long idx;int i, ret, virq; - ret =msi_domain_prepare_irqs (domain, dev, ctrl->nirqs, &arg); + ret =populate_alloc_info (domain, dev, ctrl->nirqs, &arg);if (ret)return ret;
Signature · 1 line
2.39.2
[tip: irq/msi] genirq/msi: Move prepare() call to per-device allocation
The following commit has been merged into the irq/msi branch of tip:
The following commit has been merged into the irq/msi branch of tip:
Commit-ID: 1396e89e09f00deb816e5f4a176d71d554922292 Gitweb: https://git.kernel.org/tip/1396e89e09f00deb816e5f4a176d71d554922292 Author: Marc Zyngier <[email protected]> AuthorDate: Tue, 13 May 2025 17:31:42 +01:00 Committer: Thomas Gleixner <[email protected]> CommitterDate: Wed, 14 May 2025 12:36:41 +02:00
genirq/msi: Move prepare() call to per-device allocation
The current device MSI infrastructure is subtly broken, as it will issue an .msi_prepare() callback into the MSI controller driver every time it needs to allocate an MSI. That's pretty wrong, as the contract (or unwarranted assumption, depending who you ask) between the MSI controller and the core code is that .msi_prepare() is called exactly once per device.
This leads to some subtle breakage in some MSI controller drivers, as it gives the impression that there are multiple endpoints sharing a bus identifier (RID in PCI parlance, DID for GICv3+). It implies that whatever allocation the ITS driver (for example) has done on behalf of these devices cannot be undone, as there is no way to track the shared state. This is particularly bad for wire-MSI devices, for which .msi_prepare() is called for each input line.
To address this issue, move the call to .msi_prepare() to take place at the point of irq domain allocation, which is the only place that makes sense. The msi_alloc_info_t structure is made part of the msi_domain_template, so that its life-cycle is that of the domain as well.
Finally, the msi_info::alloc_data field is made to point at this allocation tracking structure, ensuring that it is carried around the block.
This is all pretty straightforward, except for the non-device-MSI leftovers, which still have to call .msi_prepare() at the old spot. One day...
--- include/linux/msi.h | 2 ++ kernel/irq/msi.c | 35 +++++++++++++++++++++++++++++++---- 2 files changed, 33 insertions(+), 4 deletions(-)
include/linux/msi.h +2 -0
@@ -516,12 +516,14 @@ struct msi_domain_info {* @chip: Interrupt chip for this domain * @ops: MSI domain ops * @info: MSI domain info data +* @alloc_info: MSI domain allocation data (architecture specific) */ struct msi_domain_template {char name[48 ];struct irq_chip chip;struct msi_domain_ops ops;struct msi_domain_info info; +msi_alloc_info_t alloc_info; };/*
kernel/irq/msi.c +31 -4
@@ -59,7 +59,8 @@ struct msi_ctrl {static void msi_domain_free_locked (struct device *dev,struct msi_ctrl *ctrl);static unsigned int msi_domain_get_hwsize (struct device *dev,unsigned int domid);static inline int msi_sysfs_create_group (struct device *dev); - +static int msi_domain_prepare_irqs (struct irq_domain *domain,struct device *dev, +int nvec,msi_alloc_info_t *arg);/** * msi_alloc_desc - Allocate an initialized msi_desc
@@ -1023,6 +1024,7 @@ bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, bundle->info.ops = &bundle->ops; bundle->info.data = domain_data; bundle->info.chip_data = chip_data; + bundle->info.alloc_data = &bundle->alloc_info; pops = parent->msi_parent_ops; snprintf(bundle->name, sizeof(bundle->name), "%s%s-%s",
@@ -1061,11 +1063,18 @@ bool msi_create_device_irq_domain(struct device *dev, unsigned int domid,if (!domain)return false; + domain->dev = dev; + dev->msi.data->__domains[domid].domain = domain; + +if (msi_domain_prepare_irqs (domain, dev, hwsize, &bundle->alloc_info)) { + dev->msi.data->__domains[domid].domain =NULL ; +irq_domain_remove (domain); +return false; + } +/* @bundle and @fwnode_alloced are now in use. Prevent cleanup */ retain_and_null_ptr (bundle);retain_and_null_ptr (fwnode_alloced); - domain->dev = dev; - dev->msi.data->__domains[domid].domain = domain;return true; }
@@ -1232,6 +1241,24 @@ static int msi_init_virq(struct irq_domain *domain, int virq, unsigned int vflagreturn 0 ; } +static int populate_alloc_info (struct irq_domain *domain,struct device *dev, +unsigned int nirqs,msi_alloc_info_t *arg) +{ +struct msi_domain_info *info = domain->host_data; + +/* +* If the caller has provided a template alloc info, use that. Once +* all users of msi_create_irq_domain() have been eliminated, this +* should be the only source of allocation information, and the +* prepare call below should be finally removed. +*/ +if (!info->alloc_data) +return msi_domain_prepare_irqs (domain, dev, nirqs, arg); + + *arg = *info->alloc_data; +return 0 ; +} +static int __msi_domain_alloc_irqs (struct device *dev,struct irq_domain *domain,struct msi_ctrl *ctrl) {
@@ -1244,7 +1271,7 @@ static int __msi_domain_alloc_irqs(struct device *dev, struct irq_domain *domainunsigned long idx;int i, ret, virq; - ret =msi_domain_prepare_irqs (domain, dev, ctrl->nirqs, &arg); + ret =populate_alloc_info (domain, dev, ctrl->nirqs, &arg);if (ret)return ret;
Re: [PATCH v2 3/5] genirq/msi: Move prepare() call to per-device allocation
In reply to Marc ZyngierHi Marc,
Quoted 77 lines · Marc Zyngier
The current device MSI infrastructure is subtly broken, as it will issue an .msi_prepare() callback into the MSI controller driver every time it needs to allocate an MSI. That's pretty wrong, as the contract (or unwarranted assumption, depending who you ask) between the MSI controller and the core code is that .msi_prepare() is called exactly once per device.
This leads to some subtle breakage in said MSI controller drivers, as it gives the impression that there are multiple endpoints sharing a bus identifier (RID in PCI parlance, DID for GICv3+). It implies that whatever allocation the ITS driver (for example) has done on behalf of these devices cannot be undone, as there is no way to track the shared state. This is particularly bad for wire-MSI devices, for which .msi_prepare() is called for. each. input. line.
To address this issue, move the call to .msi_prepare() to take place at the point of irq domain allocation, which is the only place that makes sense. The
msi_alloc_info_tstructure is made part of themsi_domain_template, so that its life-cycle is that of the domain as well.Finally, the
msi_info::alloc_datafield is made to point at this allocation tracking structure, ensuring that it is carried around the block.This is all pretty straightforward, except for the non-device-MSI leftovers, which still have to call .msi_prepare() at the old spot. One day...
Signed-off-by Marc Zyngier <[email protected]>--- include/linux/msi.h | 2 ++ kernel/irq/msi.c | 35 +++++++++++++++++++++++++++++++---- 2 files changed, 33 insertions(+), 4 deletions(-)include/linux/msi.h +2
-0@@ -516,12 +516,14 @@ struct msi_domain_info {* @chip: Interrupt chip for this domain * @ops: MSI domain ops * @info: MSI domain info data +* @alloc_info: MSI domain allocation data (arch specific) */ struct msi_domain_template {char name[48 ];struct irq_chip chip;struct msi_domain_ops ops;struct msi_domain_info info; +msi_alloc_info_t alloc_info; };/* kernel/irq/msi.c +3
-1@@ -59,7 +59,8 @@ struct msi_ctrl {static void msi_domain_free_locked (struct device *dev,struct msi_ctrl *ctrl);static unsigned int msi_domain_get_hwsize (struct device *dev,unsigned int domid);static inline int msi_sysfs_create_group (struct device *dev); - +static int msi_domain_prepare_irqs (struct irq_domain *domain,struct device *dev, +int nvec,msi_alloc_info_t *arg);/** * msi_alloc_desc - Allocate an initialized msi_desc @@ -1023,6 +1024,7 @@ bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, bundle->info.ops = &bundle->ops; bundle->info.data = domain_data; bundle->info.chip_data = chip_data; + bundle->info.alloc_data = &bundle->alloc_info; pops = parent->msi_parent_ops; snprintf(bundle->name, sizeof(bundle->name), "%s%s-%s",
kernel/irq/msi.c bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, · line 1069
Quoted 7 lines
kernel/irq/msi.c +4
-0@@ -1061,11 +1063,18 @@ bool msi_create_device_irq_domain(struct device *dev, unsigned int domid,if (!domain)return false; + domain->dev = dev; + dev->msi.data->__domains[domid].domain = domain; + +if (msi_domain_prepare_irqs (domain, dev, hwsize, &bundle->alloc_info)) {
Does it work for MSI? hwsize is 1 in the MSI case, without taking pci_msi_vec_count() into account.
bool pci_setup_msi_device_domain(struct pci_dev *pdev)
{
[...]
return pci_create_device_domain(pdev, &pci_msi_template, 1);Thanks,
Zenghui
Re: [PATCH v2 3/5] genirq/msi: Move prepare() call to per-device allocation
Quoted 83 lines · Zenghui Yu
Hi Marc,
Quoted 80 lines · Marc Zyngier
The current device MSI infrastructure is subtly broken, as it will issue an .msi_prepare() callback into the MSI controller driver every time it needs to allocate an MSI. That's pretty wrong, as the contract (or unwarranted assumption, depending who you ask) between the MSI controller and the core code is that .msi_prepare() is called exactly once per device.
This leads to some subtle breakage in said MSI controller drivers, as it gives the impression that there are multiple endpoints sharing a bus identifier (RID in PCI parlance, DID for GICv3+). It implies that whatever allocation the ITS driver (for example) has done on behalf of these devices cannot be undone, as there is no way to track the shared state. This is particularly bad for wire-MSI devices, for which .msi_prepare() is called for. each. input. line.
To address this issue, move the call to .msi_prepare() to take place at the point of irq domain allocation, which is the only place that makes sense. The
msi_alloc_info_tstructure is made part of themsi_domain_template, so that its life-cycle is that of the domain as well.Finally, the
msi_info::alloc_datafield is made to point at this allocation tracking structure, ensuring that it is carried around the block.This is all pretty straightforward, except for the non-device-MSI leftovers, which still have to call .msi_prepare() at the old spot. One day...
Signed-off-by Marc Zyngier <[email protected]>--- include/linux/msi.h | 2 ++ kernel/irq/msi.c | 35 +++++++++++++++++++++++++++++++---- 2 files changed, 33 insertions(+), 4 deletions(-)include/linux/msi.h +2
-0@@ -516,12 +516,14 @@ struct msi_domain_info {* @chip: Interrupt chip for this domain * @ops: MSI domain ops * @info: MSI domain info data +* @alloc_info: MSI domain allocation data (arch specific) */ struct msi_domain_template {char name[48 ];struct irq_chip chip;struct msi_domain_ops ops;struct msi_domain_info info; +msi_alloc_info_t alloc_info; };/* kernel/irq/msi.c +3
-1@@ -59,7 +59,8 @@ struct msi_ctrl {static void msi_domain_free_locked (struct device *dev,struct msi_ctrl *ctrl);static unsigned int msi_domain_get_hwsize (struct device *dev,unsigned int domid);static inline int msi_sysfs_create_group (struct device *dev); - +static int msi_domain_prepare_irqs (struct irq_domain *domain,struct device *dev, +int nvec,msi_alloc_info_t *arg);/** * msi_alloc_desc - Allocate an initialized msi_desc @@ -1023,6 +1024,7 @@ bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, bundle->info.ops = &bundle->ops; bundle->info.data = domain_data; bundle->info.chip_data = chip_data; + bundle->info.alloc_data = &bundle->alloc_info; pops = parent->msi_parent_ops; snprintf(bundle->name, sizeof(bundle->name), "%s%s-%s",@@ -1061,11 +1063,18 @@ bool msi_create_device_irq_domain(struct device *dev, unsigned int domid,if (!domain)return false;
Quoted 6 lines
kernel/irq/msi.c bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, · line 1069
Quoted 4 lines
kernel/irq/msi.c +4
-0+ domain->dev = dev; + dev->msi.data->__domains[domid].domain = domain; + +if (msi_domain_prepare_irqs (domain, dev, hwsize, &bundle->alloc_info)) {Does it work for MSI?
This means that it does not work for MSI for you as it stands, right ?
If you spotted an issue, thanks for that, report it fully please.
Quoted 7 lines
hwsize is 1 in the MSI case, without taking
pci_msi_vec_count()into account.bool
pci_setup_msi_device_domain(struct pci_dev *pdev){ [...] return pci_create_device_domain(pdev, &pci_msi_template, 1);
I had a stab at it with GICv5 models and an MSI capable device and this indeed calls the ITS msi_prepare() callback with 1 as vector count, so we size the device tables wrongly.
The question is why pci_create_device_domain() is called here with hwsize == 1. Probably, before this series, the ITS MSI parent code was fixing the size up so we did not notice, I need to check.
Lorenzo
Marc Zyngier
maintainerRe: [PATCH v2 3/5] genirq/msi: Move prepare() call to per-device allocation
Quoted 9 lines · Lorenzo Pieralisi
Quoted 6 lines · Zenghui Yu
kernel/irq/msi.c bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, · line 1069
Quoted 4 lines
kernel/irq/msi.c +4
-0+ domain->dev = dev; + dev->msi.data->__domains[domid].domain = domain; + +if (msi_domain_prepare_irqs (domain, dev, hwsize, &bundle->alloc_info)) {Does it work for MSI?
Quoted 3 lines
This means that it does not work for MSI for you as it stands, right ?
If you spotted an issue, thanks for that, report it fully please.
Honestly, you're barking up the wrong tree. Zenghui points us to a glaring bug in the core code, with detailed information on what could go wrong, as well as what is wrong in the code. It doesn't get better than that.
The usual level of bug reports is "its b0rken", sometimes followed by a trace with lots of hex and no information. Spot the difference?
Quoted 7 lines
Quoted 7 lines
hwsize is 1 in the MSI case, without taking
pci_msi_vec_count()into account.bool
pci_setup_msi_device_domain(struct pci_dev *pdev){ [...]
Quoted 5 lines
Quoted 1 line
return pci_create_device_domain(pdev, &pci_msi_template, 1);I had a stab at it with GICv5 models and an MSI capable device and this indeed calls the ITS
msi_prepare()callback with 1 as vector count, so we size the device tables wrongly.
Not wrongly. Exactly as instructed.
Quoted 4 lines
The question is why
pci_create_device_domain()is called here with hwsize == 1. Probably, before this series, the ITS MSI parent code was fixing the size up so we did not notice, I need to check.
The GICv3 ITS code would upgrade the vector count to the next power of two (one bit of EID space -> 2 MSIs), but with the device domain squarely set to 1, the endpoint driver would never get more. It is prepared to fail gracefully though, hence nothing really breaks.
I don't think this patch makes anything regress though. Commit 15c72f824b327 seems to be the offending one. If Zenghui confirms that the hack I posted separately works for him, I'll follow up with a "real" patch.
M.
Signature · 1 line
Jazz isn't dead. It just smells funny.
Re: [PATCH v2 3/5] genirq/msi: Move prepare() call to per-device allocation
Quoted 15 lines · Marc Zyngier
Quoted 9 lines · Lorenzo Pieralisi
Quoted 6 lines · Zenghui Yu
kernel/irq/msi.c bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, · line 1069
Quoted 4 lines
kernel/irq/msi.c +4
-0+ domain->dev = dev; + dev->msi.data->__domains[domid].domain = domain; + +if (msi_domain_prepare_irqs (domain, dev, hwsize, &bundle->alloc_info)) {Does it work for MSI?
Quoted 3 lines
This means that it does not work for MSI for you as it stands, right ?
If you spotted an issue, thanks for that, report it fully please.
Quoted 7 lines
Honestly, you're barking up the wrong tree. Zenghui points us to a glaring bug in the core code, with detailed information on what could go wrong, as well as what is wrong in the code. It doesn't get better than that.
The usual level of bug reports is "its b0rken", sometimes followed by a trace with lots of hex and no information. Spot the difference?
Agreed, thanks again Zenghui for reporting it and forgive me if the message sounded a bit patronizing, I did not mean it.
Lorenzo
Quoted 33 lines
Quoted 11 lines
Quoted 7 lines
hwsize is 1 in the MSI case, without taking
pci_msi_vec_count()into account.bool
pci_setup_msi_device_domain(struct pci_dev *pdev){ [...] return pci_create_device_domain(pdev, &pci_msi_template, 1);I had a stab at it with GICv5 models and an MSI capable device and this indeed calls the ITS
msi_prepare()callback with 1 as vector count, so we size the device tables wrongly.Not wrongly. Exactly as instructed.
Quoted 4 lines
The question is why
pci_create_device_domain()is called here with hwsize == 1. Probably, before this series, the ITS MSI parent code was fixing the size up so we did not notice, I need to check.The GICv3 ITS code would upgrade the vector count to the next power of two (one bit of EID space -> 2 MSIs), but with the device domain squarely set to 1, the endpoint driver would never get more. It is prepared to fail gracefully though, hence nothing really breaks.
I don't think this patch makes anything regress though. Commit
15c72f824b327seems to be the offending one. If Zenghui confirms that the hack I posted separately works for him, I'll follow up with a "real" patch.M.Signature · 1 line
Jazz isn't dead. It just smells funny.
Re: [PATCH v2 3/5] genirq/msi: Move prepare() call to per-device allocation
Quoted 21 lines · Lorenzo Pieralisi
Quoted 20 lines · Marc Zyngier
Quoted 9 lines · Lorenzo Pieralisi
Quoted 6 lines · Zenghui Yu
kernel/irq/msi.c bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, · line 1069
Quoted 4 lines
kernel/irq/msi.c +4
-0+ domain->dev = dev; + dev->msi.data->__domains[domid].domain = domain; + +if (msi_domain_prepare_irqs (domain, dev, hwsize, &bundle->alloc_info)) {Does it work for MSI?
Quoted 3 lines
This means that it does not work for MSI for you as it stands, right ?
If you spotted an issue, thanks for that, report it fully please.
Honestly, you're barking up the wrong tree. Zenghui points us to a glaring bug in the core code, with detailed information on what could go wrong, as well as what is wrong in the code. It doesn't get better than that.
Quoted 5 lines
Quoted 2 lines
The usual level of bug reports is "its b0rken", sometimes followed by a trace with lots of hex and no information. Spot the difference?
Agreed, thanks again Zenghui for reporting it and forgive me if the message sounded a bit patronizing, I did not mean it.
Never mind :-)
Zenghui
Marc Zyngier
maintainerRe: [PATCH v2 3/5] genirq/msi: Move prepare() call to per-device allocation
In reply to Zenghui YuHi Zenghui,
Quoted 9 lines · Zenghui Yu
Review
Quoted 5 lines
(quoted diff) +4
-0+ domain->dev = dev; + dev->msi.data->__domains[domid].domain = domain; + + if (msi_domain_prepare_irqs(domain, dev, hwsize, &bundle->alloc_info)) {Does it work for MSI? hwsize is 1 in the MSI case, without taking
pci_msi_vec_count()into account.
Quoted 5 lines
bool
pci_setup_msi_device_domain(struct pci_dev *pdev){ [...] return pci_create_device_domain(pdev, &pci_msi_template, 1);
Well spotted.
This looks like a PCI bug ignoring Multi-MSI. Can you give the following a go and let people know whether that fixes your issue?
Thanks,
M.
drivers/pci/msi/irqdomain.c +2 -2
@@ -287,7 +287,7 @@ static bool pci_create_device_domain(struct pci_dev *pdev, const struct msi_doma* - The device is removed * - MSI is disabled and a MSI-X domain is created */ -bool pci_setup_msi_device_domain (struct pci_dev *pdev) +bool pci_setup_msi_device_domain (struct pci_dev *pdev,unsigned int hwsize) {if (WARN_ON_ONCE (pdev->msix_enabled))return false;
@@ -297,7 +297,7 @@ bool pci_setup_msi_device_domain(struct pci_dev *pdev)if (pci_match_device_domain (pdev,DOMAIN_BUS_PCI_DEVICE_MSIX ))msi_remove_device_irq_domain (&pdev->dev,MSI_DEFAULT_DOMAIN ); -return pci_create_device_domain (pdev, &pci_msi_template,1 ); +return pci_create_device_domain (pdev, &pci_msi_template, hwsize); }/**
drivers/pci/msi/msi.c +1 -1
@@ -449,7 +449,7 @@ int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,if (rc)return rc; -if (!pci_setup_msi_device_domain (dev)) +if (!pci_setup_msi_device_domain (dev, nvec))return -ENODEV ;for (;;) {
drivers/pci/msi/msi.h +1 -1
@@ -107,7 +107,7 @@ enum support_mode { };bool pci_msi_domain_supports (struct pci_dev *dev,unsigned int feature_mask,enum support_mode mode); -bool pci_setup_msi_device_domain (struct pci_dev *pdev); +bool pci_setup_msi_device_domain (struct pci_dev *pdev,unsigned int hwsize);bool pci_setup_msix_device_domain (struct pci_dev *pdev,unsigned int hwsize);/* Legacy (!IRQDOMAIN) fallbacks */
Signature · 1 line
Jazz isn't dead. It just smells funny.
Zenghui Yu
Tested-byRe: [PATCH v2 3/5] genirq/msi: Move prepare() call to per-device allocation
Hi Marc,
Quoted 17 lines · Marc Zyngier
Hi Zenghui,
Quoted 13 lines · Zenghui Yu
Review
Quoted 5 lines
(quoted diff) +4
-0+ domain->dev = dev; + dev->msi.data->__domains[domid].domain = domain; + + if (msi_domain_prepare_irqs(domain, dev, hwsize, &bundle->alloc_info)) {Does it work for MSI? hwsize is 1 in the MSI case, without taking
pci_msi_vec_count()into account.bool
pci_setup_msi_device_domain(struct pci_dev *pdev){ [...]
Quoted 6 lines
Quoted 1 line
return pci_create_device_domain(pdev, &pci_msi_template, 1);Well spotted.
This looks like a PCI bug ignoring Multi-MSI. Can you give the following a go and let people know whether that fixes your issue?
I hit this problem on Kunpeng920 with some HiSilicon SAS (Serial Attached SCSI controller) on it. These controllers are MSI-capable and didn't work after this commit.
# lspci -v -s 74:02.0 74:02.0 Serial Attached SCSI controller: Huawei Technologies Co., Ltd. HiSilicon SAS 3.0 HBA (rev 21) Flags: bus master, fast devsel, latency 0, IRQ 42, NUMA node 0, IOMMU group 27 Memory at a2000000 (32-bit, non-prefetchable) [size=32K] Capabilities: [40] Express Root Complex Integrated Endpoint, MSI 00 Capabilities: [80] MSI: Enable+ Count=32/32 Maskable+ 64bit+ Capabilities: [b0] Power Management version 3 Kernel driver in use: hisi_sas_v3_hw
Quoted 40 lines
drivers/pci/msi/irqdomain.c +2
-2@@ -287,7 +287,7 @@ static bool pci_create_device_domain(struct pci_dev *pdev, const struct msi_doma* - The device is removed * - MSI is disabled and a MSI-X domain is created */ -bool pci_setup_msi_device_domain (struct pci_dev *pdev) +bool pci_setup_msi_device_domain (struct pci_dev *pdev,unsigned int hwsize) {if (WARN_ON_ONCE (pdev->msix_enabled))return false;@@ -297,7 +297,7 @@ bool pci_setup_msi_device_domain(struct pci_dev *pdev)if (pci_match_device_domain (pdev,DOMAIN_BUS_PCI_DEVICE_MSIX ))msi_remove_device_irq_domain (&pdev->dev,MSI_DEFAULT_DOMAIN ); -return pci_create_device_domain (pdev, &pci_msi_template,1 ); +return pci_create_device_domain (pdev, &pci_msi_template, hwsize); }/** drivers/pci/msi/msi.c +1
-1@@ -449,7 +449,7 @@ int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,if (rc)return rc; -if (!pci_setup_msi_device_domain (dev)) +if (!pci_setup_msi_device_domain (dev, nvec))return -ENODEV ;for (;;) {drivers/pci/msi/msi.h +0
-0
drivers/pci/msi/msi.h enum support_mode { · line 113
Quoted 8 lines
drivers/pci/msi/msi.h +1
-1@@ -107,7 +107,7 @@ enum support_mode { };bool pci_msi_domain_supports (struct pci_dev *dev,unsigned int feature_mask,enum support_mode mode); -bool pci_setup_msi_device_domain (struct pci_dev *pdev); +bool pci_setup_msi_device_domain (struct pci_dev *pdev,unsigned int hwsize);bool pci_setup_msix_device_domain (struct pci_dev *pdev,unsigned int hwsize);/* Legacy (!IRQDOMAIN) fallbacks */
I have the exact same diff to get my box to work again ;-)
Thanks for your fix!
Zenghui
Marc Zyngier
maintainerRe: [PATCH v2 3/5] genirq/msi: Move prepare() call to per-device allocation
Quoted 23 lines · Zenghui Yu
Hi Marc,
Quoted 19 lines · Marc Zyngier
Hi Zenghui,
Quoted 14 lines · Zenghui Yu
Review
Quoted 5 lines
(quoted diff) +4
-0+ domain->dev = dev; + dev->msi.data->__domains[domid].domain = domain; + + if (msi_domain_prepare_irqs(domain, dev, hwsize, &bundle->alloc_info)) {Does it work for MSI? hwsize is 1 in the MSI case, without taking
pci_msi_vec_count()into account.bool
pci_setup_msi_device_domain(struct pci_dev *pdev){ [...] return pci_create_device_domain(pdev, &pci_msi_template, 1);
Quoted 8 lines
Quoted 4 lines
Well spotted.
This looks like a PCI bug ignoring Multi-MSI. Can you give the following a go and let people know whether that fixes your issue?
I hit this problem on Kunpeng920 with some HiSilicon SAS (Serial Attached SCSI controller) on it. These controllers are MSI-capable and didn't work after this commit.
That's interesting. It means that the sizing of the irqdomain to 1 wasn't getting in the way, even if that was obviously wrong. The funny thing is that the msi_desc would still be OK, as we only have one for MSI, no matter how many vectors are used.
Quoted 2 lines
I have the exact same diff to get my box to work again ;-)
Ah ;-)
Quoted 4 lines
Tested-by Zenghui Yu <[email protected]>Thanks for your fix!
Thank you!
M.
Signature · 1 line
Jazz isn't dead. It just smells funny.
Re: [PATCH v2 3/5] genirq/msi: Move prepare() call to per-device allocation
In reply to Marc ZyngierQuoted 55 lines · Marc Zyngier
Hi Zenghui,
Quoted 9 lines · Zenghui Yu
Review
Quoted 5 lines
(quoted diff) +4
-0+ domain->dev = dev; + dev->msi.data->__domains[domid].domain = domain; + + if (msi_domain_prepare_irqs(domain, dev, hwsize, &bundle->alloc_info)) {Does it work for MSI? hwsize is 1 in the MSI case, without taking
pci_msi_vec_count()into account.Quoted 5 lines
bool
pci_setup_msi_device_domain(struct pci_dev *pdev){ [...] return pci_create_device_domain(pdev, &pci_msi_template, 1);Well spotted.
This looks like a PCI bug ignoring Multi-MSI. Can you give the following a go and let people know whether that fixes your issue?
Thanks,
M.drivers/pci/msi/irqdomain.c +2
-2@@ -287,7 +287,7 @@ static bool pci_create_device_domain(struct pci_dev *pdev, const struct msi_doma* - The device is removed * - MSI is disabled and a MSI-X domain is created */ -bool pci_setup_msi_device_domain (struct pci_dev *pdev) +bool pci_setup_msi_device_domain (struct pci_dev *pdev,unsigned int hwsize) {if (WARN_ON_ONCE (pdev->msix_enabled))return false;@@ -297,7 +297,7 @@ bool pci_setup_msi_device_domain(struct pci_dev *pdev)if (pci_match_device_domain (pdev,DOMAIN_BUS_PCI_DEVICE_MSIX ))msi_remove_device_irq_domain (&pdev->dev,MSI_DEFAULT_DOMAIN ); -return pci_create_device_domain (pdev, &pci_msi_template,1 ); +return pci_create_device_domain (pdev, &pci_msi_template, hwsize); }/** drivers/pci/msi/msi.c +0
-0
drivers/pci/msi/msi.c int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec, · line 452
Quoted 5 lines
drivers/pci/msi/msi.c +1
-1@@ -449,7 +449,7 @@ int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,if (rc)return rc; -if (!pci_setup_msi_device_domain (dev)) +if (!pci_setup_msi_device_domain (dev, nvec))
If pci_msi_vec_count(dev) > maxvec we would cap nvec and size the domain with the capped value.
In __pci_enable_msix_range() we are sizing the device according to pci_msix_vec_count(dev) regardless of maxvec, if I read the code correctly.
While fixing it it would be good to make them consistent unless there is a reason why they should not.
Lorenzo
Quoted 19 lines
drivers/pci/msi/msi.c +0
-0return -ENODEV ;for (;;) {drivers/pci/msi/msi.h +1
-1@@ -107,7 +107,7 @@ enum support_mode { };bool pci_msi_domain_supports (struct pci_dev *dev,unsigned int feature_mask,enum support_mode mode); -bool pci_setup_msi_device_domain (struct pci_dev *pdev); +bool pci_setup_msi_device_domain (struct pci_dev *pdev,unsigned int hwsize);bool pci_setup_msix_device_domain (struct pci_dev *pdev,unsigned int hwsize);/* Legacy (!IRQDOMAIN) fallbacks */ Signature · 2 lines
-- Jazz isn't dead. It just smells funny.
Marc Zyngier
maintainerRe: [PATCH v2 3/5] genirq/msi: Move prepare() call to per-device allocation
Quoted 66 lines · Lorenzo Pieralisi
Quoted 55 lines · Marc Zyngier
Hi Zenghui,
Quoted 9 lines · Zenghui Yu
Review
Quoted 5 lines
(quoted diff) +4
-0+ domain->dev = dev; + dev->msi.data->__domains[domid].domain = domain; + + if (msi_domain_prepare_irqs(domain, dev, hwsize, &bundle->alloc_info)) {Does it work for MSI? hwsize is 1 in the MSI case, without taking
pci_msi_vec_count()into account.Quoted 5 lines
bool
pci_setup_msi_device_domain(struct pci_dev *pdev){ [...] return pci_create_device_domain(pdev, &pci_msi_template, 1);Well spotted.
This looks like a PCI bug ignoring Multi-MSI. Can you give the following a go and let people know whether that fixes your issue?
Thanks,
M.drivers/pci/msi/irqdomain.c +2
-2@@ -287,7 +287,7 @@ static bool pci_create_device_domain(struct pci_dev *pdev, const struct msi_doma* - The device is removed * - MSI is disabled and a MSI-X domain is created */ -bool pci_setup_msi_device_domain (struct pci_dev *pdev) +bool pci_setup_msi_device_domain (struct pci_dev *pdev,unsigned int hwsize) {if (WARN_ON_ONCE (pdev->msix_enabled))return false;@@ -297,7 +297,7 @@ bool pci_setup_msi_device_domain(struct pci_dev *pdev)if (pci_match_device_domain (pdev,DOMAIN_BUS_PCI_DEVICE_MSIX ))msi_remove_device_irq_domain (&pdev->dev,MSI_DEFAULT_DOMAIN ); -return pci_create_device_domain (pdev, &pci_msi_template,1 ); +return pci_create_device_domain (pdev, &pci_msi_template, hwsize); }/** drivers/pci/msi/msi.c +0
-0drivers/pci/msi/msi.c int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec, · line 452
Quoted 5 lines
drivers/pci/msi/msi.c +1
-1@@ -449,7 +449,7 @@ int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,if (rc)return rc; -if (!pci_setup_msi_device_domain (dev)) +if (!pci_setup_msi_device_domain (dev, nvec))If
pci_msi_vec_count(dev)> maxvec we would cap nvec and size the domain with the capped value.
Quoted 5 lines
In
__pci_enable_msix_range()we are sizing the device according topci_msix_vec_count(dev)regardless of maxvec, if I read the code correctly.While fixing it it would be good to make them consistent unless there is a reason why they should not.
This is indeed odd, but that'd be a separate fix. Something like:
drivers/pci/msi/msi.c +3 -3
@@ -439,9 +439,6 @@ int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,if (nvec < minvec)return -ENOSPC ; -if (nvec > maxvec) - nvec = maxvec; - rc =pci_setup_msi_context (dev);if (rc)return rc;
@@ -449,6 +446,9 @@ int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,if (!pci_setup_msi_device_domain (dev, nvec))return -ENODEV ; +if (nvec > maxvec) + nvec = maxvec; +for (;;) {if (affd) { nvec =irq_calc_affinity_vectors (minvec, nvec, affd);
Signature · 1 line
Jazz isn't dead. It just smells funny.
Marc Zyngier
maintainerRe: [PATCH v2 3/5] genirq/msi: Move prepare() call to per-device allocation
Quoted 33 lines · Marc Zyngier
Quoted 11 lines · Lorenzo Pieralisi
Review
Quoted 2 lines
(quoted diff) +1
-0+ if (!pci_setup_msi_device_domain(dev, nvec))If
pci_msi_vec_count(dev)> maxvec we would cap nvec and size the domain with the capped value.In
__pci_enable_msix_range()we are sizing the device according topci_msix_vec_count(dev)regardless of maxvec, if I read the code correctly.While fixing it it would be good to make them consistent unless there is a reason why they should not.
This is indeed odd, but that'd be a separate fix. Something like:
drivers/pci/msi/msi.c +0
-3@@ -439,9 +439,6 @@ int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,if (nvec < minvec)return -ENOSPC ; -if (nvec > maxvec) - nvec = maxvec; - rc =pci_setup_msi_context (dev);if (rc)return rc;
drivers/pci/msi/msi.c int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec, · line 454
Quoted 9 lines
drivers/pci/msi/msi.c +3
-0@@ -449,6 +446,9 @@ int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,if (!pci_setup_msi_device_domain (dev, nvec))return -ENODEV ; +if (nvec > maxvec) + nvec = maxvec; +for (;;) {if (affd) { nvec =irq_calc_affinity_vectors (minvec, nvec, affd);
Actually, let's lump the two together. There is no reason to wait.
M.
Signature · 1 line
Jazz isn't dead. It just smells funny.
Marc Zyngier
maintainerPATCH v2 4/5 genirq/msi: Engage the .msi_teardown() callback on domain removal
In reply to Marc ZyngierKindly inform the MSI driver that we are tearing down the domain, providing the allocation context previously populated on domain creation.
--- kernel/irq/msi.c | 3 +++ 1 file changed, 3 insertions(+)
kernel/irq/msi.c +3 -0
@@ -1096,6 +1096,9 @@ void msi_remove_device_irq_domain(struct device *dev, unsigned int domid) dev->msi.data->__domains[domid].domain =NULL ; info = domain->host_data; + + info->ops->msi_teardown (domain, info->alloc_data); +if (irq_domain_is_msi_device (domain)) fwnode = domain->fwnode;irq_domain_remove (domain);
Signature · 1 line
2.39.2
[tip: irq/msi] genirq/msi: Engage the .msi_teardown() callback on domain removal
The following commit has been merged into the irq/msi branch of tip:
The following commit has been merged into the irq/msi branch of tip:
Commit-ID: 03c298760ed97c5981402d64c4eed9bc4f2f0a4e Gitweb: https://git.kernel.org/tip/03c298760ed97c5981402d64c4eed9bc4f2f0a4e Author: Marc Zyngier <[email protected]> AuthorDate: Tue, 13 May 2025 17:31:43 +01:00 Committer: Thomas Gleixner <[email protected]> CommitterDate: Wed, 14 May 2025 12:36:42 +02:00
genirq/msi: Engage the .msi_teardown() callback on domain removal
Kindly inform the MSI driver that the domain is torn down, providing the allocation context previously populated on domain creation.
--- kernel/irq/msi.c | 3 +++ 1 file changed, 3 insertions(+)
kernel/irq/msi.c +3 -0
@@ -1096,6 +1096,9 @@ void msi_remove_device_irq_domain(struct device *dev, unsigned int domid) dev->msi.data->__domains[domid].domain =NULL ; info = domain->host_data; + + info->ops->msi_teardown (domain, info->alloc_data); +if (irq_domain_is_msi_device (domain)) fwnode = domain->fwnode;irq_domain_remove (domain);
Marc Zyngier
maintainerPATCH v2 5/5 irqchip/gic-v3-its: Use allocation size from the prepare call
In reply to Marc ZyngierNow that .msi_prepare() gets called at the right time and not with semi-random parameters, remove the ugly hack that tried to fix up the number of allocated vectors.
It is now correct by construction.
--- drivers/irqchip/irq-gic-v3-its-msi-parent.c | 19 ------------------- 1 file changed, 19 deletions(-)
drivers/irqchip/irq-gic-v3-its-msi-parent.c +0 -19
@@ -67,17 +67,6 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,/* ITS specific DeviceID, as the core ITS ignores dev. */ info->scratchpad[0 ].ul =pci_msi_domain_get_msi_rid (domain->parent, pdev); -/* -* @domain->msi_domain_info->hwsize contains the size of the -* MSI[-X] domain, but vector allocation happens one by one. This -* needs some thought when MSI comes into play as the size of MSI -* might be unknown at domain creation time and therefore set to -* MSI_MAX_INDEX. -*/ - msi_info =msi_get_domain_info (domain); -if (msi_info->hwsize > nvec) - nvec = msi_info->hwsize; -/* * Always allocate a power of 2, and special case device 0 for * broken systems where the DevID is not wired (and all devices
@@ -151,14 +140,6 @@ static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev,/* ITS specific DeviceID, as the core ITS ignores dev. */ info->scratchpad[0 ].ul = dev_id; -/* -* @domain->msi_domain_info->hwsize contains the size of the device -* domain, but vector allocation happens one by one. -*/ - msi_info =msi_get_domain_info (domain); -if (msi_info->hwsize > nvec) - nvec = msi_info->hwsize; -/* Allocate at least 32 MSIs, and always as a power of 2 */ nvec =max_t (int,32 ,roundup_pow_of_two (nvec));
Signature · 1 line
2.39.2
[tip: irq/msi] irqchip/gic-v3-its: Use allocation size from the prepare call
The following commit has been merged into the irq/msi branch of tip:
The following commit has been merged into the irq/msi branch of tip:
Commit-ID: 7dd20bf2f010430b75b109e4d4101cd1616afba3 Gitweb: https://git.kernel.org/tip/7dd20bf2f010430b75b109e4d4101cd1616afba3 Author: Marc Zyngier <[email protected]> AuthorDate: Tue, 13 May 2025 17:31:44 +01:00 Committer: Thomas Gleixner <[email protected]> CommitterDate: Wed, 14 May 2025 12:36:42 +02:00
irqchip/gic-v3-its: Use allocation size from the prepare call
Now that .msi_prepare() gets called at the right time and not with semi-random parameters, remove the ugly hack that tried to fix up the number of allocated vectors.
It is now correct by construction.
--- drivers/irqchip/irq-gic-v3-its-msi-parent.c | 19 ------------------- 1 file changed, 19 deletions(-)
drivers/irqchip/irq-gic-v3-its-msi-parent.c +0 -19
@@ -68,17 +68,6 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev, info->scratchpad[0 ].ul =pci_msi_domain_get_msi_rid (domain->parent, pdev);/* -* @domain->msi_domain_info->hwsize contains the size of the -* MSI[-X] domain, but vector allocation happens one by one. This -* needs some thought when MSI comes into play as the size of MSI -* might be unknown at domain creation time and therefore set to -* MSI_MAX_INDEX. -*/ - msi_info =msi_get_domain_info (domain); -if (msi_info->hwsize > nvec) - nvec = msi_info->hwsize; - -/* * Always allocate a power of 2, and special case device 0 for * broken systems where the DevID is not wired (and all devices * appear as DevID 0). For that reason, we generously allocate a
@@ -151,14 +140,6 @@ static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev,/* ITS specific DeviceID, as the core ITS ignores dev. */ info->scratchpad[0 ].ul = dev_id; -/* -* @domain->msi_domain_info->hwsize contains the size of the device -* domain, but vector allocation happens one by one. -*/ - msi_info =msi_get_domain_info (domain); -if (msi_info->hwsize > nvec) - nvec = msi_info->hwsize; -/* Allocate at least 32 MSIs, and always as a power of 2 */ nvec =max_t (int,32 ,roundup_pow_of_two (nvec));
Re: [PATCH v2 5/5] irqchip/gic-v3-its: Use allocation size from the prepare call
In reply to Marc ZyngierQuoted 5 lines · Marc Zyngier
Now that .msi_prepare() gets called at the right time and not with semi-random parameters, remove the ugly hack that tried to fix up the number of allocated vectors.
It is now correct by construction.
FWIW, while looking at something related, it occured to me that with this change you can enable MSI_FLAG_PCI_MSIX_ALLOC_DYN now on GIC ITS.
Thanks,
tglx
Marc Zyngier
maintainerRe: [PATCH v2 5/5] irqchip/gic-v3-its: Use allocation size from the prepare call
Quoted 11 lines · Thomas Gleixner
Quoted 5 lines · Marc Zyngier
Now that .msi_prepare() gets called at the right time and not with semi-random parameters, remove the ugly hack that tried to fix up the number of allocated vectors.
It is now correct by construction.
FWIW, while looking at something related, it occured to me that with this change you can enable
MSI_FLAG_PCI_MSIX_ALLOC_DYNnow on GIC ITS.
Maybe. It is rather unclear to me what this "dynamic allocation" actually provides in terms of guarantees to the endpoint driver.
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH v2 5/5] irqchip/gic-v3-its: Use allocation size from the prepare call
Quoted 9 lines · Marc Zyngier
Quoted 7 lines · Thomas Gleixner
Quoted 4 lines · Marc Zyngier
Now that .msi_prepare() gets called at the right time and not with semi-random parameters, remove the ugly hack that tried to fix up the number of allocated vectors.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
It is now correct by construction.
FWIW, while looking at something related, it occured to me that with this change you can enable
MSI_FLAG_PCI_MSIX_ALLOC_DYNnow on GIC ITS.Maybe. It is rather unclear to me what this "dynamic allocation" actually provides in terms of guarantees to the endpoint driver.
It allows the driver to avoid allocating a gazillion of interrupts upfront during initialization. Instead it can allocate them on demand, when e.g. a queue is initialized. Of course that means that such an allocation can fail, but so can request_irq() and other things. I'm not sure what you mean with guarantees here.
Thanks
tglx
Marc Zyngier
maintainerRe: [PATCH v2 5/5] irqchip/gic-v3-its: Use allocation size from the prepare call
Quoted 16 lines · Thomas Gleixner
Quoted 14 lines · Marc Zyngier
Quoted 11 lines · Thomas Gleixner
Quoted 5 lines · Marc Zyngier
Now that .msi_prepare() gets called at the right time and not with semi-random parameters, remove the ugly hack that tried to fix up the number of allocated vectors.
It is now correct by construction.
FWIW, while looking at something related, it occured to me that with this change you can enable
MSI_FLAG_PCI_MSIX_ALLOC_DYNnow on GIC ITS.
Quoted 8 lines
Quoted 2 lines
Maybe. It is rather unclear to me what this "dynamic allocation" actually provides in terms of guarantees to the endpoint driver.
It allows the driver to avoid allocating a gazillion of interrupts upfront during initialization. Instead it can allocate them on demand, when e.g. a queue is initialized. Of course that means that such an allocation can fail, but so can
request_irq()and other things. I'm not sure what you mean with guarantees here.
What is the endpoint driver allowed to expect in terms of continuity of allocation in the IRQ space? If this is solely limited to MSI-X, then the answer probably is "none whatsoever", and the driver should only manage the MSI descriptor index.
Can any other MSI-like mechanism end-up with multiple allocations and require extra alignment/contiguity guarantees in the hwirq space, more or less similar to what MultiMSI requires? Because that'd be much harder to provide.
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH v2 5/5] irqchip/gic-v3-its: Use allocation size from the prepare call
Quoted 16 lines · Marc Zyngier
Quoted 8 lines · Thomas Gleixner
Quoted 2 lines
Maybe. It is rather unclear to me what this "dynamic allocation" actually provides in terms of guarantees to the endpoint driver.
It allows the driver to avoid allocating a gazillion of interrupts upfront during initialization. Instead it can allocate them on demand, when e.g. a queue is initialized. Of course that means that such an allocation can fail, but so can
request_irq()and other things. I'm not sure what you mean with guarantees here.What is the endpoint driver allowed to expect in terms of continuity of allocation in the IRQ space? If this is solely limited to MSI-X, then the answer probably is "none whatsoever", and the driver should only manage the MSI descriptor index.
Quoted 4 lines
Can any other MSI-like mechanism end-up with multiple allocations and require extra alignment/contiguity guarantees in the hwirq space, more or less similar to what MultiMSI requires? Because that'd be much harder to provide.
It's only relevant to MSI-X today. That's the only facility, which actually provides an interface _if_ the underlying parent supports it.
static const struct msi_domain_template pci_msix_template = {
....
.info = {
.flags = MSI_COMMON_FLAGS | MSI_FLAG_PCI_MSIX |
MSI_FLAG_PCI_MSIX_ALLOC_DYN,
.bus_token = DOMAIN_BUS_PCI_DEVICE_MSIX,
},
};That's the device domain template, which requests the functionality and the core then checks whether the parent domain supports it. If so the functionality is enabled.
Thanks,
tglx