Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely

[Marc Zyngier]

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...