[PATCH 1/2] arm: irqchip: add lock in irqchip_inject_pending

[Peng Fan]

Thinking about core0 is inject SGI to core1, core1 is handling SGI
interrupt.

That means core0 might be in path to enqueue SGI into the pending_irqs
array, core1 might be in path handling SGI and pick one from
pending_irqs array. So need to use lock to protect unqueue, not only
enqueue.

Signed-off-by: Peng Fan <peng...@nxp.com>
---

V1:
The best case is only lock one entry, so no good solution, because
there is possibility that inject fail.

hypervisor/arch/arm-common/irqchip.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/hypervisor/arch/arm-common/irqchip.c b/hypervisor/arch/arm-common/irqchip.c
index 1c881b64..fbaa3099 100644
--- a/hypervisor/arch/arm-common/irqchip.c
+++ b/hypervisor/arch/arm-common/irqchip.c
@@ -279,11 +279,14 @@ void irqchip_inject_pending(void)
struct pending_irqs *pending = &this_cpu_public()->pending_irqs;
u16 irq_id, sender;

+ spin_lock(&pending->lock);
+
while (pending->head != pending->tail) {
irq_id = pending->irqs[pending->head];
sender = pending->sender[pending->head];

if (irqchip.inject_irq(irq_id, sender) == -EBUSY) {
+ spin_unlock(&pending->lock);
/*
* The list registers are full, trigger maintenance
* interrupt and leave.
@@ -295,6 +298,8 @@ void irqchip_inject_pending(void)
pending->head = (pending->head + 1) % MAX_PENDING_IRQS;
}

+ spin_unlock(&pending->lock);
+
/*
* The software interrupt queue is empty - turn off the maintenance
* interrupt.
--
2.16.4

[Peng Fan]

spin_unlock implies memory barrier, no need explicit memory barrier.

Signed-off-by: Peng Fan <peng...@nxp.com>
---

hypervisor/arch/arm-common/irqchip.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hypervisor/arch/arm-common/irqchip.c b/hypervisor/arch/arm-common/irqchip.c
index fbaa3099..eb6258df 100644
--- a/hypervisor/arch/arm-common/irqchip.c
+++ b/hypervisor/arch/arm-common/irqchip.c
@@ -247,13 +247,12 @@ void irqchip_set_pending(struct public_per_cpu *cpu_public, u16 irq_id)
pending->irqs[pending->tail] = irq_id;
pending->sender[pending->tail] = sender;
pending->tail = new_tail;
- /*
- * Make the change to pending_irqs.tail visible before the
- * caller sends SGI_INJECT.
- */
- memory_barrier();
}

+ /*
+ * spin_unlock will make the change to pending_irqs.tail
+ * visible before the caller sends SGI_INJECT.
+ */
spin_unlock(&pending->lock);

/*
--
2.16.4

[Jan Kiszka]

Did you see real corruptions?

The idea behind this was that the injection to the lock-less queue
happens in a way that it won't make changes visible to the consumer that
are inconsistent, hence the barrier in irqchip_set_pending. Looking that
again, though, we possibly need another barrier, right before updating
pending->tail.

Jan

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

[Peng Fan]

> Subject: Re: [PATCH 1/2] arm: irqchip: add lock in irqchip_inject_pending

No. just code inspection currently. We met some SGI inject return -EBUSY,
so go through the code, and think this piece code needs a lock.

>
> The idea behind this was that the injection to the lock-less queue happens in
> a way that it won't make changes visible to the consumer that are
> inconsistent, hence the barrier in irqchip_set_pending. Looking that again,
> though, we possibly need another barrier, right before updating
> pending->tail.

Barrier could not prohibit enqueue/unqueue contention.
enqueue will check head, unqueue will update head.

Thanks,
Peng.

[Jan Kiszka]

Some topic as with lockless enqueuing: We need a barrier to shield the
readout of the head entry from the update of pending->head. So, we are
definitely lacking barriers here, but I don't think we need the spinlock
between producer and consumer because there is only one consumer.

[Peng Fan]

Lock-free should be possible, let me work out a non-lock solution.

Regards,
Peng.

[Jan Kiszka]

This is what comes to my mind so far, but please re-check carefully:

diff --git a/hypervisor/arch/arm-common/irqchip.c b/hypervisor/arch/arm-common/irqchip.c
index 1c881b64..a83cd81d 100644
--- a/hypervisor/arch/arm-common/irqchip.c
+++ b/hypervisor/arch/arm-common/irqchip.c
@@ -246,12 +246,12 @@ void irqchip_set_pending(struct public_per_cpu *cpu_public, u16 irq_id)
if (new_tail != pending->head) {

pending->irqs[pending->tail] = irq_id;
pending->sender[pending->tail] = sender;

- pending->tail = new_tail;

/*
- * Make the change to pending_irqs.tail visible before the
- * caller sends SGI_INJECT.

+ * Make the entry content visible before updating the tail
+ * index.
*/
memory_barrier();
+ pending->tail = new_tail;
}

spin_unlock(&pending->lock);
@@ -264,6 +264,12 @@ void irqchip_set_pending(struct public_per_cpu *cpu_public, u16 irq_id)
if (local_injection) {
irqchip.enable_maint_irq(true);
} else {
+ /*
+ * Make the change to pending_irqs.tail visible before the
+ * caller sends SGI_INJECT.
+ */
+ memory_barrier();
+
sgi.targets = irqchip_get_cpu_target(cpu_public->cpu_id);
sgi.cluster_id =
irqchip_get_cluster_target(cpu_public->cpu_id);
@@ -292,6 +298,12 @@ void irqchip_inject_pending(void)
return;
}

+ /*
+ * Ensure that the entry was read befor updating the head
+ * index.
+ */
+ memory_barrier();
+

