[RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling
Marc Zyngier
marc.zyngier at arm.com
Thu Mar 8 01:49:43 PST 2018
[updated Christoffer's email address]
Hi Shunyong,
On 08/03/18 07:01, Shunyong Yang wrote:
> When resampling irqfds is enabled, level interrupt should be
> de-asserted when resampling happens. On page 4-47 of GIC v3
> specification IHI0069D, it said,
> "When the PE acknowledges an SGI, a PPI, or an SPI at the CPU
> interface, the IRI changes the status of the interrupt to active
> and pending if:
> • It is an edge-triggered interrupt, and another edge has been
> detected since the interrupt was acknowledged.
> • It is a level-sensitive interrupt, and the level has not been
> deasserted since the interrupt was acknowledged."
>
> GIC v2 specification IHI0048B.b has similar description on page
> 3-42 for state machine transition.
>
> When some VFIO device, like mtty(8250 VFIO mdev emulation driver
> in samples/vfio-mdev) triggers a level interrupt, the status
> transition in LR is pending-->active-->active and pending.
> Then it will wait resampling to de-assert the interrupt.
>
> Current design of lr_signals_eoi_mi() will return false if state
> in LR is not invalid(Inactive). It causes resampling will not happen
> in mtty case.
Let me rephrase this, and tell me if I understood it correctly:
- A level interrupt is injected, activated by the guest (LR state=active)
- guest exits, re-enters, (LR state=pending+active)
- guest EOIs the interrupt (LR state=pending)
- maintenance interrupt
- we don't signal the resampling because we're not in an invalid state
Is that correct?
That's an interesting case, because it seems to invalidate some of the
optimization that went in over a year ago.
096f31c4360f KVM: arm/arm64: vgic: Get rid of MISR and EISR fields
b6095b084d87 KVM: arm/arm64: vgic: Get rid of unnecessary save_maint_int_state
af0614991ab6 KVM: arm/arm64: vgic: Get rid of unnecessary process_maintenance operation
We could compare the value of the LR before the guest entry with
the value at exit time, but we still could miss it if we have a
transition such as P+A -> P -> A and assume a long enough propagation
delay for the maintenance interrupt (which is very likely).
In essence, we have lost the benefit of EISR, which was to give us a
way to deal with asynchronous signalling.
>
> This will cause interrupt fired continuously to guest even 8250 IIR
> has no interrupt. When 8250's interrupt is configured in shared mode,
> it will pass interrupt to other drivers to handle. However, there
> is no other driver involved. Then, a "nobody cared" kernel complaint
> occurs.
>
> / # cat /dev/ttyS0
> [ 4.826836] random: crng init done
> [ 6.373620] irq 41: nobody cared (try booting with the "irqpoll"
> option)
> [ 6.376414] CPU: 0 PID: 1307 Comm: cat Not tainted 4.16.0-rc4 #4
> [ 6.378927] Hardware name: linux,dummy-virt (DT)
> [ 6.380876] Call trace:
> [ 6.381937] dump_backtrace+0x0/0x180
> [ 6.383495] show_stack+0x14/0x1c
> [ 6.384902] dump_stack+0x90/0xb4
> [ 6.386312] __report_bad_irq+0x38/0xe0
> [ 6.387944] note_interrupt+0x1f4/0x2b8
> [ 6.389568] handle_irq_event_percpu+0x54/0x7c
> [ 6.391433] handle_irq_event+0x44/0x74
> [ 6.393056] handle_fasteoi_irq+0x9c/0x154
> [ 6.394784] generic_handle_irq+0x24/0x38
> [ 6.396483] __handle_domain_irq+0x60/0xb4
> [ 6.398207] gic_handle_irq+0x98/0x1b0
> [ 6.399796] el1_irq+0xb0/0x128
> [ 6.401138] _raw_spin_unlock_irqrestore+0x18/0x40
> [ 6.403149] __setup_irq+0x41c/0x678
> [ 6.404669] request_threaded_irq+0xe0/0x190
> [ 6.406474] univ8250_setup_irq+0x208/0x234
> [ 6.408250] serial8250_do_startup+0x1b4/0x754
> [ 6.410123] serial8250_startup+0x20/0x28
> [ 6.411826] uart_startup.part.21+0x78/0x144
> [ 6.413633] uart_port_activate+0x50/0x68
> [ 6.415328] tty_port_open+0x84/0xd4
> [ 6.416851] uart_open+0x34/0x44
> [ 6.418229] tty_open+0xec/0x3c8
> [ 6.419610] chrdev_open+0xb0/0x198
> [ 6.421093] do_dentry_open+0x200/0x310
> [ 6.422714] vfs_open+0x54/0x84
> [ 6.424054] path_openat+0x2dc/0xf04
> [ 6.425569] do_filp_open+0x68/0xd8
> [ 6.427044] do_sys_open+0x16c/0x224
> [ 6.428563] SyS_openat+0x10/0x18
> [ 6.429972] el0_svc_naked+0x30/0x34
> [ 6.431494] handlers:
> [ 6.432479] [<000000000e9fb4bb>] serial8250_interrupt
> [ 6.434597] Disabling IRQ #41
>
> This patch changes the lr state condition in lr_signals_eoi_mi() from
> invalid(Inactive) to active and pending to avoid this.
>
> I am not sure about the original design of the condition of
> invalid(active). So, This RFC is sent out for comments.
>
> Cc: Joey Zheng <yu.zheng at hxt-semitech.com>
> Signed-off-by: Shunyong Yang <shunyong.yang at hxt-semitech.com>
> ---
> virt/kvm/arm/vgic/vgic-v2.c | 4 ++--
> virt/kvm/arm/vgic/vgic-v3.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index e9d840a75e7b..740ee9a5f551 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -46,8 +46,8 @@ void vgic_v2_set_underflow(struct kvm_vcpu *vcpu)
>
> static bool lr_signals_eoi_mi(u32 lr_val)
> {
> - return !(lr_val & GICH_LR_STATE) && (lr_val & GICH_LR_EOI) &&
> - !(lr_val & GICH_LR_HW);
> + return !((lr_val & GICH_LR_STATE) ^ GICH_LR_STATE) &&
That feels very wrong. You're now signalling the resampling in both
invalid and pending+active, and the latter state doesn't mean you've
EOIed anything. You're now over-signalling, and signalling the
wrong event.
> + (lr_val & GICH_LR_EOI) && !(lr_val & GICH_LR_HW);
> }
>
> /*
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 6b329414e57a..43111bba7af9 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -35,8 +35,8 @@ void vgic_v3_set_underflow(struct kvm_vcpu *vcpu)
>
> static bool lr_signals_eoi_mi(u64 lr_val)
> {
> - return !(lr_val & ICH_LR_STATE) && (lr_val & ICH_LR_EOI) &&
> - !(lr_val & ICH_LR_HW);
> + return !((lr_val & ICH_LR_STATE) ^ ICH_LR_STATE) &&
> + (lr_val & ICH_LR_EOI) && !(lr_val & ICH_LR_HW);
> }
>
> void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
>
Assuming I understand the issue correctly, I cannot really see how
to solve this without reintroducing EISR, which sucks majorly.
I'll try to cook something shortly and we can all have a good
fight about how crap this is.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
More information about the linux-arm-kernel
mailing list