Hi Hector,

On Mon, 22 Feb 2021 19:35:03 +0000,
Hector Martin [off-list ref] wrote:

On 16/02/2021 03.09, Marc Zyngier wrote:

quoted

On Mon, 15 Feb 2021 12:17:03 +0000,
Hector Martin [off-list ref] wrote:

quoted

This patch introduces basic UP irqchip support, without SMP/IPI support.

This last comment seems outdated now.

Heh, I forgot to reword this one. Thanks :)

quoted

quoted

+config APPLE_AIC
+	bool "Apple Interrupt Controller (AIC)"
+	depends on ARM64
+	default ARCH_APPLE
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY

arm64 selects GENERIC_IRQ_IPI, which selects IRQ_DOMAIN_HIERARCHY,
which selects IRQ_DOMAIN. So these two lines are superfluous.

Ack, removing these for v3.

quoted

quoted

+ * In addition, this driver also handles FIQs, as these are routed to the same IRQ vector. These
+ * are used for Fast IPIs (TODO), the ARMv8 timer IRQs, and performance counters (TODO).
+ *

nit: A bit of comment formatting could be helpful.

Wrapped this to 80 columns for v3.

quoted

quoted

+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/cpuhotplug.h>
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/arm-gic-v3.h>

I'd rather you move the ICH_HCR_* definitions to sysreg.h rather than
including the GICv3 stuff. They are only there for historical reasons
(such as supporting KVM on 32bit systems), none of which apply anymore.

Just ICH_HCR, or should I bring all of the ICH_ and ICC_ defines along
with it?

Just ICH_HCR for now would be fine.

quoted

quoted

+	aic_ic_write(ic, AIC_TARGET_CPU + hwirq * 4, BIT(cpu));
+	irq_data_update_effective_affinity(d, cpumask_of(cpu));

It is fine to pick a single CPU out of the whole affinity set, but you
should tell the kernel that this is the case (irqd_set_single_target()).

quoted

quoted

+
+	irq_set_status_flags(irq, IRQ_LEVEL);

I'm definitely not keen on this override, and the trigger information
should be the one coming from the DT, which is already set for you.
It'd probably be useful to provide an irq_set_type() callback that
returns an error when fed an unsupported trigger.

quoted

quoted

+	irq_set_noprobe(irq);

This seems to be cargo-culted, and I don't believe this is necessary.

quoted

quoted

+static const struct irq_domain_ops aic_irq_domain_ops = {
+	.map = aic_irq_domain_map,
+	.unmap = aic_irq_domain_unmap,
+	.xlate = aic_irq_domain_xlate,
+};

You are mixing two APIs: the older OF-specific one, and the newer one
that uses fwnode_handle for hierarchical support. That's OK for older
drivers that were forcefully converted to using generic IPIs, but as
this is a brand new driver, I'd rather it consistently used the new
API. See a proposed rework at [1] (compile tested only).

Applying your fixups for these, thanks! :)

My pleasure!

quoted

quoted

+	atomic_and(~irq_bit, &aic_vipi_mask[this_cpu]);

atomic_andnot()?

quoted

+
+	if (!atomic_read(&aic_vipi_mask[this_cpu]))
+		aic_ic_write(ic, AIC_IPI_MASK_SET, AIC_IPI_OTHER);

This is odd. It means that you still perform the MMIO write if the bit
was already clear. I think this could be written as:

	u32 val;
	val = atomic_fetch_andnot(irq_bit, &aic_vipi_mask[this_cpu]);
	if (val && !(val & ~irq_bit))
		aic_ic_write();

quoted

	val  = atomic_fetch_or(irq_bit, &aic_vipi_mask[this_cpu]);
	if (!val)
		aic_ic_write();

This makes more sense to avoid the redundant MMIO writes. I need to
get more familiar with all the available atomic ops... lots of useful
stuff in there I didn't know about.

quoted

quoted

