Hi Jan,
I just accidentally came across commit 05c1d67e0a79 ("arm: Remove cpuid
from pending_irq") from last year.
Seems like there already has been a preparation for the feature I'm now
reintroducing. Can you remember why you removed the code instead of
providing the correct CPU ID?
Ralf
On 09/13/2017 04:29 PM, Ralf Ramsauer wrote:
> When receiving an SGI on a GICv2 system, the receiving CPU expects the
> caller's CPU ID in the IAR. Jailhouse did not forward the caller's CPU
> ID, so cells might erroneously assume that the calling CPU is (always)
> CPU0.
>
> GICv3+ doesn't support this feature.
>
> Let's fix this in two steps:
> - expand irqchip_set_pending() by a u16 caller parameter that is only
> respected in case of SGIs
> - change the data type of pending_irqs from u16 to u32 to be able to
> encode the calling interface in the upper bits
>
> Signed-off-by: Ralf Ramsauer <[email protected]>
> ---
> hypervisor/arch/arm-common/control.c | 2 +-
> hypervisor/arch/arm-common/gic-v2.c | 4 +++-
> hypervisor/arch/arm-common/include/asm/irqchip.h | 4 ++--
> hypervisor/arch/arm-common/irqchip.c | 22 ++++++++++++++--------
> hypervisor/arch/arm-common/ivshmem.c | 2 +-
> hypervisor/arch/arm/gic-v3.c | 5 ++++-
> hypervisor/arch/arm/include/asm/percpu.h | 4 +++-
> hypervisor/arch/arm64/include/asm/percpu.h | 4 +++-
> 8 files changed, 31 insertions(+), 16 deletions(-)
>
> diff --git a/hypervisor/arch/arm-common/control.c
> b/hypervisor/arch/arm-common/control.c
> index 4d3abd25..8aa39028 100644
> --- a/hypervisor/arch/arm-common/control.c
> +++ b/hypervisor/arch/arm-common/control.c
> @@ -188,7 +188,7 @@ bool arch_handle_phys_irq(struct per_cpu *cpu_data, u32
> irqn,
> }
>
> cpu_data->stats[JAILHOUSE_CPU_STAT_VMEXITS_VIRQ] += count_event;
> - irqchip_set_pending(cpu_data, irqn);
> + irqchip_set_pending(cpu_data, irqn, -1);
>
> return false;
> }
> diff --git a/hypervisor/arch/arm-common/gic-v2.c
> b/hypervisor/arch/arm-common/gic-v2.c
> index 1aec2a2f..7255cd50 100644
> --- a/hypervisor/arch/arm-common/gic-v2.c
> +++ b/hypervisor/arch/arm-common/gic-v2.c
> @@ -265,7 +265,7 @@ static int gic_send_sgi(struct sgi *sgi)
> return 0;
> }
>
> -static int gic_inject_irq(struct per_cpu *cpu_data, u16 irq_id)
> +static int gic_inject_irq(struct per_cpu *cpu_data, u16 irq_id, u16 caller)
> {
> int i;
> int first_free = -1;
> @@ -298,6 +298,8 @@ static int gic_inject_irq(struct per_cpu *cpu_data, u16
> irq_id)
> if (!is_sgi(irq_id)) {
> lr |= GICH_LR_HW_BIT;
> lr |= (u32)irq_id << GICH_LR_PHYS_ID_SHIFT;
> + } else {
> + lr |= (caller & 0x7) << GICH_LR_CPUID_SHIFT;
> }
>
> gic_write_lr(first_free, lr);
> diff --git a/hypervisor/arch/arm-common/include/asm/irqchip.h
> b/hypervisor/arch/arm-common/include/asm/irqchip.h
> index 7cf5d704..5a4a3705 100644
> --- a/hypervisor/arch/arm-common/include/asm/irqchip.h
> +++ b/hypervisor/arch/arm-common/include/asm/irqchip.h
> @@ -48,7 +48,7 @@ struct irqchip {
> int (*send_sgi)(struct sgi *sgi);
> u32 (*read_iar_irqn)(void);
> void (*eoi_irq)(u32 irqn, bool deactivate);
> - int (*inject_irq)(struct per_cpu *cpu_data, u16 irq_id);
> + int (*inject_irq)(struct per_cpu *cpu_data, u16 irq_id, u16 caller);
> void (*enable_maint_irq)(bool enable);
> bool (*has_pending_irqs)(void);
>
> @@ -85,7 +85,7 @@ void irqchip_handle_irq(struct per_cpu *cpu_data);
> bool irqchip_has_pending_irqs(void);
>
> void irqchip_inject_pending(struct per_cpu *cpu_data);
> -void irqchip_set_pending(struct per_cpu *cpu_data, u16 irq_id);
> +void irqchip_set_pending(struct per_cpu *cpu_data, u16 irq_id, u16 caller);
>
> bool irqchip_irq_in_cell(struct cell *cell, unsigned int irq_id);
>
> diff --git a/hypervisor/arch/arm-common/irqchip.c
> b/hypervisor/arch/arm-common/irqchip.c
> index f0f84c92..5e8e28ba 100644
> --- a/hypervisor/arch/arm-common/irqchip.c
> +++ b/hypervisor/arch/arm-common/irqchip.c
> @@ -106,10 +106,11 @@ void gic_handle_sgir_write(struct sgi *sgi)
> struct per_cpu *cpu_data = this_cpu_data();
> unsigned int cpu, target;
> u64 cluster;
> + const u16 caller = cpu_data->cpu_id;
>
> if (sgi->routing_mode == 2)
> /* Route to the caller itself */
> - irqchip_set_pending(cpu_data, sgi->id);
> + irqchip_set_pending(cpu_data, sgi->id, caller);
> else
> for_each_cpu(cpu, cpu_data->cell->cpu_set) {
> if (sgi->routing_mode == 1) {
> @@ -126,7 +127,7 @@ void gic_handle_sgir_write(struct sgi *sgi)
> continue;
> }
>
> - irqchip_set_pending(per_cpu(cpu), sgi->id);
> + irqchip_set_pending(per_cpu(cpu), sgi->id, caller);
> }
> }
>
> @@ -220,7 +221,7 @@ bool irqchip_has_pending_irqs(void)
> return irqchip.has_pending_irqs();
> }
>
> -void irqchip_set_pending(struct per_cpu *cpu_data, u16 irq_id)
> +void irqchip_set_pending(struct per_cpu *cpu_data, u16 irq_id, u16 caller)
> {
> bool local_injection = (this_cpu_data() == cpu_data);
> unsigned int new_tail;
> @@ -233,7 +234,8 @@ void irqchip_set_pending(struct per_cpu *cpu_data, u16
> irq_id)
> return;
> }
>
> - if (local_injection && irqchip.inject_irq(cpu_data, irq_id) != -EBUSY)
> + if (local_injection &&
> + irqchip.inject_irq(cpu_data, irq_id, caller) != -EBUSY)
> return;
>
> spin_lock(&cpu_data->pending_irqs_lock);
> @@ -242,7 +244,8 @@ void irqchip_set_pending(struct per_cpu *cpu_data, u16
> irq_id)
>
> /* Queue space available? */
> if (new_tail != cpu_data->pending_irqs_head) {
> - cpu_data->pending_irqs[cpu_data->pending_irqs_tail] = irq_id;
> + cpu_data->pending_irqs[cpu_data->pending_irqs_tail] =
> + irq_id | (caller << 16);
> cpu_data->pending_irqs_tail = new_tail;
> /*
> * Make the change to pending_irqs_tail visible before the
> @@ -272,12 +275,15 @@ void irqchip_set_pending(struct per_cpu *cpu_data, u16
> irq_id)
>
> void irqchip_inject_pending(struct per_cpu *cpu_data)
> {
> - u16 irq_id;
> + u32 pending;
> + u16 irq_id, caller;
>
> while (cpu_data->pending_irqs_head != cpu_data->pending_irqs_tail) {
> - irq_id = cpu_data->pending_irqs[cpu_data->pending_irqs_head];
> + pending = cpu_data->pending_irqs[cpu_data->pending_irqs_head];
> + irq_id = pending & 0xffff;
> + caller = (pending >> 16) & 0xffff;
>
> - if (irqchip.inject_irq(cpu_data, irq_id) == -EBUSY) {
> + if (irqchip.inject_irq(cpu_data, irq_id, caller) == -EBUSY) {
> /*
> * The list registers are full, trigger maintenance
> * interrupt and leave.
> diff --git a/hypervisor/arch/arm-common/ivshmem.c
> b/hypervisor/arch/arm-common/ivshmem.c
> index 19d300d3..79b8eaf3 100644
> --- a/hypervisor/arch/arm-common/ivshmem.c
> +++ b/hypervisor/arch/arm-common/ivshmem.c
> @@ -18,7 +18,7 @@ void arch_ivshmem_trigger_interrupt(struct ivshmem_endpoint
> *ive)
> unsigned int irq_id = ive->arch.irq_id;
>
> if (irq_id)
> - irqchip_set_pending(NULL, irq_id);
> + irqchip_set_pending(NULL, irq_id, -1);
> }
>
> int arch_ivshmem_update_msix(struct pci_device *device)
> diff --git a/hypervisor/arch/arm/gic-v3.c b/hypervisor/arch/arm/gic-v3.c
> index 9052caee..a331bcbf 100644
> --- a/hypervisor/arch/arm/gic-v3.c
> +++ b/hypervisor/arch/arm/gic-v3.c
> @@ -460,7 +460,7 @@ static void gic_eoi_irq(u32 irq_id, bool deactivate)
> arm_write_sysreg(ICC_DIR_EL1, irq_id);
> }
>
> -static int gic_inject_irq(struct per_cpu *cpu_data, u16 irq_id)
> +static int gic_inject_irq(struct per_cpu *cpu_data, u16 irq_id, u16 caller)
> {
> int i;
> int free_lr = -1;
> @@ -501,6 +501,9 @@ static int gic_inject_irq(struct per_cpu *cpu_data, u16
> irq_id)
> if (!is_sgi(irq_id)) {
> lr |= ICH_LR_HW_BIT;
> lr |= (u64)irq_id << ICH_LR_PHYS_ID_SHIFT;
> + } else {
> + /* For GICv2, we save the calling CPU ID in the list register
> + * in case of SGIs. GICv3 doesn't support this feature. */
> }
>
> gic_write_lr(free_lr, lr);
> diff --git a/hypervisor/arch/arm/include/asm/percpu.h
> b/hypervisor/arch/arm/include/asm/percpu.h
> index 14cf3e13..6bf0a582 100644
> --- a/hypervisor/arch/arm/include/asm/percpu.h
> +++ b/hypervisor/arch/arm/include/asm/percpu.h
> @@ -43,7 +43,9 @@ struct per_cpu {
>
> /* synchronizes parallel insertions of SGIs into the pending ring */
> spinlock_t pending_irqs_lock;
> - u16 pending_irqs[MAX_PENDING_IRQS];
> + /* first 16 bits are for irqn, bits 16- describe the calling cpu
> + * interface */
> + u32 pending_irqs[MAX_PENDING_IRQS];
> unsigned int pending_irqs_head;
> /* removal from the ring happens lockless, thus tail is volatile */
> volatile unsigned int pending_irqs_tail;
> diff --git a/hypervisor/arch/arm64/include/asm/percpu.h
> b/hypervisor/arch/arm64/include/asm/percpu.h
> index e0ffa234..a6938a94 100644
> --- a/hypervisor/arch/arm64/include/asm/percpu.h
> +++ b/hypervisor/arch/arm64/include/asm/percpu.h
> @@ -37,7 +37,9 @@ struct per_cpu {
>
> /* synchronizes parallel insertions of SGIs into the pending ring */
> spinlock_t pending_irqs_lock;
> - u16 pending_irqs[MAX_PENDING_IRQS];
> + /* first 16 bits are for irqn, bits 16- describe the calling cpu
> + * interface */
> + u32 pending_irqs[MAX_PENDING_IRQS];
> unsigned int pending_irqs_head;
> /* removal from the ring happens lockless, thus tail is volatile */
> volatile unsigned int pending_irqs_tail;
>
--
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 [email protected].
For more options, visit https://groups.google.com/d/optout.
