Message-ID: <87h6ily53k.fsf@all.your.base.are.belong.to.us>
Date: Tue, 06 Feb 2024 16:39:27 +0100
From: Björn Töpel <bjorn@...nel.org>
To: Anup Patel <apatel@...tanamicro.com>, Palmer Dabbelt
 <palmer@...belt.com>, Paul Walmsley <paul.walmsley@...ive.com>, Thomas
 Gleixner <tglx@...utronix.de>, Rob Herring <robh+dt@...nel.org>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Frank Rowand
 <frowand.list@...il.com>, Conor Dooley <conor+dt@...nel.org>
Cc: Anup Patel <apatel@...tanamicro.com>, devicetree@...r.kernel.org,
 Saravana Kannan <saravanak@...gle.com>, Marc Zyngier <maz@...nel.org>,
 Anup Patel <anup@...infault.org>, linux-kernel@...r.kernel.org, Atish
 Patra <atishp@...shpatra.org>, linux-riscv@...ts.infradead.org,
 linux-arm-kernel@...ts.infradead.org, Andrew Jones
 <ajones@...tanamicro.com>
Subject: Re: [PATCH v12 00/25] Linux RISC-V AIA Support

Hi Anup,

Anup Patel <apatel@...tanamicro.com> writes:

> The RISC-V AIA specification is ratified as-per the RISC-V international
> process. The latest ratified AIA specifcation can be found at:
> https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf
>
> At a high-level, the AIA specification adds three things:
> 1) AIA CSRs
>    - Improved local interrupt support
> 2) Incoming Message Signaled Interrupt Controller (IMSIC)
>    - Per-HART MSI controller
>    - Support MSI virtualization
>    - Support IPI along with virtualization
> 3) Advanced Platform-Level Interrupt Controller (APLIC)
>    - Wired interrupt controller
>    - In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
>    - In Direct-mode, injects external interrupts directly into HARTs
>
> For an overview of the AIA specification, refer the AIA virtualization
> talk at KVM Forum 2022:
> https://static.sched.com/hosted_files/kvmforum2022/a1/AIA_Virtualization_in_KVM_RISCV_final.pdf
> https://www.youtube.com/watch?v=r071dL8Z0yo

Thank you for continuing to work on this series! I like this
direction of the series!

TL;DR: I think we can get rid of most of the id/householding data
structures, except for the irq matrix.

Most of my comments are more of a design/overview nature, so I'll
comment here in the cover letter.

I took the series for a spin with and it with Alex' ftrace fix it,
passes all my tests nicely!

Now some thoughts/comments (I'm coming from the x86 side of things!):

id/enable-tracking: There are a lot of different id/enabled tracking
with corresponding locks, where there's IMO overlap with what the
matrix provides.

Let's start with struct imsic_priv:

   | /* Dummy HW interrupt numbers */
   | unsigned int nr_hwirqs;
   | raw_spinlock_t hwirqs_lock;
   | unsigned long *hwirqs_used_bitmap;

These are used to for the domain routing (hwirq -> desc/virq), and not
needed. Just use the same id as virq (at allocation time), and get rid
of these data structures/corresponding functions. The lookup in the
interrupt handler via imsic_local_priv.vectors doesn't care about
hwirq. This is what x86 does... The imsic_vector roughly corresponds
to apic_chip_data (nit: imsic_vector could have the chip_data suffix
as well, at least it would have helped me!)

Moving/affinity changes. The moving of a vector to another CPU
currently involves:

1. Allocate a new vector from the matrix
2. Disable/enable the corresponding per-cpu ids_enabled_bitmap (nested
   spinlocks)
3. Trigger two IPIs to apply the bitmap
4. On each CPU target (imsic_local_sync()) loop the bitmap and flip
   all bits, and potentially rearm

This seems a bit heavy-weight: Why are you explicitly setting/clearing
all the bits in a loop at the local sync?

x86 does it a bit differently (more lazily): The chip_data has
prev_{cpu,vector}/move_in_progress fields, and keep both vectors
enabled until there's an interrupt on the new vector, and then the old
one is cleaned (irq_complete_move()).

Further; When it's time to remove the old vector, x86 doesn't trigger
an IPI on the disabling side, but queues a cleanup job on a per-cpu
list and triggers a timeout. So, the per-cpu chip_data (per-cpu
"vectors" in your series) can reside in two places during the transit.

I wonder if this clean up is less intrusive, and you just need to
perform what's in the per-list instead of dealing with the
ids_enabled_bitmap? Maybe we can even remove that bitmap as well. The
chip_data/desc has that information. This would mean that
imsic_local_priv() would only have the local vectors (chip_data), and
a cleanup list/timer.

My general comment is that instead of having these global id-tracking
structures, use the matrix together with some desc/chip_data local
data, which should be sufficient.

Random thought: Do we need to explicitly disable (csr) the vector,
when we're changing the affinity? What if we just leave it enabled,
and only when mask/unmask is performed it's actually explicitly masked
(writes to the csr)?

Missing features (which can be added later):
* Reservation mode/activate support (allocate many MSI, but only
  request/activate a subset)
* Handle managed interrupts
* There might be some irqd flags are missing, which mostly cpuhp care
  about (e.g. irqd_*_single_target())...

Finally; Given that the APLIC requires a lot more patches, depending
on how the review process moves on -- maybe the IMSIC side could go as
a separate series?


Cheers,
Björn

Powered by blists - more mailing lists