linux-kernel - Re: [PATCH] irqchip/riscv-imsic: Support allocating multiple msi-x vectors

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] [day] [month] [year] [list]
Message-ID: <CAK9=C2W1M8bXE2mLW1VUeDTKuqoiLy=PfdGxP5jxpGm8P3EGqQ@mail.gmail.com>
Date: Sat, 4 Jan 2025 13:19:07 +0530
From: Anup Patel <apatel@...tanamicro.com>
To: Xu Lu <luxu.kernel@...edance.com>
Cc: anup@...infault.org, tglx@...utronix.de, paul.walmsley@...ive.com, 
	lihangjing@...edance.com, xieyongji@...edance.com, 
	guojinhui.liam@...edance.com, linux-riscv@...ts.infradead.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] irqchip/riscv-imsic: Support allocating multiple msi-x vectors

On Fri, Jan 3, 2025 at 6:30 PM Xu Lu <luxu.kernel@...edance.com> wrote:
>
> This commit modifies alloc and free callback of imsic_base_domain to
> support allocating and freeing multiple msi-x vectors at once.

There are two separate concepts here: "multiple MSI-X vectors" and
"multi MSI vector".

For multiple MSI-X vectors, we don't need to do anything special since
each MSI-X vector will be a separate MSI message (aka MSI address
and MSI data). We have tested the IMSIC driver for PCIe devices with
multiple MSI-X vectors and it works fine because the PCI framework
sets up a separate MSI-X descriptor for each MSI-X vector [1].

For multi MSI, there will be a single MSI address and multiple consecutive
MSI data values for multiple interrupts. This is not properly supported
by the AIA v1.0 specification because changing the MSI address of a
single interrupt would change the MSI address of all interrupts. The
currently the Linux IMSIC driver does not support multi MSI. In fact,
there was a partial multi MSI support in the v11 of Linux AIA driver
series [2] which was later dropped due to past reasoning provided
in the past by Thomas [3].

AFAIK, most PCIe devices use MSI-X and even Linux arch/x86 does
not support multi MSI so we really need a strong reason to support
multi MSI in future AIA specification or Linux IMSIC driver.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/msi/msi.c?h=v6.13-rc5#n622
[2] https://www.spinics.net/lists/devicetree/msg643772.html
[3] https://lore.kernel.org/all/877d7yhve7.ffs@tglx/

>
> Signed-off-by: Xu Lu <luxu.kernel@...edance.com>
> ---
>  drivers/irqchip/irq-riscv-imsic-platform.c | 54 +++++++++++++---------
>  1 file changed, 32 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/irqchip/irq-riscv-imsic-platform.c b/drivers/irqchip/irq-riscv-imsic-platform.c
> index c708780e8760..d0ea0eb22e2e 100644
> --- a/drivers/irqchip/irq-riscv-imsic-platform.c
> +++ b/drivers/irqchip/irq-riscv-imsic-platform.c
> @@ -141,35 +141,45 @@ static struct irq_chip imsic_irq_base_chip = {
>                                   IRQCHIP_MASK_ON_SUSPEND,
>  };
>
> +static void imsic_irq_domain_free(struct irq_domain *domain, unsigned int virq,
> +                                 unsigned int nr_irqs)
> +{
> +       struct irq_data *d;
> +       int i;
> +
> +       for (i = 0; i < nr_irqs; i++) {
> +               d = irq_domain_get_irq_data(domain, virq + i);
> +               imsic_vector_free(irq_data_get_irq_chip_data(d));
> +       }
> +       irq_domain_free_irqs_top(domain, virq, nr_irqs);
> +}
> +
>  static int imsic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>                                   unsigned int nr_irqs, void *args)
>  {
>         struct imsic_vector *vec;
> -
> -       /* Multi-MSI is not supported yet. */
> -       if (nr_irqs > 1)
> -               return -EOPNOTSUPP;

I don't see why we need to handle "nr_irqs > 1" over here because
PCI framework already creates a separate MSI-X descriptor for
each MSI-X vector.

If we do support "nr_irqs > 1" over here then we still need a check
which prevents multi MSI (as explained above).

> -
> -       vec = imsic_vector_alloc(virq, cpu_online_mask);
> -       if (!vec)
> -               return -ENOSPC;
> -
> -       irq_domain_set_info(domain, virq, virq, &imsic_irq_base_chip, vec,
> -                           handle_simple_irq, NULL, NULL);
> -       irq_set_noprobe(virq);
> -       irq_set_affinity(virq, cpu_online_mask);
> -       irq_data_update_effective_affinity(irq_get_irq_data(virq), cpumask_of(vec->cpu));
> +       int i, err;
> +
> +       for (i = 0; i < nr_irqs; i++) {
> +               vec = imsic_vector_alloc(virq + i, cpu_online_mask);
> +               if (!vec) {
> +                       err = -ENOSPC;
> +                       goto error;
> +               }
> +
> +               irq_domain_set_info(domain, virq + i, virq + i, &imsic_irq_base_chip,
> +                                   vec, handle_simple_irq, NULL, NULL);
> +               irq_set_noprobe(virq + i);
> +               irq_set_affinity(virq + i, cpu_online_mask);
> +               irq_data_update_effective_affinity(irq_get_irq_data(virq + i),
> +                                                  cpumask_of(vec->cpu));
> +       }
>
>         return 0;
> -}
> -
> -static void imsic_irq_domain_free(struct irq_domain *domain, unsigned int virq,
> -                                 unsigned int nr_irqs)
> -{
> -       struct irq_data *d = irq_domain_get_irq_data(domain, virq);
>
> -       imsic_vector_free(irq_data_get_irq_chip_data(d));
> -       irq_domain_free_irqs_parent(domain, virq, nr_irqs);
> +error:
> +       imsic_irq_domain_free(domain, virq, i);
> +       return err;
>  }
>
>  static int imsic_irq_domain_select(struct irq_domain *domain, struct irq_fwspec *fwspec,
> --
> 2.20.1
>
>

Regards,
Anup