Received-SPF: pass (google.com: domain of linux-kernel+bounces-75191-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: =?utf-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>
To: Anup Patel <apatel@ventanamicro.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>, Paul Walmsley
 <paul.walmsley@sifive.com>, Thomas Gleixner <tglx@linutronix.de>, Rob
 Herring <robh+dt@kernel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@linaro.org>, Frank Rowand
 <frowand.list@gmail.com>, Conor Dooley <conor+dt@kernel.org>,
 devicetree@vger.kernel.org, Saravana Kannan <saravanak@google.com>, Marc
 Zyngier <maz@kernel.org>, Anup Patel <anup@brainfault.org>,
 linux-kernel@vger.kernel.org, Atish Patra <atishp@atishpatra.org>,
 linux-riscv@lists.infradead.org, linux-arm-kernel@lists.infradead.org,
 Andrew Jones <ajones@ventanamicro.com>
Subject: Re: [PATCH v13 06/13] irqchip: Add RISC-V incoming MSI controller
 early driver
In-Reply-To: <CAK9=C2WXR49KZg3rstChqAHda+hUhPm3AEo6o2jh0NM3kvoSUA@mail.gmail.com>
References: <20240220060718.823229-1-apatel@ventanamicro.com>
 <20240220060718.823229-7-apatel@ventanamicro.com>
 <87frxnfj3p.fsf@all.your.base.are.belong.to.us>
 <CAK9=C2Xnzg3KAVETXN+ZGLWhVtaJuU4uXs3WH2ZondkBJMHFcA@mail.gmail.com>
 <874je2yqn9.fsf@all.your.base.are.belong.to.us>
 <CAK9=C2WXR49KZg3rstChqAHda+hUhPm3AEo6o2jh0NM3kvoSUA@mail.gmail.com>
Date: Wed, 21 Feb 2024 18:22:40 +0100
Message-ID: <87frxl3f6n.fsf@all.your.base.are.belong.to.us>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Anup Patel <apatel@ventanamicro.com> writes:

> On Wed, Feb 21, 2024 at 5:29=E2=80=AFPM Bj=C3=B6rn T=C3=B6pel <bjorn@kern=
el.org> wrote:
>>
>> Anup Patel <apatel@ventanamicro.com> writes:
>>
>> >> > +void imsic_vector_mask(struct imsic_vector *vec)
>> >> > +{
>> >> > +     struct imsic_local_priv *lpriv;
>> >> > +
>> >> > +     lpriv =3D per_cpu_ptr(imsic->lpriv, vec->cpu);
>> >> > +     if (WARN_ON(&lpriv->vectors[vec->local_id] !=3D vec))
>> >> > +             return;
>> >> > +
>> >> > +     /*
>> >> > +      * This function is called through Linux irq subsystem with
>> >> > +      * irqs disabled so no need to save/restore irq flags.
>> >> > +      */
>> >> > +
>> >> > +     raw_spin_lock(&lpriv->lock);
>> >> > +
>> >> > +     vec->enable =3D false;
>> >> > +     bitmap_set(lpriv->dirty_bitmap, vec->local_id, 1);
>> >> > +     __imsic_remote_sync(lpriv, vec->cpu);
>> >> > +
>> >> > +     raw_spin_unlock(&lpriv->lock);
>> >> > +}
>> >>
>> >> Really nice that you're using a timer for the vector affinity change,
>> >> and got rid of the special/weird IMSIC/sync IPI. Can you really use a
>> >> timer for mask/unmask? That makes the mask/unmask operation
>> >> asynchronous!
>> >>
>> >> That was what I was trying to get though with this comment:
>> >> https://lore.kernel.org/linux-riscv/87sf24mo1g.fsf@all.your.base.are.=
belong.to.us/
>> >>
>> >> Also, using the smp_* IPI functions, you can pass arguments, so you
>> >> don't need the dirty_bitmap tracking the changes.
>> >
>> > The mask/unmask operations are called with irqs disabled so if
>> > CPU X does synchronous IPI to another CPU Y from mask/unmask
>> > operation then while CPU X is waiting for IPI to complete it cannot
>> > receive IPI from other CPUs which can lead to crashes and stalls.
>> >
>> > In general, we should not do any block/busy-wait work in
>> > mask/unmask operation of an irqchip driver.
>>
>> Hmm, OK. Still, a bit odd that when the .irq_mask callback return, the
>> masking is not actually completed.
>>
>> 1. CPU 0 tries to mask an interrupt tried to CPU 1.
>> 2. The timer is queued on CPU 1.
>> 3. The call irq_mask returns on CPU 0
>> 4. ...the irq is masked at some future point, determined by the callback
>>    at CPU 1
>>
>> Is that the expected outcome?
>
> Yes, that's right.
>
>>
>> There are .irq_mask implementation that does seem to go at length
>> (blocking) to perform the mask, e.g.: gic_mask_irq() which calls
>> gic_{re,}dist_wait_for_rwp that have sleep/retry loops. The GIC3 ITS
>> code has similar things going on.
>
> The gic_{re,}dist_wait_for_rwp() polls on a HW register for completion
> which will certainly complete in a predictable time whereas waiting
> for IPI to be executed by another CPU is not predictable and fragile.
>
>>
>> I'm not saying you're wrong, I'm just trying to wrap my head around the
>> masking semantics.
>>
>> > The AIA IMSIC spec allows setting ID pending bit using MSI write
>> > irrespective whether ID is enabled or not but the interrupt will be
>> > taken only after ID is enabled. In other words, there will be no
>> > loss of interrupt with delayed mask/unmask using async IPI or
>> > lazy timer.
>>
>> No loss, but we might *get* an interrupt when we explicitly asked not to
>> get any. Maybe that's ok?
>>
>
> The delayed spurious interrupt after masking is avoided by additional
> masking at the source of interrupt. For wired-to-MSI interrupts, we have
> additional masking on the APLIC MSI-mode. For PCI MSI interrupts, we
> have additional masking at PCI device level using pci_msi_mask_irq().

Thanks for the clarifications, Anup! Much appreciated!