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);
>> }
>> }
>>
>> @@ -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
> 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.
Whole percpu.h could probably be consolidated. ARM just has ~5 members
more than ARM64.
Ralf
>
>> 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.
