linux-kernel - Re: [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER

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: <CAAhSdy1rqZBt5LSgs9yQwewwKXvaS23ou5Ah7Xfu3n7S9sK6RA@mail.gmail.com>
Date:   Fri, 23 Oct 2020 17:12:09 +0530
From:   Anup Patel <anup@...infault.org>
To:     Guo Ren <guoren@...nel.org>
Cc:     Palmer Dabbelt <palmerdabbelt@...gle.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Greentime Hu <greentime.hu@...ive.com>,
        Zong Li <zong.li@...ive.com>,
        Atish Patra <atish.patra@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Marc Zyngier <maz@...nel.org>, wesley@...ive.com,
        Yash Shah <yash.shah@...ive.com>,
        Christoph Hellwig <hch@....de>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org List" <linux-kernel@...r.kernel.org>,
        Guo Ren <guoren@...ux.alibaba.com>
Subject: Re: [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of
 xxx_PER_HART and reg base

On Fri, Oct 23, 2020 at 3:48 PM <guoren@...nel.org> wrote:
>
> From: Guo Ren <guoren@...ux.alibaba.com>
>
> ENABLE and CONTEXT registers contain M & S status for per-hart, so
> ref to the specification the correct definition is double to the
> current value.
>
> The value of hart_base and enable_base should be calculated by real
> physical hartid not software id. Sometimes the CPU node's <reg>
> from dts is not equal to the sequence index.
>
> Signed-off-by: Guo Ren <guoren@...ux.alibaba.com>
> ---
>  drivers/irqchip/irq-sifive-plic.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index eaa3e9f..2e56576 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -44,16 +44,16 @@
>   * Each hart context has a vector of interrupt enable bits associated with it.
>   * There's one bit for each interrupt source.
>   */
> -#define ENABLE_BASE                    0x2000
> -#define     ENABLE_PER_HART            0x80
> +#define ENABLE_BASE                    0x2080
> +#define     ENABLE_PER_HART            0x100
>
>  /*
>   * Each hart context has a set of control registers associated with it.  Right
>   * now there's only two: a source priority threshold over which the hart will
>   * take an interrupt, and a register to claim interrupts.
>   */
> -#define CONTEXT_BASE                   0x200000
> -#define     CONTEXT_PER_HART           0x1000
> +#define CONTEXT_BASE                   0x201000
> +#define     CONTEXT_PER_HART           0x2000
>  #define     CONTEXT_THRESHOLD          0x00
>  #define     CONTEXT_CLAIM              0x04
>
> @@ -358,10 +358,10 @@ static int __init plic_init(struct device_node *node,
>                 cpumask_set_cpu(cpu, &priv->lmask);
>                 handler->present = true;
>                 handler->hart_base =
> -                       priv->regs + CONTEXT_BASE + i * CONTEXT_PER_HART;
> +                       priv->regs + CONTEXT_BASE + hartid * CONTEXT_PER_HART;
>                 raw_spin_lock_init(&handler->enable_lock);
>                 handler->enable_base =
> -                       priv->regs + ENABLE_BASE + i * ENABLE_PER_HART;
> +                       priv->regs + ENABLE_BASE + hartid * ENABLE_PER_HART;
>                 handler->priv = priv;
>  done:
>                 for (hwirq = 1; hwirq <= nr_irqs; hwirq++)
> --
> 2.7.4
>

There is no one-to-one mapping between PLIC context and HARTID. Instead,
we have many-to-one mapping between PLIC contexts and HARTID. In other
words, we have one PLIC context for each interrupt capable mode (i.e.
M/S-mode) of each HART.

For example, on SiFive Unleashed we have 5 HARTs but HARTID=0 has
only M-mode capable of taking interrupts so we have total (1 + 2x4) = 9
PLIC contexts.

I would also like to highlight that this patch is forcing PLIC driver to always
use PLIC S-mode context for each HART which breaks the Linux RISC-V
NoMMU kernel.

There is no issue with the existing defines because these are aligned with
above and latest PLIC spec.
(Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc)

NACK to this patch from my side.

Regards,
Anup