On 15/09/17 18:55, Jan Kiszka wrote: > On 2017-09-13 16:29, 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); [1] >> } >> } >> >> @@ -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) > > Sender (not "caller") ID should better derived in-place, i.e. in this > function, than passed down. That prevents that all users of this How can I derive it in-place? cpu_data doesn't necessarily have to be the SGI sending CPU. Cf. [1]. > function needs to be changed and even have to pass a value even for > physical IRQs. That will shrink the patch notably. > >> { >> 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. */ > > Please no control structures (even if empty) solely for placing > comments. Just add that remark before the block to explain why there is > no SGI path. > >> } >> >> 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]; > > Use a structure or two arrays, but please no magic bit encoding. In > fact, the struct declaration could even be shared between both archs. > >> 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; >> > > Jan >
-- 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.
