On 21.02.17 12:41, Christoffer Dall wrote:
Hi Alex,
On Fri, Feb 03, 2017 at 05:51:18PM +0000, Peter Maydell wrote:
On 3 February 2017 at 14:56, Christoffer Dall <cdall@xxxxxxxxxx> wrote:
From: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
We have 2 modes for dealing with interrupts in the ARM world. We can
either handle them all using hardware acceleration through the vgic or
we can emulate a gic in user space and only drive CPU IRQ pins from
there.
Unfortunately, when driving IRQs from user space, we never tell user
space about events from devices emulated inside the kernel, which may
result in interrupt line state changes, so we lose out on for example
timer and PMU events if we run with user space gic emulation.
Define an ABI to publish such device output levels to userspace.
Signed-off-by: Alexander Graf <agraf@xxxxxxx>
Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
Acked-by: Peter Maydell <peter.maydell@xxxxxxxxxx>
for the userspace ABI/docs part.
Given Peter's ack on this ABI, any chance you could take a look at
fixing the userspace SMP issue and respinning the userspace patches for
this series?
The problem with user space SMP was simply a missing lock:
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index a66845f..1b33895 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -548,10 +548,14 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct
kvm_run *run)
/* Synchronize our internal vtimer irq line with the kvm one */
if (run->s.regs.device_irq_level != cpu->device_irq_level) {
- qemu_set_irq(ARM_CPU(cs)->gt_timer_outputs[GTIMER_VIRT],
+ qemu_mutex_lock_iothread();
+ qemu_set_irq(cpu->gt_timer_outputs[GTIMER_VIRT],
run->s.regs.device_irq_level &
KVM_ARM_DEV_EL1_VTIMER);
+ qemu_set_irq(cpu->gt_timer_outputs[GTIMER_PHYS],
+ run->s.regs.device_irq_level &
KVM_ARM_DEV_EL1_PTIMER);
/* TODO: Handle changes in PTIMER and PMU as well */
cpu->device_irq_level = run->s.regs.device_irq_level;
+ qemu_mutex_unlock_iothread();
}
return MEMTXATTRS_UNSPECIFIED;
----
I also wasn't able to use your series out of the box. There are several
places in the code that check whether a timer is enabled (for register
save/restore for example) and without vgic we never ended up setting
that to true.
I don't know how safe it is to set it to true regardless. It feels to me
like someone has to put more thought into how to switch from an
initialized user space timer state to a vgic one. For reference, my test
patch is below:
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 891a725..56a5d51 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -629,8 +629,11 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
return 0;
/* Without a VGIC we do not map virtual IRQs to physical IRQs */
- if (!irqchip_in_kernel(vcpu->kvm))
+ if (!irqchip_in_kernel(vcpu->kvm)) {
+ /* Enable timer for now, may be racy? */
+ timer->enabled = 1;
return 0;
+ }
if (!vgic_initialized(vcpu->kvm))
return -ENODEV;
----
Once we solved that piece, I'll happily respin my user space patch.
Alex
