Re: [PATCH] irqchip/gic-v3-its: Don't confuse get_vlpi_map() by writing DB config

[Zenghui Yu]

Hi Marc,

On 2020/1/22 18:44, Marc Zyngier wrote:
> Hi Zenghui,
>
> Thanks for this.
>
> On 2020-01-22 08:56, Zenghui Yu wrote:
> > When we're writing config for the doorbell interrupt, get_vlpi_map() will
> > get confused by doorbell's d->parent_data hack and find the wrong its_dev
> > as chip data and the wrong event.
> >
> > Fix this issue by making sure no doorbells will be involved before 
> > invoking
> > get_vlpi_map(), which restore some of the logic in lpi_write_config().
> >
> > Fixes: c1d4d5cd203c ("irqchip/gic-v3-its: Add its_vlpi_map helpers")
> > Signed-off-by: Zenghui Yu <yuzenghui@xxxxxxxxxx>
> > ---
> >
> > This is based on mainline and can't be directly applied to the current
> > irqchip-next.
> >
> > Âdrivers/irqchip/irq-gic-v3-its.c | 5 +++--
> > Â1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> > b/drivers/irqchip/irq-gic-v3-its.c
> > index e05673bcd52b..cc8a4fcbd6d6 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > @@ -1181,12 +1181,13 @@ static struct its_vlpi_map
> > *get_vlpi_map(struct irq_data *d)
> >
> > Âstatic void lpi_write_config(struct irq_data *d, u8 clr, u8 set)
> > Â{
> > -ÂÂÂ struct its_vlpi_map *map = get_vlpi_map(d);
> > ÂÂÂÂ irq_hw_number_t hwirq;
> > ÂÂÂÂ void *va;
> > ÂÂÂÂ u8 *cfg;
> >
> > -ÂÂÂ if (map) {
> > +ÂÂÂ if (irqd_is_forwarded_to_vcpu(d)) {
> > +ÂÂÂÂÂÂÂ struct its_vlpi_map *map = get_vlpi_map(d);
> > +
> > ÂÂÂÂÂÂÂÂ va = page_address(map->vm->vprop_page);
> > ÂÂÂÂÂÂÂÂ hwirq = map->vintid;
>
> Shouldn't we fix get_vlpi_map() instead? Something like (untested):
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index e05673bcd52b..b704214390c0 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1170,13 +1170,14 @@ static void its_send_vclear(struct its_device 
> *dev, u32 event_id)
>  Â */
>  Âstatic struct its_vlpi_map *get_vlpi_map(struct irq_data *d)
>  Â{
> -ÂÂÂ struct its_device *its_dev = irq_data_get_irq_chip_data(d);
> -ÂÂÂ u32 event = its_get_event_id(d);
> +ÂÂÂ if (irqd_is_forwarded_to_vcpu(d)) {
> +ÂÂÂÂÂÂÂ struct its_device *its_dev = irq_data_get_irq_chip_data(d);
> +ÂÂÂÂÂÂÂ u32 event = its_get_event_id(d);
>
> -ÂÂÂ if (!irqd_is_forwarded_to_vcpu(d))
> -ÂÂÂÂÂÂÂ return NULL;
> +ÂÂÂÂÂÂÂ return dev_event_to_vlpi_map(its_dev, event);
> +ÂÂÂ }
>
> -ÂÂÂ return dev_event_to_vlpi_map(its_dev, event);
> +ÂÂÂ return NULL;
>  Â}
>
>  Âstatic void lpi_write_config(struct irq_data *d, u8 clr, u8 set)
>
>
> Or am I missing the actual problem?

No. I also thought about fixing the issue by this way and I'm OK with
both approaches.

> Overall, I'm starting to hate that ->parent hack as it's been the source
> of a number of bugs.

(thankfully it's rarely used and we've so far handled them carefully,
except the lpi_write_config issue in this patch...)

> The main issue is that the VPE hierarchy is missing one level (it has
> no ITS domain, and goes directly from the VPE domain to the low-level
> GIC domain). It means we end-up special-casing things, and that's never
> good...

Yeah, this may comes from the fact that the per-vPE doorbell is not
served by ITS and can be asserted by redistributor directly. And the
special doorbell is programmed together with those normal LPI
(translated from MSI by ITS).


Thanks,
Zenghui