Re: [PATCH v4 08/20] irqchip/gic-v4.1: Plumb get/set_irqchip_state SGI callbacks

[Marc Zyngier]

On 2020-02-20 03:11, Zenghui Yu wrote:
> Hi Marc,
>
> On 2020/2/18 23:31, Marc Zyngier wrote:
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> > b/drivers/irqchip/irq-gic-v3-its.c
> > index 7656b353a95f..0ed286dba827 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > @@ -144,7 +144,7 @@ struct event_lpi_map {
> >  ÂÂÂÂ u16ÂÂÂÂÂÂÂÂÂÂÂ *col_map;
> >  ÂÂÂÂ irq_hw_number_tÂÂÂÂÂÂÂ lpi_base;
> >  ÂÂÂÂ intÂÂÂÂÂÂÂÂÂÂÂ nr_lpis;
> > -ÂÂÂ raw_spinlock_tÂÂÂÂÂÂÂ vlpi_lock;
> > +ÂÂÂ raw_spinlock_tÂÂÂÂÂÂÂ map_lock;
>
> So we use map_lock to protect both LPI's and VLPI's mapping affinity of
> a device, and use vpe_lock to protect vPE's affinity, OK.
>
> >  ÂÂÂÂ struct its_vmÂÂÂÂÂÂÂ *vm;
> >  ÂÂÂÂ struct its_vlpi_mapÂÂÂ *vlpi_maps;
> >  ÂÂÂÂ intÂÂÂÂÂÂÂÂÂÂÂ nr_vlpis;
> > @@ -240,15 +240,33 @@ static struct its_vlpi_map *get_vlpi_map(struct 
> > irq_data *d)
> >  ÂÂÂÂ return NULL;
> >  Â}
> >
> > -static int irq_to_cpuid(struct irq_data *d)
> > +static int irq_to_cpuid_lock(struct irq_data *d, unsigned long 
> > *flags)
> >  Â{
> > -ÂÂÂ struct its_device *its_dev = irq_data_get_irq_chip_data(d);
> >  ÂÂÂÂ struct its_vlpi_map *map = get_vlpi_map(d);
> > +ÂÂÂ int cpu;
> >
> > -ÂÂÂ if (map)
> > -ÂÂÂÂÂÂÂ return map->vpe->col_idx;
> > +ÂÂÂ if (map) {
> > +ÂÂÂÂÂÂÂ raw_spin_lock_irqsave(&map->vpe->vpe_lock, *flags);
> > +ÂÂÂÂÂÂÂ cpu = map->vpe->col_idx;
> > +ÂÂÂ } else {
> > +ÂÂÂÂÂÂÂ struct its_device *its_dev = irq_data_get_irq_chip_data(d);
> > +ÂÂÂÂÂÂÂ raw_spin_lock_irqsave(&its_dev->event_map.map_lock, *flags);
> > +ÂÂÂÂÂÂÂ cpu = its_dev->event_map.col_map[its_get_event_id(d)];
> > +ÂÂÂ }
> >
> > -ÂÂÂ return its_dev->event_map.col_map[its_get_event_id(d)];
> > +ÂÂÂ return cpu;
> > +}
>
> This helper is correct for normal LPIs and VLPIs, but wrong for per-vPE
> IRQ (doorbell) and vSGIs. irq_data_get_irq_chip_data() gets confused by
> both of them.

Yes, I've fixed that in the current state of the tree last week. Do have 
a
look if you can, but it seems to survive on both the model with v4.1 and
my D05.

[...]

> > -ÂÂÂÂÂÂÂ rdbase = per_cpu_ptr(gic_rdists->rdist, 
> > vpe->col_idx)->rd_base;
> > +ÂÂÂÂÂÂÂ cpu = irq_to_cpuid_lock(d, &flags);
> > +ÂÂÂÂÂÂÂ rdbase = per_cpu_ptr(gic_rdists->rdist, cpu)->rd_base;
> >  ÂÂÂÂÂÂÂÂ gic_write_lpir(d->parent_data->hwirq, rdbase + 
> > GICR_INVLPIR);
> >  ÂÂÂÂÂÂÂÂ wait_for_syncr(rdbase);
> > +ÂÂÂÂÂÂÂ irq_to_cpuid_unlock(d, flags);
> >  ÂÂÂÂ } else {
> >  ÂÂÂÂÂÂÂÂ its_vpe_send_cmd(vpe, its_send_inv);
> >  ÂÂÂÂ }
>
> Do we really need to grab the vpe_lock for those which are belong to
> the same irqchip with its_vpe_set_affinity()? The IRQ core code should
> already ensure the mutual exclusion among them, wrong?

I've been trying to think about that, but jet-lag keeps getting in the 
way.
I empirically think that you are right, but I need to go and check the 
various
code paths to be sure. Hopefully I'll have a bit more brain space next 
week.

For sure this patch tries to do too many things at once...

        M.
--
Jazz is not dead. It just smells funny...