| 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
| ||
|
Message-ID: <mhng-8de39ab4-730a-4ded-a8b5-d50f34d1697b@palmer-si-x1e>
Date: Sun, 15 Sep 2019 10:31:33 -0700 (PDT)
From: Palmer Dabbelt <palmer@...ive.com>
To: maz@...nel.org, Paul Walmsley <paul.walmsley@...ive.com>,
David Johnson <davidj@...ive.com>
CC: Darius Rad <darius@...espec.com>, linux-riscv@...ts.infradead.org,
Paul Walmsley <paul.walmsley@...ive.com>,
linux-kernel@...r.kernel.org, tglx@...utronix.de,
jason@...edaemon.net
Subject: Re: [PATCH] irqchip/sifive-plic: add irq_mask and irq_unmask
On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@...nel.org wrote:
> On Thu, 12 Sep 2019 22:40:34 +0100,
> Darius Rad <darius@...espec.com> wrote:
>
> Hi Darius,
>
>>
>> As per the existing comment, irq_mask and irq_unmask do not need
>> to do anything for the PLIC. However, the functions must exist
>> (the pointers cannot be NULL) as they are not optional, based on
>> the documentation (Documentation/core-api/genericirq.rst) as well
>> as existing usage (e.g., include/linux/irqchip/chained_irq.h).
>>
>> Signed-off-by: Darius Rad <darius@...espec.com>
>> ---
>> drivers/irqchip/irq-sifive-plic.c | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
>> index cf755964f2f8..52d5169f924f 100644
>> --- a/drivers/irqchip/irq-sifive-plic.c
>> +++ b/drivers/irqchip/irq-sifive-plic.c
>> @@ -111,6 +111,13 @@ static void plic_irq_disable(struct irq_data *d)
>> plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
>> }
>>
>> +/*
>> + * There is no need to mask/unmask PLIC interrupts. They are "masked"
>> + * by reading claim and "unmasked" when writing it back.
>> + */
>> +static void plic_irq_mask(struct irq_data *d) { }
>> +static void plic_irq_unmask(struct irq_data *d) { }
>
> This outlines a bigger issue. If your irqchip doesn't require
> mask/unmask, you're probably not using the right interrupt
> flow. Looking at the code, I see you're using handle_simple_irq, which
> is almost universally wrong.
>
> As per the description above, these interrupts should be using the
> fasteoi flow, which is designed for this exact behaviour (the
> interrupt controller knows which interrupt is in flight and doesn't
> require SW to do anything bar signalling the EOI).
>
> Another thing is that mask/unmask tends to be a requirement, while
> enable/disable tends to be optional. There is no hard line here, but
> the expectations are that:
>
> (a) A disabled line can drop interrupts
> (b) A masked line cannot drop interrupts
>
> Depending what the PLIC architecture mandates, you'll need to
> implement one and/or the other. Having just (a) is indicative of a HW
> bug, and I'm not assuming that this is the case. (b) only is pretty
> common, and (a)+(b) has a few adepts. My bet is that it requires (b)
> only.
>
>> +
>> #ifdef CONFIG_SMP
>> static int plic_set_affinity(struct irq_data *d,
>> const struct cpumask *mask_val, bool force)
>> @@ -138,12 +145,10 @@ static int plic_set_affinity(struct irq_data *d,
>>
>> static struct irq_chip plic_chip = {
>> .name = "SiFive PLIC",
>> - /*
>> - * There is no need to mask/unmask PLIC interrupts. They are "masked"
>> - * by reading claim and "unmasked" when writing it back.
>> - */
>> .irq_enable = plic_irq_enable,
>> .irq_disable = plic_irq_disable,
>> + .irq_mask = plic_irq_mask,
>> + .irq_unmask = plic_irq_unmask,
>> #ifdef CONFIG_SMP
>> .irq_set_affinity = plic_set_affinity,
>> #endif
>
> Can you give the following patch a go? It brings the irq flow in line
> with what the HW can do. It is of course fully untested (not even
> compile tested...).
>
> Thanks,
>
> M.
>
> From c0ce33a992ec18f5d3bac7f70de62b1ba2b42090 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@...nel.org>
> Date: Sun, 15 Sep 2019 15:17:45 +0100
> Subject: [PATCH] irqchip/sifive-plic: Switch to fasteoi flow
>
> The SiFive PLIC interrupt controller seems to have all the HW
> features to support the fasteoi flow, but the driver seems to be
> stuck in a distant past. Bring it into the 21st century.
Thanks. We'd gotten these comments during the review process but nobody had
gotten the time to actually fix the issues.
>
> Signed-off-by: Marc Zyngier <maz@...nel.org>
> ---
> drivers/irqchip/irq-sifive-plic.c | 29 +++++++++++++++--------------
> 1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index cf755964f2f8..8fea384d392b 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -97,7 +97,7 @@ static inline void plic_irq_toggle(const struct cpumask *mask,
> }
> }
>
> -static void plic_irq_enable(struct irq_data *d)
> +static void plic_irq_mask(struct irq_data *d)
> {
> unsigned int cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
> cpu_online_mask);
> @@ -106,7 +106,7 @@ static void plic_irq_enable(struct irq_data *d)
> plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
> }
>
> -static void plic_irq_disable(struct irq_data *d)
> +static void plic_irq_unmask(struct irq_data *d)
> {
> plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
> }
> @@ -125,10 +125,8 @@ static int plic_set_affinity(struct irq_data *d,
> if (cpu >= nr_cpu_ids)
> return -EINVAL;
>
> - if (!irqd_irq_disabled(d)) {
> - plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
> - plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
> - }
> + plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
> + plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
>
> irq_data_update_effective_affinity(d, cpumask_of(cpu));
>
> @@ -136,14 +134,18 @@ static int plic_set_affinity(struct irq_data *d,
> }
> #endif
>
> +static void plic_irq_eoi(struct irq_data *d)
> +{
> + struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> +
> + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> +}
> +
> static struct irq_chip plic_chip = {
> .name = "SiFive PLIC",
> - /*
> - * There is no need to mask/unmask PLIC interrupts. They are "masked"
> - * by reading claim and "unmasked" when writing it back.
> - */
> - .irq_enable = plic_irq_enable,
> - .irq_disable = plic_irq_disable,
> + .irq_mask = plic_irq_mask,
> + .irq_unmask = plic_irq_unmask,
> + .irq_eoi = plic_irq_eoi,
> #ifdef CONFIG_SMP
> .irq_set_affinity = plic_set_affinity,
> #endif
> @@ -152,7 +154,7 @@ static struct irq_chip plic_chip = {
> static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> irq_hw_number_t hwirq)
> {
> - irq_set_chip_and_handler(irq, &plic_chip, handle_simple_irq);
> + irq_set_chip_and_handler(irq, &plic_chip, handle_fasteoi_irq);
> irq_set_chip_data(irq, NULL);
> irq_set_noprobe(irq);
> return 0;
> @@ -188,7 +190,6 @@ static void plic_handle_irq(struct pt_regs *regs)
> hwirq);
> else
> generic_handle_irq(irq);
> - writel(hwirq, claim);
> }
> csr_set(sie, SIE_SEIE);
> }
> --
> 2.20.1
>
>
> --
> Jazz is not dead, it just smells funny.
Reviewed-by: Palmer Dabbelt <palmer@...ive.com>
Tested-by: Palmer Dabbelt <palmer@...ive.com> (QEMU Boot)
We should test them on the hardware, but I don't have any with me right now.
David's probably in the best spot to do this, as he's got a setup that does all
the weird interrupt sources (ie, PCIe).
David: do you mind testing this? I've put the patch here:
ssh://gitolite.kernel.org/pub/scm/linux/kernel/git/palmer/linux.git
-b plic-fasteoi
Powered by blists - more mailing lists