* [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
@ 2020-07-30 17:03 ` Valentin Schneider
0 siblings, 0 replies; 18+ messages in thread
From: Valentin Schneider @ 2020-07-30 17:03 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: Thomas Gleixner, Jason Cooper, Marc Zyngier, Lorenzo Pieralisi
The GIC irqchips can now use a HW resend when a retrigger is invoked by
check_irq_resend(). However, should the HW resend fail, check_irq_resend()
will still attempt to trigger a SW resend, which is still a bad idea for
the GICs.
Prevent this from happening by setting IRQD_HANDLE_ENFORCE_IRQCTX on all
GIC IRQs. Technically per-cpu IRQs do not need this, as their flow handlers
never set IRQS_PENDING, but this aligns all IRQs wrt context enforcement:
this also forces all GIC IRQ handling to happen in IRQ context (as defined
by in_irq()).
Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
drivers/irqchip/irq-gic-v3.c | 5 ++++-
drivers/irqchip/irq-gic.c | 6 +++++-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 0fbcbf55ec48..1a8acf7cd8ac 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1279,6 +1279,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hw)
{
struct irq_chip *chip = &gic_chip;
+ struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
if (static_branch_likely(&supports_deactivate_key))
chip = &gic_eoimode1_chip;
@@ -1296,7 +1297,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
irq_domain_set_info(d, irq, hw, chip, d->host_data,
handle_fasteoi_irq, NULL, NULL);
irq_set_probe(irq);
- irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
+ irqd_set_single_target(irqd);
break;
case LPI_RANGE:
@@ -1310,6 +1311,8 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
return -EPERM;
}
+ /* Prevents SW retriggers which mess up the ACK/EOI ordering */
+ irqd_set_handle_enforce_irqctx(irqd);
return 0;
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index e2b4cae88bce..a91ce1e73bd2 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -983,6 +983,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hw)
{
struct gic_chip_data *gic = d->host_data;
+ struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
if (hw < 32) {
irq_set_percpu_devid(irq);
@@ -992,8 +993,11 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data,
handle_fasteoi_irq, NULL, NULL);
irq_set_probe(irq);
- irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
+ irqd_set_single_target(irqd);
}
+
+ /* Prevents SW retriggers which mess up the ACK/EOI ordering */
+ irqd_set_handle_enforce_irqctx(irqd);
return 0;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
2020-07-30 17:03 ` Valentin Schneider
@ 2020-07-30 18:10 ` Marc Zyngier
-1 siblings, 0 replies; 18+ messages in thread
From: Marc Zyngier @ 2020-07-30 18:10 UTC (permalink / raw)
To: Valentin Schneider
Cc: Thomas Gleixner, Lorenzo Pieralisi, linux-kernel,
linux-arm-kernel, Jason Cooper
Hi Valentin,
On 2020-07-30 18:03, Valentin Schneider wrote:
> The GIC irqchips can now use a HW resend when a retrigger is invoked by
> check_irq_resend(). However, should the HW resend fail,
> check_irq_resend()
> will still attempt to trigger a SW resend, which is still a bad idea
> for
> the GICs.
>
> Prevent this from happening by setting IRQD_HANDLE_ENFORCE_IRQCTX on
> all
> GIC IRQs. Technically per-cpu IRQs do not need this, as their flow
> handlers
> never set IRQS_PENDING, but this aligns all IRQs wrt context
> enforcement:
> this also forces all GIC IRQ handling to happen in IRQ context (as
> defined
> by in_irq()).
>
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
> drivers/irqchip/irq-gic-v3.c | 5 ++++-
> drivers/irqchip/irq-gic.c | 6 +++++-
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c
> b/drivers/irqchip/irq-gic-v3.c
> index 0fbcbf55ec48..1a8acf7cd8ac 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -1279,6 +1279,7 @@ static int gic_irq_domain_map(struct irq_domain
> *d, unsigned int irq,
> irq_hw_number_t hw)
> {
> struct irq_chip *chip = &gic_chip;
> + struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
>
> if (static_branch_likely(&supports_deactivate_key))
> chip = &gic_eoimode1_chip;
> @@ -1296,7 +1297,7 @@ static int gic_irq_domain_map(struct irq_domain
> *d, unsigned int irq,
> irq_domain_set_info(d, irq, hw, chip, d->host_data,
> handle_fasteoi_irq, NULL, NULL);
> irq_set_probe(irq);
> - irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
> + irqd_set_single_target(irqd);
> break;
>
> case LPI_RANGE:
> @@ -1310,6 +1311,8 @@ static int gic_irq_domain_map(struct irq_domain
> *d, unsigned int irq,
> return -EPERM;
> }
>
> + /* Prevents SW retriggers which mess up the ACK/EOI ordering */
> + irqd_set_handle_enforce_irqctx(irqd);
> return 0;
> }
>
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index e2b4cae88bce..a91ce1e73bd2 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -983,6 +983,7 @@ static int gic_irq_domain_map(struct irq_domain
> *d, unsigned int irq,
> irq_hw_number_t hw)
> {
> struct gic_chip_data *gic = d->host_data;
> + struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
>
> if (hw < 32) {
> irq_set_percpu_devid(irq);
> @@ -992,8 +993,11 @@ static int gic_irq_domain_map(struct irq_domain
> *d, unsigned int irq,
> irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data,
> handle_fasteoi_irq, NULL, NULL);
> irq_set_probe(irq);
> - irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
> + irqd_set_single_target(irqd);
> }
> +
> + /* Prevents SW retriggers which mess up the ACK/EOI ordering */
> + irqd_set_handle_enforce_irqctx(irqd);
> return 0;
> }
I'm OK with this in principle, but this requires additional changes
in the rest of the GIC universe. The ITS driver needs to provide its own
retrigger function for LPIs (queuing an INT command), and any of the
SPI generating widgets that can be stacked on top of a GIC (GICv3-MBI,
GICv2m, and all the other Annapurna/Marvell/NVDIA wonders need to gain
directly or indirectly a call to irq_chip_retrigger_hierarchy().
We can probably avoid changing the MSI widgets by teaching the MSI
code about the HW retrigger, but a number of other non-MSI drivers
will need some help...
I'll have a look tomorrow.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
@ 2020-07-30 18:10 ` Marc Zyngier
0 siblings, 0 replies; 18+ messages in thread
From: Marc Zyngier @ 2020-07-30 18:10 UTC (permalink / raw)
To: Valentin Schneider
Cc: linux-kernel, linux-arm-kernel, Thomas Gleixner, Jason Cooper,
Lorenzo Pieralisi
Hi Valentin,
On 2020-07-30 18:03, Valentin Schneider wrote:
> The GIC irqchips can now use a HW resend when a retrigger is invoked by
> check_irq_resend(). However, should the HW resend fail,
> check_irq_resend()
> will still attempt to trigger a SW resend, which is still a bad idea
> for
> the GICs.
>
> Prevent this from happening by setting IRQD_HANDLE_ENFORCE_IRQCTX on
> all
> GIC IRQs. Technically per-cpu IRQs do not need this, as their flow
> handlers
> never set IRQS_PENDING, but this aligns all IRQs wrt context
> enforcement:
> this also forces all GIC IRQ handling to happen in IRQ context (as
> defined
> by in_irq()).
>
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
> drivers/irqchip/irq-gic-v3.c | 5 ++++-
> drivers/irqchip/irq-gic.c | 6 +++++-
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c
> b/drivers/irqchip/irq-gic-v3.c
> index 0fbcbf55ec48..1a8acf7cd8ac 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -1279,6 +1279,7 @@ static int gic_irq_domain_map(struct irq_domain
> *d, unsigned int irq,
> irq_hw_number_t hw)
> {
> struct irq_chip *chip = &gic_chip;
> + struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
>
> if (static_branch_likely(&supports_deactivate_key))
> chip = &gic_eoimode1_chip;
> @@ -1296,7 +1297,7 @@ static int gic_irq_domain_map(struct irq_domain
> *d, unsigned int irq,
> irq_domain_set_info(d, irq, hw, chip, d->host_data,
> handle_fasteoi_irq, NULL, NULL);
> irq_set_probe(irq);
> - irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
> + irqd_set_single_target(irqd);
> break;
>
> case LPI_RANGE:
> @@ -1310,6 +1311,8 @@ static int gic_irq_domain_map(struct irq_domain
> *d, unsigned int irq,
> return -EPERM;
> }
>
> + /* Prevents SW retriggers which mess up the ACK/EOI ordering */
> + irqd_set_handle_enforce_irqctx(irqd);
> return 0;
> }
>
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index e2b4cae88bce..a91ce1e73bd2 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -983,6 +983,7 @@ static int gic_irq_domain_map(struct irq_domain
> *d, unsigned int irq,
> irq_hw_number_t hw)
> {
> struct gic_chip_data *gic = d->host_data;
> + struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
>
> if (hw < 32) {
> irq_set_percpu_devid(irq);
> @@ -992,8 +993,11 @@ static int gic_irq_domain_map(struct irq_domain
> *d, unsigned int irq,
> irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data,
> handle_fasteoi_irq, NULL, NULL);
> irq_set_probe(irq);
> - irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
> + irqd_set_single_target(irqd);
> }
> +
> + /* Prevents SW retriggers which mess up the ACK/EOI ordering */
> + irqd_set_handle_enforce_irqctx(irqd);
> return 0;
> }
I'm OK with this in principle, but this requires additional changes
in the rest of the GIC universe. The ITS driver needs to provide its own
retrigger function for LPIs (queuing an INT command), and any of the
SPI generating widgets that can be stacked on top of a GIC (GICv3-MBI,
GICv2m, and all the other Annapurna/Marvell/NVDIA wonders need to gain
directly or indirectly a call to irq_chip_retrigger_hierarchy().
We can probably avoid changing the MSI widgets by teaching the MSI
code about the HW retrigger, but a number of other non-MSI drivers
will need some help...
I'll have a look tomorrow.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
2020-07-30 18:10 ` Marc Zyngier
@ 2020-07-31 0:08 ` Valentin Schneider
-1 siblings, 0 replies; 18+ messages in thread
From: Valentin Schneider @ 2020-07-31 0:08 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Lorenzo Pieralisi, linux-kernel,
linux-arm-kernel, Jason Cooper
Hi Marc,
On 30/07/20 19:10, Marc Zyngier wrote:
[...]
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index e2b4cae88bce..a91ce1e73bd2 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -983,6 +983,7 @@ static int gic_irq_domain_map(struct irq_domain
>> *d, unsigned int irq,
>> irq_hw_number_t hw)
>> {
>> struct gic_chip_data *gic = d->host_data;
>> + struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
>>
>> if (hw < 32) {
>> irq_set_percpu_devid(irq);
>> @@ -992,8 +993,11 @@ static int gic_irq_domain_map(struct irq_domain
>> *d, unsigned int irq,
>> irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data,
>> handle_fasteoi_irq, NULL, NULL);
>> irq_set_probe(irq);
>> - irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
>> + irqd_set_single_target(irqd);
>> }
>> +
>> + /* Prevents SW retriggers which mess up the ACK/EOI ordering */
>> + irqd_set_handle_enforce_irqctx(irqd);
>> return 0;
>> }
>
> I'm OK with this in principle, but this requires additional changes
> in the rest of the GIC universe. The ITS driver needs to provide its own
> retrigger function for LPIs (queuing an INT command), and any of the
> SPI generating widgets that can be stacked on top of a GIC (GICv3-MBI,
> GICv2m, and all the other Annapurna/Marvell/NVDIA wonders need to gain
> directly or indirectly a call to irq_chip_retrigger_hierarchy().
>
Eep, yes indeed... I didn't see that can was full of worms, though even if
it only really matters for eoimode=0 I think it might still be worth it
(if only to respect the spec).
> We can probably avoid changing the MSI widgets by teaching the MSI
> code about the HW retrigger, but a number of other non-MSI drivers
> will need some help...
>
> I'll have a look tomorrow.
>
For LPIs AFAICT we could directly reuse its_irq_set_irqchip_state() - I see
the VPE side of things already has a HW retrigger callback.
For gicv2m, I *think* we'd want irq_chip_retrigger_hierarchy() on both MSI
domains (which IIUC you suggest might be doable by adding the retrigger as
a default MSI chip op).
I'm not very familiar with the rest of the fauna, so I'll have to do some
reading tomorrow as well; it's probably high time for me to actually read
up on LPIs & ITS while I'm at it...
> Thanks,
>
> M.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
@ 2020-07-31 0:08 ` Valentin Schneider
0 siblings, 0 replies; 18+ messages in thread
From: Valentin Schneider @ 2020-07-31 0:08 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-kernel, linux-arm-kernel, Thomas Gleixner, Jason Cooper,
Lorenzo Pieralisi
Hi Marc,
On 30/07/20 19:10, Marc Zyngier wrote:
[...]
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index e2b4cae88bce..a91ce1e73bd2 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -983,6 +983,7 @@ static int gic_irq_domain_map(struct irq_domain
>> *d, unsigned int irq,
>> irq_hw_number_t hw)
>> {
>> struct gic_chip_data *gic = d->host_data;
>> + struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
>>
>> if (hw < 32) {
>> irq_set_percpu_devid(irq);
>> @@ -992,8 +993,11 @@ static int gic_irq_domain_map(struct irq_domain
>> *d, unsigned int irq,
>> irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data,
>> handle_fasteoi_irq, NULL, NULL);
>> irq_set_probe(irq);
>> - irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
>> + irqd_set_single_target(irqd);
>> }
>> +
>> + /* Prevents SW retriggers which mess up the ACK/EOI ordering */
>> + irqd_set_handle_enforce_irqctx(irqd);
>> return 0;
>> }
>
> I'm OK with this in principle, but this requires additional changes
> in the rest of the GIC universe. The ITS driver needs to provide its own
> retrigger function for LPIs (queuing an INT command), and any of the
> SPI generating widgets that can be stacked on top of a GIC (GICv3-MBI,
> GICv2m, and all the other Annapurna/Marvell/NVDIA wonders need to gain
> directly or indirectly a call to irq_chip_retrigger_hierarchy().
>
Eep, yes indeed... I didn't see that can was full of worms, though even if
it only really matters for eoimode=0 I think it might still be worth it
(if only to respect the spec).
> We can probably avoid changing the MSI widgets by teaching the MSI
> code about the HW retrigger, but a number of other non-MSI drivers
> will need some help...
>
> I'll have a look tomorrow.
>
For LPIs AFAICT we could directly reuse its_irq_set_irqchip_state() - I see
the VPE side of things already has a HW retrigger callback.
For gicv2m, I *think* we'd want irq_chip_retrigger_hierarchy() on both MSI
domains (which IIUC you suggest might be doable by adding the retrigger as
a default MSI chip op).
I'm not very familiar with the rest of the fauna, so I'll have to do some
reading tomorrow as well; it's probably high time for me to actually read
up on LPIs & ITS while I'm at it...
> Thanks,
>
> M.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
2020-07-31 0:08 ` Valentin Schneider
@ 2020-07-31 8:08 ` Marc Zyngier
-1 siblings, 0 replies; 18+ messages in thread
From: Marc Zyngier @ 2020-07-31 8:08 UTC (permalink / raw)
To: Valentin Schneider
Cc: Thomas Gleixner, Lorenzo Pieralisi, linux-kernel,
linux-arm-kernel, Jason Cooper
Hi Valentin,
On 2020-07-31 01:08, Valentin Schneider wrote:
> Hi Marc,
>
> On 30/07/20 19:10, Marc Zyngier wrote:
> [...]
>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>> index e2b4cae88bce..a91ce1e73bd2 100644
>>> --- a/drivers/irqchip/irq-gic.c
>>> +++ b/drivers/irqchip/irq-gic.c
>>> @@ -983,6 +983,7 @@ static int gic_irq_domain_map(struct irq_domain
>>> *d, unsigned int irq,
>>> irq_hw_number_t hw)
>>> {
>>> struct gic_chip_data *gic = d->host_data;
>>> + struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
>>>
>>> if (hw < 32) {
>>> irq_set_percpu_devid(irq);
>>> @@ -992,8 +993,11 @@ static int gic_irq_domain_map(struct irq_domain
>>> *d, unsigned int irq,
>>> irq_domain_set_info(d, irq, hw, &gic->chip,
>>> d->host_data,
>>> handle_fasteoi_irq, NULL, NULL);
>>> irq_set_probe(irq);
>>> - irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
>>> + irqd_set_single_target(irqd);
>>> }
>>> +
>>> + /* Prevents SW retriggers which mess up the ACK/EOI ordering */
>>> + irqd_set_handle_enforce_irqctx(irqd);
>>> return 0;
>>> }
>>
>> I'm OK with this in principle, but this requires additional changes
>> in the rest of the GIC universe. The ITS driver needs to provide its
>> own
>> retrigger function for LPIs (queuing an INT command), and any of the
>> SPI generating widgets that can be stacked on top of a GIC (GICv3-MBI,
>> GICv2m, and all the other Annapurna/Marvell/NVDIA wonders need to gain
>> directly or indirectly a call to irq_chip_retrigger_hierarchy().
>>
>
> Eep, yes indeed... I didn't see that can was full of worms, though even
> if
> it only really matters for eoimode=0 I think it might still be worth it
> (if only to respect the spec).
Well, given that we are using EOImode=0 for all guests at the moment,
there is some value it getting it right! ;-)
>
>> We can probably avoid changing the MSI widgets by teaching the MSI
>> code about the HW retrigger, but a number of other non-MSI drivers
>> will need some help...
>>
>> I'll have a look tomorrow.
>>
>
> For LPIs AFAICT we could directly reuse its_irq_set_irqchip_state() - I
> see
> the VPE side of things already has a HW retrigger callback.
Yes, that's the idea (in general, if you implement the PENDING side of
irq_set_irqchip_state(), retrigger comes for free).
> For gicv2m, I *think* we'd want irq_chip_retrigger_hierarchy() on both
> MSI
> domains (which IIUC you suggest might be doable by adding the retrigger
> as
> a default MSI chip op).
Yes, that was my idea.
> I'm not very familiar with the rest of the fauna, so I'll have to do
> some
> reading tomorrow as well; it's probably high time for me to actually
> read
> up on LPIs & ITS while I'm at it...
Look for anything that performs an interrupt allocation by calling
into the parent with a 3 cell (DT case) fwspec. There is a bunch
of them.
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
@ 2020-07-31 8:08 ` Marc Zyngier
0 siblings, 0 replies; 18+ messages in thread
From: Marc Zyngier @ 2020-07-31 8:08 UTC (permalink / raw)
To: Valentin Schneider
Cc: linux-kernel, linux-arm-kernel, Thomas Gleixner, Jason Cooper,
Lorenzo Pieralisi
Hi Valentin,
On 2020-07-31 01:08, Valentin Schneider wrote:
> Hi Marc,
>
> On 30/07/20 19:10, Marc Zyngier wrote:
> [...]
>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>> index e2b4cae88bce..a91ce1e73bd2 100644
>>> --- a/drivers/irqchip/irq-gic.c
>>> +++ b/drivers/irqchip/irq-gic.c
>>> @@ -983,6 +983,7 @@ static int gic_irq_domain_map(struct irq_domain
>>> *d, unsigned int irq,
>>> irq_hw_number_t hw)
>>> {
>>> struct gic_chip_data *gic = d->host_data;
>>> + struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
>>>
>>> if (hw < 32) {
>>> irq_set_percpu_devid(irq);
>>> @@ -992,8 +993,11 @@ static int gic_irq_domain_map(struct irq_domain
>>> *d, unsigned int irq,
>>> irq_domain_set_info(d, irq, hw, &gic->chip,
>>> d->host_data,
>>> handle_fasteoi_irq, NULL, NULL);
>>> irq_set_probe(irq);
>>> - irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
>>> + irqd_set_single_target(irqd);
>>> }
>>> +
>>> + /* Prevents SW retriggers which mess up the ACK/EOI ordering */
>>> + irqd_set_handle_enforce_irqctx(irqd);
>>> return 0;
>>> }
>>
>> I'm OK with this in principle, but this requires additional changes
>> in the rest of the GIC universe. The ITS driver needs to provide its
>> own
>> retrigger function for LPIs (queuing an INT command), and any of the
>> SPI generating widgets that can be stacked on top of a GIC (GICv3-MBI,
>> GICv2m, and all the other Annapurna/Marvell/NVDIA wonders need to gain
>> directly or indirectly a call to irq_chip_retrigger_hierarchy().
>>
>
> Eep, yes indeed... I didn't see that can was full of worms, though even
> if
> it only really matters for eoimode=0 I think it might still be worth it
> (if only to respect the spec).
Well, given that we are using EOImode=0 for all guests at the moment,
there is some value it getting it right! ;-)
>
>> We can probably avoid changing the MSI widgets by teaching the MSI
>> code about the HW retrigger, but a number of other non-MSI drivers
>> will need some help...
>>
>> I'll have a look tomorrow.
>>
>
> For LPIs AFAICT we could directly reuse its_irq_set_irqchip_state() - I
> see
> the VPE side of things already has a HW retrigger callback.
Yes, that's the idea (in general, if you implement the PENDING side of
irq_set_irqchip_state(), retrigger comes for free).
> For gicv2m, I *think* we'd want irq_chip_retrigger_hierarchy() on both
> MSI
> domains (which IIUC you suggest might be doable by adding the retrigger
> as
> a default MSI chip op).
Yes, that was my idea.
> I'm not very familiar with the rest of the fauna, so I'll have to do
> some
> reading tomorrow as well; it's probably high time for me to actually
> read
> up on LPIs & ITS while I'm at it...
Look for anything that performs an interrupt allocation by calling
into the parent with a 3 cell (DT case) fwspec. There is a bunch
of them.
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
2020-07-31 8:08 ` Marc Zyngier
@ 2020-07-31 11:27 ` Marc Zyngier
-1 siblings, 0 replies; 18+ messages in thread
From: Marc Zyngier @ 2020-07-31 11:27 UTC (permalink / raw)
To: Valentin Schneider
Cc: Thomas Gleixner, Lorenzo Pieralisi, linux-kernel,
linux-arm-kernel, Jason Cooper
On 2020-07-31 09:08, Marc Zyngier wrote:
> Hi Valentin,
>
> On 2020-07-31 01:08, Valentin Schneider wrote:
>> Hi Marc,
>>
>> On 30/07/20 19:10, Marc Zyngier wrote:
>> [...]
>>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>>> index e2b4cae88bce..a91ce1e73bd2 100644
>>>> --- a/drivers/irqchip/irq-gic.c
>>>> +++ b/drivers/irqchip/irq-gic.c
>>>> @@ -983,6 +983,7 @@ static int gic_irq_domain_map(struct irq_domain
>>>> *d, unsigned int irq,
>>>> irq_hw_number_t hw)
>>>> {
>>>> struct gic_chip_data *gic = d->host_data;
>>>> + struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
>>>>
>>>> if (hw < 32) {
>>>> irq_set_percpu_devid(irq);
>>>> @@ -992,8 +993,11 @@ static int gic_irq_domain_map(struct irq_domain
>>>> *d, unsigned int irq,
>>>> irq_domain_set_info(d, irq, hw, &gic->chip,
>>>> d->host_data,
>>>> handle_fasteoi_irq, NULL, NULL);
>>>> irq_set_probe(irq);
>>>> - irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
>>>> + irqd_set_single_target(irqd);
>>>> }
>>>> +
>>>> + /* Prevents SW retriggers which mess up the ACK/EOI ordering */
>>>> + irqd_set_handle_enforce_irqctx(irqd);
>>>> return 0;
>>>> }
>>>
>>> I'm OK with this in principle, but this requires additional changes
>>> in the rest of the GIC universe. The ITS driver needs to provide its
>>> own
>>> retrigger function for LPIs (queuing an INT command), and any of the
>>> SPI generating widgets that can be stacked on top of a GIC
>>> (GICv3-MBI,
>>> GICv2m, and all the other Annapurna/Marvell/NVDIA wonders need to
>>> gain
>>> directly or indirectly a call to irq_chip_retrigger_hierarchy().
>>>
>>
>> Eep, yes indeed... I didn't see that can was full of worms, though
>> even if
>> it only really matters for eoimode=0 I think it might still be worth
>> it
>> (if only to respect the spec).
>
> Well, given that we are using EOImode=0 for all guests at the moment,
> there is some value it getting it right! ;-)
>
>>
>>> We can probably avoid changing the MSI widgets by teaching the MSI
>>> code about the HW retrigger, but a number of other non-MSI drivers
>>> will need some help...
>>>
>>> I'll have a look tomorrow.
>>>
>>
>> For LPIs AFAICT we could directly reuse its_irq_set_irqchip_state() -
>> I see
>> the VPE side of things already has a HW retrigger callback.
>
> Yes, that's the idea (in general, if you implement the PENDING side of
> irq_set_irqchip_state(), retrigger comes for free).
>
>> For gicv2m, I *think* we'd want irq_chip_retrigger_hierarchy() on both
>> MSI
>> domains (which IIUC you suggest might be doable by adding the
>> retrigger as
>> a default MSI chip op).
>
> Yes, that was my idea.
>
>> I'm not very familiar with the rest of the fauna, so I'll have to do
>> some
>> reading tomorrow as well; it's probably high time for me to actually
>> read
>> up on LPIs & ITS while I'm at it...
>
> Look for anything that performs an interrupt allocation by calling
> into the parent with a 3 cell (DT case) fwspec. There is a bunch
> of them.
For what it is worth, I have just pushed out a branch[1] containing some
of this rework as well as your patches.
The only tricky part is the GICv4.1 doorbell retriggering, which just
can't be re-injected. It shouldn't matter though. Same for vSGIs, they
never fire on the host.
Thanks,
M.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gic-retrigger
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
@ 2020-07-31 11:27 ` Marc Zyngier
0 siblings, 0 replies; 18+ messages in thread
From: Marc Zyngier @ 2020-07-31 11:27 UTC (permalink / raw)
To: Valentin Schneider
Cc: linux-kernel, linux-arm-kernel, Thomas Gleixner, Jason Cooper,
Lorenzo Pieralisi
On 2020-07-31 09:08, Marc Zyngier wrote:
> Hi Valentin,
>
> On 2020-07-31 01:08, Valentin Schneider wrote:
>> Hi Marc,
>>
>> On 30/07/20 19:10, Marc Zyngier wrote:
>> [...]
>>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>>> index e2b4cae88bce..a91ce1e73bd2 100644
>>>> --- a/drivers/irqchip/irq-gic.c
>>>> +++ b/drivers/irqchip/irq-gic.c
>>>> @@ -983,6 +983,7 @@ static int gic_irq_domain_map(struct irq_domain
>>>> *d, unsigned int irq,
>>>> irq_hw_number_t hw)
>>>> {
>>>> struct gic_chip_data *gic = d->host_data;
>>>> + struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
>>>>
>>>> if (hw < 32) {
>>>> irq_set_percpu_devid(irq);
>>>> @@ -992,8 +993,11 @@ static int gic_irq_domain_map(struct irq_domain
>>>> *d, unsigned int irq,
>>>> irq_domain_set_info(d, irq, hw, &gic->chip,
>>>> d->host_data,
>>>> handle_fasteoi_irq, NULL, NULL);
>>>> irq_set_probe(irq);
>>>> - irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
>>>> + irqd_set_single_target(irqd);
>>>> }
>>>> +
>>>> + /* Prevents SW retriggers which mess up the ACK/EOI ordering */
>>>> + irqd_set_handle_enforce_irqctx(irqd);
>>>> return 0;
>>>> }
>>>
>>> I'm OK with this in principle, but this requires additional changes
>>> in the rest of the GIC universe. The ITS driver needs to provide its
>>> own
>>> retrigger function for LPIs (queuing an INT command), and any of the
>>> SPI generating widgets that can be stacked on top of a GIC
>>> (GICv3-MBI,
>>> GICv2m, and all the other Annapurna/Marvell/NVDIA wonders need to
>>> gain
>>> directly or indirectly a call to irq_chip_retrigger_hierarchy().
>>>
>>
>> Eep, yes indeed... I didn't see that can was full of worms, though
>> even if
>> it only really matters for eoimode=0 I think it might still be worth
>> it
>> (if only to respect the spec).
>
> Well, given that we are using EOImode=0 for all guests at the moment,
> there is some value it getting it right! ;-)
>
>>
>>> We can probably avoid changing the MSI widgets by teaching the MSI
>>> code about the HW retrigger, but a number of other non-MSI drivers
>>> will need some help...
>>>
>>> I'll have a look tomorrow.
>>>
>>
>> For LPIs AFAICT we could directly reuse its_irq_set_irqchip_state() -
>> I see
>> the VPE side of things already has a HW retrigger callback.
>
> Yes, that's the idea (in general, if you implement the PENDING side of
> irq_set_irqchip_state(), retrigger comes for free).
>
>> For gicv2m, I *think* we'd want irq_chip_retrigger_hierarchy() on both
>> MSI
>> domains (which IIUC you suggest might be doable by adding the
>> retrigger as
>> a default MSI chip op).
>
> Yes, that was my idea.
>
>> I'm not very familiar with the rest of the fauna, so I'll have to do
>> some
>> reading tomorrow as well; it's probably high time for me to actually
>> read
>> up on LPIs & ITS while I'm at it...
>
> Look for anything that performs an interrupt allocation by calling
> into the parent with a 3 cell (DT case) fwspec. There is a bunch
> of them.
For what it is worth, I have just pushed out a branch[1] containing some
of this rework as well as your patches.
The only tricky part is the GICv4.1 doorbell retriggering, which just
can't be re-injected. It shouldn't matter though. Same for vSGIs, they
never fire on the host.
Thanks,
M.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gic-retrigger
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
2020-07-31 11:27 ` Marc Zyngier
@ 2020-07-31 12:11 ` Valentin Schneider
-1 siblings, 0 replies; 18+ messages in thread
From: Valentin Schneider @ 2020-07-31 12:11 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Lorenzo Pieralisi, linux-kernel,
linux-arm-kernel, Jason Cooper
On 31/07/20 12:27, Marc Zyngier wrote:
>> Look for anything that performs an interrupt allocation by calling
>> into the parent with a 3 cell (DT case) fwspec. There is a bunch
>> of them.
>
> For what it is worth, I have just pushed out a branch[1] containing some
> of this rework as well as your patches.
>
Brilliant, thanks for taking a shot at this! I'll try to look for stragglers.
> The only tricky part is the GICv4.1 doorbell retriggering, which just
> can't be re-injected. It shouldn't matter though. Same for vSGIs, they
> never fire on the host.
>
> Thanks,
>
> M.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gic-retrigger
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely
@ 2020-07-31 12:11 ` Valentin Schneider
0 siblings, 0 replies; 18+ messages in thread
From: Valentin Schneider @ 2020-07-31 12:11 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-kernel, linux-arm-kernel, Thomas Gleixner, Jason Cooper,
Lorenzo Pieralisi
On 31/07/20 12:27, Marc Zyngier wrote:
>> Look for anything that performs an interrupt allocation by calling
>> into the parent with a 3 cell (DT case) fwspec. There is a bunch
>> of them.
>
> For what it is worth, I have just pushed out a branch[1] containing some
> of this rework as well as your patches.
>
Brilliant, thanks for taking a shot at this! I'll try to look for stragglers.
> The only tricky part is the GICv4.1 doorbell retriggering, which just
> can't be re-injected. It shouldn't matter though. Same for vSGIs, they
> never fire on the host.
>
> Thanks,
>
> M.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gic-retrigger
^ permalink raw reply [flat|nested] 18+ messages in thread
* [tip: irq/core] irqchip/gic-v2, v3: Prevent SW resends entirely
2020-07-30 17:03 ` Valentin Schneider
(?)
(?)
@ 2020-10-11 17:57 ` tip-bot2 for Valentin Schneider
-1 siblings, 0 replies; 18+ messages in thread
From: tip-bot2 for Valentin Schneider @ 2020-10-11 17:57 UTC (permalink / raw)
To: linux-tip-commits; +Cc: Valentin Schneider, Marc Zyngier, x86, LKML
The following commit has been merged into the irq/core branch of tip:
Commit-ID: 1b57d91b969cda1d2c3530f2e829ca366a9c7df7
Gitweb: https://git.kernel.org/tip/1b57d91b969cda1d2c3530f2e829ca366a9c7df7
Author: Valentin Schneider <valentin.schneider@arm.com>
AuthorDate: Thu, 30 Jul 2020 18:03:21 +01:00
Committer: Marc Zyngier <maz@kernel.org>
CommitterDate: Sun, 06 Sep 2020 18:26:13 +01:00
irqchip/gic-v2, v3: Prevent SW resends entirely
The GIC irqchips can now use a HW resend when a retrigger is invoked by
check_irq_resend(). However, should the HW resend fail, check_irq_resend()
will still attempt to trigger a SW resend, which is still a bad idea for
the GICs.
Prevent this from happening by setting IRQD_HANDLE_ENFORCE_IRQCTX on all
GIC IRQs. Technically per-cpu IRQs do not need this, as their flow handlers
never set IRQS_PENDING, but this aligns all IRQs wrt context enforcement:
this also forces all GIC IRQ handling to happen in IRQ context (as defined
by in_irq()).
Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20200730170321.31228-3-valentin.schneider@arm.com
---
drivers/irqchip/irq-gic-v3.c | 5 ++++-
drivers/irqchip/irq-gic.c | 6 +++++-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index b507bc7..4e9387a 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1279,6 +1279,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hw)
{
struct irq_chip *chip = &gic_chip;
+ struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
if (static_branch_likely(&supports_deactivate_key))
chip = &gic_eoimode1_chip;
@@ -1296,7 +1297,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
irq_domain_set_info(d, irq, hw, chip, d->host_data,
handle_fasteoi_irq, NULL, NULL);
irq_set_probe(irq);
- irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
+ irqd_set_single_target(irqd);
break;
case LPI_RANGE:
@@ -1310,6 +1311,8 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
return -EPERM;
}
+ /* Prevents SW retriggers which mess up the ACK/EOI ordering */
+ irqd_set_handle_enforce_irqctx(irqd);
return 0;
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index e92ee2b..b59bcef 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -975,6 +975,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hw)
{
struct gic_chip_data *gic = d->host_data;
+ struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
if (hw < 32) {
irq_set_percpu_devid(irq);
@@ -984,8 +985,11 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data,
handle_fasteoi_irq, NULL, NULL);
irq_set_probe(irq);
- irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
+ irqd_set_single_target(irqd);
}
+
+ /* Prevents SW retriggers which mess up the ACK/EOI ordering */
+ irqd_set_handle_enforce_irqctx(irqd);
return 0;
}
^ permalink raw reply related [flat|nested] 18+ messages in thread