genirq/msi: Fix device MSI prepare/alloc sequencing
Subsystems (4)
Marc Zyngier
maintainercover 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 odd bad 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 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 6.15-rc5.
Marc Zyngier (4):
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 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 | 56 +++++++++++++-------- include/linux/msi.h | 12 ++++- kernel/irq/msi.c | 51 +++++++++++++++++-- 4 files changed, 101 insertions(+), 47 deletions(-)
Signature · 1 line
2.39.2
Marc Zyngier
maintainerPATCH 1/4 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, we don't have a callback reversing this setup.
For this purpose, let's a .msi_teardown() callback. This is reliying on the msi_domain_info structure having a non-NULL alloc_data field.
Nobody is populating this field yet, so there is no change in behaviour yet.
--- include/linux/msi.h | 10 ++++++++-- kernel/irq/msi.c | 12 ++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-)
include/linux/msi.h +8 -2
@@ -420,6 +420,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
@@ -435,8 +436,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
@@ -458,6 +460,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,
@@ -486,6 +490,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 {
@@ -498,6 +503,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 +12 -0
@@ -796,6 +796,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) {
@@ -821,6 +826,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, };
@@ -842,6 +848,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; }
@@ -1088,6 +1096,10 @@ void msi_remove_device_irq_domain(struct device *dev, unsigned int domid) dev->msi.data->__domains[domid].domain =NULL ; info = domain->host_data; + +if (info->alloc_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
Re: [PATCH 1/4] genirq/msi: Add .msi_teardown() callback as the reverse of .msi_prepare()
Quoted 3 lines · Marc Zyngier
While the MSI ops do have a .msi_prepare() callback that is responsible for setting up the relevant (usually per-device) allocation, we don't have a callback reversing this setup.
..., there is no callback reversing ...
Quoted 1 line
For this purpose, let's a .msi_teardown() callback. This is
'let's a ...' is not a sentence. Just say: add a .... calback.
Quoted 1 line
reliying on the
msi_domain_infostructure having a non-NULL
^^^^^ spell check is your friend.
Quoted 3 lines
alloc_datafield.Nobody is populating this field yet, so there is no change
No driver is ..
kernel/irq/msi.c static int msi_domain_ops_prepare(struct irq_domain *domain, struct device *dev, · line 800
Quoted 3 lines
kernel/irq/msi.c +2
-0+static void msi_domain_ops_teardown (struct irq_domain *domain, +msi_alloc_info_t *arg)
No line break required.
Quoted 24 lines
kernel/irq/msi.c +6
-0+{ +} +static void msi_domain_ops_set_desc (msi_alloc_info_t *arg,struct msi_desc *desc) {@@ -821,6 +826,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, };@@ -842,6 +848,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; }
kernel/irq/msi.c void msi_remove_device_irq_domain(struct device *dev, unsigned int domid) · line 1101
Quoted 6 lines
kernel/irq/msi.c +3
-0@@ -1088,6 +1096,10 @@ void msi_remove_device_irq_domain(struct device *dev, unsigned int domid) dev->msi.data->__domains[domid].domain =NULL ; info = domain->host_data; + +if (info->alloc_data) + info->ops->msi_teardown (domain, info->alloc_data);
Hmm, that's weird.
Why not call it unconditionally. The empty teardown() default callback does not care about @arg being NULL. No?
Thanks,
tglx
Marc Zyngier
maintainerRe: [PATCH 1/4] genirq/msi: Add .msi_teardown() callback as the reverse of .msi_prepare()
Quoted 9 lines · Thomas Gleixner
Quoted 3 lines · Marc Zyngier
While the MSI ops do have a .msi_prepare() callback that is responsible for setting up the relevant (usually per-device) allocation, we don't have a callback reversing this setup.
..., there is no callback reversing ...
Quoted 7 lines
Quoted 1 line
For this purpose, let's a .msi_teardown() callback. This is
'let's a ...' is not a sentence. Just say: add a .... calback.
Quoted 1 line
reliying on the
msi_domain_infostructure having a non-NULL^^^^^ spell check is your friend.
I rely on humans for that. But maybe I should ask someone to put these recommendations into one of these AI bots, and generate the stuff automatically. It will be devoid of any actual reasoning, but at least it will have the "officially sanctioned" verbiage.
Quoted 6 lines
Quoted 4 lines
alloc_datafield.Nobody is populating this field yet, so there is no change
No driver is ..
No. There is definitely no driver populating this, nor there will ever be. That's 100% MSI infrastructure.
Quoted 6 lines
kernel/irq/msi.c static int msi_domain_ops_prepare(struct irq_domain *domain, struct device *dev, · line 800
Quoted 4 lines
kernel/irq/msi.c +2
-0+static void msi_domain_ops_teardown (struct irq_domain *domain, +msi_alloc_info_t *arg)No line break required.
You mean...
Quoted 6 lines
kernel/irq/msi.c static int msi_domain_ops_prepare(struct irq_domain *domain, struct device *dev, · line 805
Quoted 6 lines
kernel/irq/msi.c +3
-0+{ +} +static void msi_domain_ops_set_desc (msi_alloc_info_t *arg,struct msi_desc *desc)
... not like this?
Quoted 28 lines
Quoted 19 lines
{
kernel/irq/msi.c +3
-0@@ -821,6 +826,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, };@@ -842,6 +848,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; }kernel/irq/msi.c void msi_remove_device_irq_domain(struct device *dev, unsigned int domid) · line 1101
Quoted 6 lines
kernel/irq/msi.c +3
-0@@ -1088,6 +1096,10 @@ void msi_remove_device_irq_domain(struct device *dev, unsigned int domid) dev->msi.data->__domains[domid].domain =NULL ; info = domain->host_data; + +if (info->alloc_data) + info->ops->msi_teardown (domain, info->alloc_data);Hmm, that's weird.
Quoted 2 lines
Why not call it unconditionally. The empty
teardown()default callback does not care about @arg being NULL. No?
Because at this point, nothing populates that pointer. It is only after patch 3 that this pointer is valid. After patch 2, we get a non-default callback, which should never be presented with a NULL allocation context.
And since I value keeping things bisectable, it has to happen in this order.
Thanks,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH 1/4] genirq/msi: Add .msi_teardown() callback as the reverse of .msi_prepare()
Quoted 10 lines · Marc Zyngier
Quoted 4 lines · Thomas Gleixner
kernel/irq/msi.c static int msi_domain_ops_prepare(struct irq_domain *domain, struct device *dev, · line 800
Quoted 2 lines
kernel/irq/msi.c +2
-0+static void msi_domain_ops_teardown (struct irq_domain *domain, +msi_alloc_info_t *arg)No line break required.
You mean...
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
kernel/irq/msi.c static int msi_domain_ops_prepare(struct irq_domain *domain, struct device *dev, · line 805
Quoted 5 lines
kernel/irq/msi.c +3
-0+{ +} +static void msi_domain_ops_set_desc (msi_alloc_info_t *arg,struct msi_desc *desc)... not like this?
Right. That was written before we went to 100 characters line length.
Marc Zyngier
maintainerPATCH 2/4 irqchip/gic-v3-its: Implement .msi_teardown() callback
In reply to Marc ZyngierWe currently nuke 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, let's 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 the following patch.
--- drivers/irqchip/irq-gic-v3-its-msi-parent.c | 10 ++++ drivers/irqchip/irq-gic-v3-its.c | 56 +++++++++++++-------- 2 files changed, 45 insertions(+), 21 deletions(-)
drivers/irqchip/irq-gic-v3-its-msi-parent.c +10 -0
@@ -159,6 +159,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) {
@@ -182,6 +190,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 :
@@ -190,6 +199,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 +35 -21
76 lines not on this page · raw message
Signature · 1 line
2.39.2
Re: [PATCH 2/4] irqchip/gic-v3-its: Implement .msi_teardown() callback
Quoted 1 line · Marc Zyngier
We currently nuke the structure representing an endpoint device
How is we? We means nothing as you know :)
Quoted 7 lines
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.
Quoted 5 lines
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, let's start by implementing the new
s/let's//
I really have to understand why everyone is so fond of "let's". It only makes limited sense when the patch is proposed, but in a change log it does not make sense at all.
Quoted 4 lines
.msi_teardown() callback. Nothing calls it yet, so a side effect is that the
its_devstructure will not be freed and that the DID will stay mapped. Not a big deal, and this will be solved in the following patch.
Now I see why you added this incomprehensible condition into the core code. Bah.
Why don't you add this callback once you changed the prepare muck, which introduces info::alloc_data?
Thanks,
tglx
Marc Zyngier
maintainerRe: [PATCH 2/4] irqchip/gic-v3-its: Implement .msi_teardown() callback
Quoted 5 lines · Thomas Gleixner
Quoted 1 line · Marc Zyngier
We currently nuke the structure representing an endpoint device
How is we? We means nothing as you know :)
We means a lot to *me*.
Quoted 25 lines
Quoted 8 lines
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.
Quoted 5 lines
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, let's start by implementing the new
s/let's//
I really have to understand why everyone is so fond of "let's". It only makes limited sense when the patch is proposed, but in a change log it does not make sense at all.
Quoted 4 lines
.msi_teardown() callback. Nothing calls it yet, so a side effect is that the
its_devstructure will not be freed and that the DID will stay mapped. Not a big deal, and this will be solved in the following patch.
Quoted 5 lines
Now I see why you added this incomprehensible condition into the core code. Bah.
Why don't you add this callback once you changed the prepare muck, which introduces
info::alloc_data?
Changing prepare first breaks nvme and igbxe, amongst other things, for the reasons explained just above, so you need to implement teardown first, plug the MSI controller into it, and only then you can switch prepare.
What I can do is:
(1) introduce teardown without the call site in msi_remove_device_irq_domain()
(2) add its implementation in the ITS driver
(3) move the prepare crap
(4) add the teardown call to msi_remove_device_irq_domain(), without
the check on info->alloc_data
With that order, everything will keep working, with a temporary leak of ITTs in the ITS driver between patches (2) and (4).
Is that something you can live with?
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH 2/4] irqchip/gic-v3-its: Implement .msi_teardown() callback
Quoted 16 lines · Marc Zyngier
On Mon, 12 May 2025 15:34:59 +0100,
Thomas Gleixner <[email protected]> wrote:Changing prepare first breaks nvme and igbxe, amongst other things, for the reasons explained just above, so you need to implement teardown first, plug the MSI controller into it, and only then you can switch prepare.
What I can do is:
(1) introduce teardown without the call site in
msi_remove_device_irq_domain()(2) add its implementation in the ITS driver
(3) move the prepare crap
Quoted 7 lines
(4) add the teardown call to
msi_remove_device_irq_domain(), withoutthe check on info->alloc_dataWith that order, everything will keep working, with a temporary leak of ITTs in the ITS driver between patches (2) and (4).
Is that something you can live with?
Sounds good.
Re: [PATCH 2/4] irqchip/gic-v3-its: Implement .msi_teardown() callback
In reply to Marc ZyngierQuoted 18 lines · Marc Zyngier
We currently nuke 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, let's start by implementing the new .msi_teardown() callback. Nothing calls it yet, so a side effect is that the
its_devstructure will not be freed and that the DID will stay mapped. Not a big deal, and this will be solved in the following patch.
Quoted 5 lines
Signed-off-by Marc Zyngier <[email protected]>--- drivers/irqchip/irq-gic-v3-its-msi-parent.c | 10 ++++ drivers/irqchip/irq-gic-v3-its.c | 56 +++++++++++++-------- 2 files changed, 45 insertions(+), 21 deletions(-)
First off, thanks a lot for putting this together, it makes an awful lot of sense to me.
Quoted 5 lines
index
0115ad6c82593..3472b97477104100644drivers/irqchip/irq-gic-v3-its.c +0
-0@@ -3620,8 +3620,43 @@ static int its_msi_prepare(struct irq_domain *domain, struct device *dev,return err;
drivers/irqchip/irq-gic-v3-its.c static int its_msi_prepare(struct irq_domain *domain, struct device *dev, · line 3630
Quoted 10 lines
drivers/irqchip/irq-gic-v3-its.c +8
-0} +static void its_msi_teardown (struct irq_domain *domain,msi_alloc_info_t *info) +{ +struct msi_domain_info *msi_info; +struct its_device *its_dev; +struct its_node *its; +u32 dev_id; + + dev_id = info->scratchpad[0 ].ul;
I have just managed to get to a keyboard :), I don't think the dev_id makes it to this point, we overwrite it with the its_dev pointer in its_msi_prepare() (could use second scratchpad for the pointer maybe ?).
I was bitten by this while removing the old IWB code into the new one (unrelated to this code but that's how I noticed scratchpad is a union).
Ignore me if I am mistaken, just reading the code, have not tested it (but I am about to do it for v5).
Thanks,
Lorenzo
Quoted 70 lines
drivers/irqchip/irq-gic-v3-its.c +27
-21+ + msi_info =msi_get_domain_info (domain); + its = msi_info->data; + +guard (mutex)(&its->dev_alloc_lock); + + its_dev =its_find_device (its, dev_id); + +/* If the device is shared, keep everything around */ +if (its_dev->shared) +return ; + +/* LPIs should have been already unmapped at this stage */ +if (WARN_ON_ONCE (!bitmap_empty (its_dev->event_map.lpi_map, + its_dev->event_map.nr_lpis))) +return ; + +its_lpi_free (its_dev->event_map.lpi_map, + its_dev->event_map.lpi_base, + its_dev->event_map.nr_lpis); + +/* Unmap device/itt, and get rid of the tracking */ +its_send_mapd (its_dev,0 ); +its_free_device (its_dev); +} +static struct msi_domain_ops its_msi_domain_ops = { .msi_prepare = its_msi_prepare, + .msi_teardown = its_msi_teardown, }; static int its_irq_gic_domain_alloc(struct irq_domain *domain,@@ -3722,7 +3757,6 @@ static void its_irq_domain_free(struct irq_domain *domain, unsigned int virq, {struct irq_data *d =irq_domain_get_irq_data (domain, virq);struct its_device *its_dev =irq_data_get_irq_chip_data (d); -struct its_node *its = its_dev->its;int i; bitmap_release_region(its_dev->event_map.lpi_map,@@ -3736,26 +3770,6 @@ static void its_irq_domain_free(struct irq_domain *domain, unsigned int virq,irq_domain_reset_irq_data (data); } -mutex_lock (&its->dev_alloc_lock); - -/* -* If all interrupts have been freed, start mopping the -* floor. This is conditioned on the device not being shared. -*/ -if (!its_dev->shared && -bitmap_empty (its_dev->event_map.lpi_map, - its_dev->event_map.nr_lpis)) { -its_lpi_free (its_dev->event_map.lpi_map, - its_dev->event_map.lpi_base, - its_dev->event_map.nr_lpis); - -/* Unmap device/itt */ -its_send_mapd (its_dev,0 ); -its_free_device (its_dev); - } - -mutex_unlock (&its->dev_alloc_lock); -irq_domain_free_irqs_parent (domain, virq, nr_irqs); }Signature · 1 line
2.39.2
Marc Zyngier
maintainerRe: [PATCH 2/4] irqchip/gic-v3-its: Implement .msi_teardown() callback
Quoted 49 lines · Lorenzo Pieralisi
Quoted 18 lines · Marc Zyngier
We currently nuke 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, let's start by implementing the new .msi_teardown() callback. Nothing calls it yet, so a side effect is that the
its_devstructure will not be freed and that the DID will stay mapped. Not a big deal, and this will be solved in the following patch.Quoted 5 lines
Signed-off-by Marc Zyngier <[email protected]>--- drivers/irqchip/irq-gic-v3-its-msi-parent.c | 10 ++++ drivers/irqchip/irq-gic-v3-its.c | 56 +++++++++++++-------- 2 files changed, 45 insertions(+), 21 deletions(-)First off, thanks a lot for putting this together, it makes an awful lot of sense to me.
Quoted 5 lines
index
0115ad6c82593..3472b97477104100644drivers/irqchip/irq-gic-v3-its.c +0
-0@@ -3620,8 +3620,43 @@ static int its_msi_prepare(struct irq_domain *domain, struct device *dev,return err;drivers/irqchip/irq-gic-v3-its.c static int its_msi_prepare(struct irq_domain *domain, struct device *dev, · line 3630
Quoted 10 lines
drivers/irqchip/irq-gic-v3-its.c +8
-0} +static void its_msi_teardown (struct irq_domain *domain,msi_alloc_info_t *info) +{ +struct msi_domain_info *msi_info; +struct its_device *its_dev; +struct its_node *its; +u32 dev_id; + + dev_id = info->scratchpad[0 ].ul;I have just managed to get to a keyboard :), I don't think the
dev_idmakes it to this point, we overwrite it with theits_devpointer inits_msi_prepare()(could use second scratchpad for the pointer maybe ?).
Quoted 2 lines
I was bitten by this while removing the old IWB code into the new one (unrelated to this code but that's how I noticed scratchpad is a union).
Gah, this is missing the fixup I had on top and that I didn't squash:
drivers/irqchip/irq-gic-v3-its.c +2 -12
@@ -3622,19 +3622,9 @@ static int its_msi_prepare(struct irq_domain *domain, struct device *dev,static void its_msi_teardown (struct irq_domain *domain,msi_alloc_info_t *info) { -struct msi_domain_info *msi_info; -struct its_device *its_dev; -struct its_node *its; -u32 dev_id; - - dev_id = info->scratchpad[0 ].ul; - - msi_info =msi_get_domain_info (domain); - its = msi_info->data; - -guard (mutex)(&its->dev_alloc_lock); +struct its_device *its_dev = info->scratchpad[0 ].ptr; - its_dev =its_find_device (its, dev_id); +guard (mutex)(&its_dev->its->dev_alloc_lock);/* If the device is shared, keep everything around */ if (its_dev->shared)
So no need for another scratchpad entry -- the device structure has everything we need. I'll repost things with tglx's comments addressed and this fix correctly squashed in.
Sorry for the noise,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Marc Zyngier
maintainerPATCH 3/4 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 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 | 39 ++++++++++++++++++++++++++++++++++----- 2 files changed, 36 insertions(+), 5 deletions(-)
include/linux/msi.h +2 -0
@@ -513,12 +513,14 @@ struct msi_domain_info {* @chip: Interrupt chip for this domain * @ops: MSI domain ops * @info: MSI domain info data +* @arg: 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 arg; };/*
kernel/irq/msi.c +34 -5
@@ -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
@@ -1025,6 +1026,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->arg; pops = parent->msi_parent_ops; snprintf(bundle->name, sizeof(bundle->name), "%s%s-%s",
@@ -1053,21 +1055,28 @@ bool msi_create_device_irq_domain(struct device *dev, unsigned int domid,msi_lock_descs (dev);if (WARN_ON_ONCE (msi_get_device_domain (dev, domid))) - goto fail; + goto fail_unlock;if (!pops->init_dev_msi_info (dev, parent, parent, &bundle->info)) - goto fail; + goto fail_unlock; domain =__msi_create_irq_domain (fwnode, &bundle->info,IRQ_DOMAIN_FLAG_MSI_DEVICE , parent);if (!domain) - goto fail; + goto fail_unlock; domain->dev = dev; dev->msi.data->__domains[domid].domain = domain; + +if (msi_domain_prepare_irqs (domain, dev, hwsize, &bundle->arg)) + goto fail; +msi_unlock_descs (dev);return true; fail: + dev->msi.data->__domains[domid].domain =NULL ; +irq_domain_remove (domain); +fail_unlock:msi_unlock_descs (dev); free_fwnode:irq_domain_free_fwnode (fwnalloced);
@@ -1250,6 +1259,26 @@ 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; +int ret =0 ; + +/* +* 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) + *arg = *info->alloc_data; +else + ret =msi_domain_prepare_irqs (domain, dev, nirqs, arg); + +return ret; +} +static int __msi_domain_alloc_irqs (struct device *dev,struct irq_domain *domain,struct msi_ctrl *ctrl) {
@@ -1262,7 +1291,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
Re: [PATCH 3/4] genirq/msi: Move prepare() call to per-device allocation
Quoted 5 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 between the MSI controller and the core code is that .msi_prepare() is called exactly once per device.
That contract is nowhere written in stone.
There are some MSI controller which get confused about that, but that's a problem of said controllers
include/linux/msi.h struct msi_domain_info { · line 516
Quoted 9 lines
include/linux/msi.h +1
-0@@ -513,12 +513,14 @@ struct msi_domain_info {* @chip: Interrupt chip for this domain * @ops: MSI domain ops * @info: MSI domain info data +* @arg: MSI domain allocation data (arch specific)
arg is a horrible name. Can this please be alloc_info or such?
include/linux/msi.h bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, · line 1055
Quoted 10 lines
include/linux/msi.h +1
-0@@ -1025,6 +1026,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->arg; pops = parent->msi_parent_ops; snprintf(bundle->name, sizeof(bundle->name), "%s%s-%s",@@ -1053,21 +1055,28 @@ bool msi_create_device_irq_domain(struct device *dev, unsigned int domid,msi_lock_descs (dev);
Please work against tip irq/msi which carries the guard() replacement for msi_lock_descs(). This patch heavily conflicts with the queued changes.
include/linux/msi.h bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, · line 1058
Quoted 3 lines
include/linux/msi.h +3
-0+static int __populate_alloc_info (struct irq_domain *domain,struct device *dev, +unsigned int nirqs,msi_alloc_info_t *arg) +{
Why does this need double underscores?
include/linux/msi.h bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, · line 1066
Quoted 8 lines
include/linux/msi.h +8
-0+struct msi_domain_info *info = domain->host_data; +int ret =0 ; + +/* +* 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.
That's only a matter of decades :)
include/linux/msi.h bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, · line 1073
Quoted 7 lines
include/linux/msi.h +7
-0+*/ +if (info->alloc_data) + *arg = *info->alloc_data; +else + ret =msi_domain_prepare_irqs (domain, dev, nirqs, arg); + +return ret;
if (!info->alloc_data)
return msi_domain_prepare_irqs(domain, dev, nirqs, arg);
*arg = *info->alloc_data;
return 0;perhaps?
Thanks,
tglx
Marc Zyngier
maintainerRe: [PATCH 3/4] genirq/msi: Move prepare() call to per-device allocation
Quoted 9 lines · Thomas Gleixner
Quoted 5 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 between the MSI controller and the core code is that .msi_prepare() is called exactly once per device.
That contract is nowhere written in stone.
It was *definitely* there the first place, and a baked in assumption since the ITS code was merged. You're welcome to come up with a new scheme, but the way the HW works requires this prepare phase to take place once per device.
If we can't have that, maybe we should consider reverting the GICv3/v4 code back to the pre-6.10 scheme that doesn't suffer from this issue.
Quoted 2 lines
There are some MSI controller which get confused about that, but that's a problem of said controllers
No. It's an infrastructure problem. This model worked before for a whole class of HW, until it was mutated into something else.
Quoted 12 lines
include/linux/msi.h struct msi_domain_info { · line 516
Quoted 10 lines
include/linux/msi.h +1
-0@@ -513,12 +513,14 @@ struct msi_domain_info {* @chip: Interrupt chip for this domain * @ops: MSI domain ops * @info: MSI domain info data +* @arg: MSI domain allocation data (arch specific) arg is a horrible name. Can this please be
alloc_infoor such?
Because that's the name every single function that takes it as a parameter uses? But sure, whatever name you want.
Quoted 16 lines
include/linux/msi.h bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, · line 1055
Quoted 11 lines
include/linux/msi.h +1
-0@@ -1025,6 +1026,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->arg; pops = parent->msi_parent_ops; snprintf(bundle->name, sizeof(bundle->name), "%s%s-%s",@@ -1053,21 +1055,28 @@ bool msi_create_device_irq_domain(struct device *dev, unsigned int domid,msi_lock_descs (dev);Please work against tip irq/msi which carries the
guard()replacement formsi_lock_descs(). This patch heavily conflicts with the queued changes.
Quoted 5 lines
include/linux/msi.h bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, · line 1058
Quoted 3 lines
include/linux/msi.h +3
-0+static int __populate_alloc_info (struct irq_domain *domain,struct device *dev, +unsigned int nirqs,msi_alloc_info_t *arg) +{Why does this need double underscores?
Because it doesn't look that out of place in this file?
Quoted 26 lines
include/linux/msi.h bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, · line 1066
Quoted 9 lines
include/linux/msi.h +8
-0+struct msi_domain_info *info = domain->host_data; +int ret =0 ; + +/* +* 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. That's only a matter of decades :)
include/linux/msi.h bool msi_create_device_irq_domain(struct device *dev, unsigned int domid, · line 1073
Quoted 7 lines
include/linux/msi.h +7
-0+*/ +if (info->alloc_data) + *arg = *info->alloc_data; +else + ret =msi_domain_prepare_irqs (domain, dev, nirqs, arg); + +return ret;if (!info->alloc_data) return msi_domain_prepare_irqs(domain, dev, nirqs, arg); *arg = *info->alloc_data; return 0;
Quoted 1 line
perhaps?
Sure.
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Marc Zyngier
maintainerPATCH 4/4 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
@@ -143,14 +132,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