+	for_each_cpu(cpu, mask) {
+		if (atomic_read(&aic_vipi_mask[cpu]) & irq_bit) {
+			atomic_or(irq_bit, &aic_vipi_flag[cpu]);
+			send |= AIC_IPI_SEND_CPU(cpu);

That's really odd. A masked IPI should be made pending, and delivered
on unmask. I think this all works because we never mask individual
IPIs, as this would otherwise drop interrupts on the floor.

I wasn't really sure whether IPIs are supposed to end up pending like
that; indeed if that's how it's supposed to work, then I also need
logic at mask/unmask time to fire off any pending IPIs. I'll do it
like that for v3.

Yes, unmask() needs to release pending IPIs.

Now I wonder how other drivers do it... I'm guessing this never gets
tested, since the IPI code only exercises a fraction of the irq
features...

In most cases, the HW does it for you, fortunately. A masked IPI that
is made pending stays pending until unmasked. On the GIC, the SGIs
(which is the interrupt type we use for IPIs) are just dealt with like
any other interrupt, and are subject to the same life cycle. I think
this is the first SW-based IPI we have on arm64.

quoted

quoted

+static void aic_handle_ipi(struct pt_regs *regs)
+{
+	int this_cpu = smp_processor_id();
+	int i;
+	unsigned long firing;
+
+	aic_ic_write(aic_irqc, AIC_IPI_ACK, AIC_IPI_OTHER);
+
+	/*
+	 * Ensure that we've received and acked the IPI before we load the vIPI
+	 * flags. This pairs with the second wmb() above.
+	 */
+	mb();

I don't get your ordering here.

If you are trying to order against something that has happened on
another CPU (which is pretty likely in the case of an IPI), why isn't
this a smp_mb_before_atomic() (and conversely smp_mb_after_atomic() in
aic_ipi_send_mask())?

Although this looks to me like a good case for _acquire/_release
semantics.

This is trying to order the atomic ops with the IPI IRQ itself, in
particular the ACK in the preceding line. If they execute in reverse
order (or more precisely if the ACK takes effect after the xchg), this
happens and we lose an IPI:

CPU1              CPU2
set vIPI #0
fire IPI
                  IPI IRQ
                  read EVENT
                  / xchg vIPI
set vIPI #1       X
fire IPI          |
                  \ ACK IPI

The converse race can also happen in the CPU1 path, of course, if the
IPI ends up fired before the vIPI is set, hence the barrier there.

I'm going to be brave and suggest something. Will can chime in and
explain why I'm totally wrong! :D

I *think* this scenario should be solved with:

- sender:
	// virtually signal the remote CPU, making sure this RMW
	// is ordered after all the previous writes
	atomic_or_release(irq_bit, &vipi[target]);
	// Signal the IPI
	writel(...);

  although I think the _release is superfluous given that the writel()
  is a pretty heavy hammer already. You may need to rejig this to
  account for the loop

- receiver
        // Ack the interrupt, all further memory accesses are ordered
        // after it
	irq = readl();
	// Order all further loads after this
	ipis = atomic_xchg_acquire(&vipi[me], 0);
	// handle all interrupts described in ipis, which may include
	// more than a single source
	handle_ipis(ipis);

  Here, we may end-up with multiple IPIs (#0 and #1 in your
  example). It doesn't really matter, and the only ill effect is a
  potential spurious IPI if we have consumed more than a single one.

What I'm not sure is how the smp_ ops order with regard to
writel_relaxed. It seems like mb() is dsb(sy) and smp_mb() is dmb(ish)
and the atomic versions just default to aliasing smp_mb()). An
inner-sharable dmb doesn't sound like it would safely satisfy this
requirement, as MMIO out to AIC is involved (and we don't know if it's
in the inner sharable domain or not for these purposes). Since the
MMIO is nGnRnE, I would expect that a dsb(sy) would satisfy this
requirement, as then the write op really shouldn't complete until it
has taken effect in AIC.

In the example above, I used a non-relaxed write, as it guarantees
that it is ordered after any DMA agent that can access the data
ordered before (and I assume that if DMA can see it, another CPU can
as well).

I'm not convinced that the DSB(SY) solves anything here, *unless*
there are some other rules specific to the AIC. DSB doesn't guarantee
that the device has actually changed state as a consequence of the
MMIO access. If I remember well, you actually need a read from the
same device to ensure the state has been changed. But after all, a
state change isn't what you're after, only ordering.

quoted

quoted

+	/*
+	 * Make sure the kernel's idea of logical CPU order is the same as AIC's
+	 * If we ever end up with a mismatch here, we will have to introduce
+	 * a mapping table similar to what other irqchip drivers do.
+	 */
+	WARN_ON(aic_ic_read(aic_irqc, AIC_WHOAMI) != smp_processor_id());

This is unlikely to work as soon as you get kexec up and running. You
may not have to worry about this for some time...

Ah, can kexec randomly shuffle CPUs around?

Not completely randomly ;-). kexec can execute on any CPU, and all the
others will be shut down. When that CPU enters the secondary kernel,
it will be CPU0, no matter what it was in the previous instance.

The solution here is obvious, but at this point I'm more keen on
punting this to a future patch instead of introducing more complexity
into the initial series; gotta leave behind some bugs to fix later
:)

I think that's fine for now. kexec is a long way away (we need to
solve the firmware story first), so keeping it on the back burner
seems like a sensible approach.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel