| Message ID | 20200730170321.31228-3-valentin.schneider@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irqchip/gic-v2, v3: Enforce ACK/EOI pairing on IRQ retrigger | expand |
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.
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.
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.
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
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
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; }
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(-)