pending->head = (pending->head + 1) % MAX_PENDING_IRQS;
}

Thanks for bringing this up!

[Peng Fan]

Hi Jan,

The spin_unlock implies memory barrier. I think no need memory_barrier here.

> }
>
> spin_unlock(&pending->lock);
> @@ -264,6 +264,12 @@ void irqchip_set_pending(struct public_per_cpu
> *cpu_public, u16 irq_id)
> if (local_injection) {
> irqchip.enable_maint_irq(true);
> } else {
> + /*
> + * Make the change to pending_irqs.tail visible before the
> + * caller sends SGI_INJECT.
> + */
> + memory_barrier();

Not needed, see above, spin_unlock already done this.

> +
> sgi.targets = irqchip_get_cpu_target(cpu_public->cpu_id);
> sgi.cluster_id =
> irqchip_get_cluster_target(cpu_public->cpu_id);
> @@ -292,6 +298,12 @@ void irqchip_inject_pending(void)
> return;
> }
>
> + /*
> + * Ensure that the entry was read befor updating the head
> + * index.
> + */
> + memory_barrier();

No need. The index update will not be speculative before reading pending->head.

> +
> pending->head = (pending->head + 1) % MAX_PENDING_IRQS;

Need a barrier here, to make update visible to producer.

> }

My version,

diff --git a/hypervisor/arch/arm-common/irqchip.c b/hypervisor/arch/arm-common/irqchip.c
index 1c881b64..5abf1c37 100644
--- a/hypervisor/arch/arm-common/irqchip.c
+++ b/hypervisor/arch/arm-common/irqchip.c
@@ -247,13 +247,12 @@ void irqchip_set_pending(struct public_per_cpu *cpu_public, u16 irq_id)

pending->irqs[pending->tail] = irq_id;
pending->sender[pending->tail] = sender;

pending->tail = new_tail;
- /*

- * Make the change to pending_irqs.tail visible before the
- * caller sends SGI_INJECT.

- */
- memory_barrier();
}

+ /*

+ * spin_unlock will make the change to pending_irqs.tail and
+ * entry content visible before the caller sends SGI_INJECT.
+ */
spin_unlock(&pending->lock);

/*
@@ -293,6 +292,9 @@ void irqchip_inject_pending(void)

}

pending->head = (pending->head + 1) % MAX_PENDING_IRQS;
+

+ /* Make the change to pending->head visible to enqueue. */
+ memory_barrier();
}

/*

Thanks,
Peng.

[Jan Kiszka]

We /might/ be fine here for the archs we support, but Linux is more
strict:

"(2) RELEASE operation implication:

Memory operations issued before the RELEASE will be completed before the
RELEASE operation has completed.

Memory operations issued after the RELEASE may be completed before the
RELEASE operation has completed."

Now, the x86 is ordered anyway, ARMv7 indeed has the same barrier in the
unload as in the memory_barrier(). However, ARM64 is not that clear to me.

Jan

[Peng Fan]

ARM64 STLRH:
Store-Release Register Halfword stores a halfword from a 32-bit register to a
memory location. The instruction also has memory ordering semantics as
described in Load-Acquire, Load-AcquirePC, and Store-Release on page B2-108.

DDI0487D version, B2-108:
The Load-Acquire, Load-AcquirePC, and Store-Release instructions can remove
the requirement to use the explicit DMB instruction.

Hope this is clear.

Thanks,
Peng.

> --
> You received this message because you are subscribed to the Google Groups
> "Jailhouse" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to jailhouse-de...@googlegroups.com.
> To view this discussion on the web visit
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups
> .google.com%2Fd%2Fmsgid%2Fjailhouse-dev%2Fbef9b0b0-3bc0-4982-3b3d-
> aa829ad6ceb5%2540siemens.com&amp;data=02%7C01%7Cpeng.fan%40nxp
> .com%7C10ff76f9bdad430d02d208d77959a6aa%7C686ea1d3bc2b4c6fa92cd
> 99c5c301635%7C0%7C0%7C637111298262016631&amp;sdata=z9GG03UgG
> tQUqoaRzSrZip%2BA0pM7mxUM5YrAJmJ6QPI%3D&amp;reserved=0.

[Jan Kiszka]

OK, let's document this in our spinlock implementations, to ensure that
potential future ones (and there will be at least one further arch added
in to mid-term) will read that and follow that semantic. Then we can go
away without the explicit barriers in my diff.

[Peng Fan]

Yes. Add a comment to arm64 spin_lock/unlock code is enough?

Then we can go away
> without the explicit barriers in my diff.

Please review the following diff the same one posted earlier.

diff --git a/hypervisor/arch/arm-common/irqchip.c b/hypervisor/arch/arm-common/irqchip.c
index 1c881b64..5abf1c37 100644
--- a/hypervisor/arch/arm-common/irqchip.c
+++ b/hypervisor/arch/arm-common/irqchip.c
@@ -247,13 +247,12 @@ void irqchip_set_pending(struct public_per_cpu *cpu_public, u16 irq_id)

pending->irqs[pending->tail] = irq_id;
pending->sender[pending->tail] = sender;

pending->tail = new_tail;
- /*

- * Make the change to pending_irqs.tail visible before the
- * caller sends SGI_INJECT.

- */
- memory_barrier();
}

+ /*
+ * spin_unlock will make the change to pending_irqs.tail and
+ * entry content visible before the caller sends SGI_INJECT.
+ */
spin_unlock(&pending->lock);

/*

@@ -293,6 +292,9 @@ void irqchip_inject_pending(void)

}

pending->head = (pending->head + 1) % MAX_PENDING_IRQS;
+

+ /* Make the change to pending->head visible to enqueue. */
+ memory_barrier();
}

/*

Thanks,
Peng.
>