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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8dfeb7782d7ffaa1f107a8e4aca10840@kernel.org>
Date:   Fri, 31 Jul 2020 12:27:35 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Valentin Schneider <valentin.schneider@....com>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>
Subject: Re: [PATCH v2 2/2] irqchip/gic-v2, v3: Prevent SW resends entirely

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