[PATCH v2 2/5] KVM: arm/arm64: Add ARM user space interrupt signaling ABI

[Alexander Graf]

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 at linaro.org> wrote:
>>> From: Christoffer Dall <christoffer.dall at linaro.org>
>>>
>>> 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 at suse.de>
>>> Signed-off-by: Christoffer Dall <christoffer.dall at linaro.org>
>>
>>
>> Acked-by: Peter Maydell <peter.maydell at linaro.org>
>>
>> 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