Re: [Qemu-devel] [PATCH 04/13] target: arm: Move all interrupt and exception handlers into their own file

[Samuel Ortiz]

On Wed, Nov 28, 2018 at 02:57:19PM +0100, Samuel Ortiz wrote:
> On Wed, Nov 28, 2018 at 11:39:57AM +0000, Peter Maydell wrote:
> > On Wed, 28 Nov 2018 at 10:40, Samuel Ortiz <address@hidden> wrote:
> > > Given that this piece of code effectively builds a dependency to TCG
> > > from the KVM code, I see a few solutions but I need your input here. We
> > > could:
> > >
> > > - Decide we don't want to support --disable-tcg for ARM. We'd then carry
> > >   this patch serie from the NEMU code repo. Worst case scenario, at
> > >   least for us.
> > > - Manage to implement exception injection from userspace without TCG.
> > >   Would it even be possible?
> > > - Offload exception injections back to the kernel in those cases. I feel
> > >   this would be the cleanest solution but may need kernel changes.
> > 
> > The kernel folk were firmly against 3, IIRC, but you can go
> > and have the discussion if you like.
> > 
> > I don't really see what the problem is. This is just a bit
> > of code that's used by both TCG and KVM. Therefore it goes
> > in the binary whether TCG is enabled or not. Other functions
> > and bits of code are TCG only and therefore don't go in a
> > KVM-only binary.
> Keeping this code with --disable-tcg means:
> 
> Keep arm_cpu_do_interrupt -> Keep check_for_semihosting -> Keep the arm
> instruction loading code -> Keep a large chunk of the TCG core code
> itself. Does that dependency chain looks fine to you?
A simplified, aarch64 specific arm_cpu_do_interrupt() implementation
would not pull the TCG code in. Something like:

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 0a502091e7..eba7ced564 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -1034,7 +1034,6 @@ bool kvm_arm_handle_debug(CPUState *cs, struct 
kvm_debug_exit_arch *debug_exit)
 {
     int hsr_ec = syn_get_ec(debug_exit->hsr);
     ARMCPU *cpu = ARM_CPU(cs);
-    CPUClass *cc = CPU_GET_CLASS(cs);
     CPUARMState *env = &cpu->env;
 
     /* Ensure PC is synchronised */
@@ -1088,7 +1087,22 @@ bool kvm_arm_handle_debug(CPUState *cs, struct 
kvm_debug_exit_arch *debug_exit)
     env->exception.vaddress = debug_exit->far;
     env->exception.target_el = 1;
     qemu_mutex_lock_iothread();
-    cc->do_interrupt(cs);
+
+    /* Hooks may change global state so BQL should be held, also the
+     * BQL needs to be held for any modification of
+     * cs->interrupt_request.
+     */
+    g_assert(qemu_mutex_iothread_locked());
+
+    arm_call_pre_el_change_hook(cpu);
+
+    assert(!excp_is_internal(cs->exception_index));
+    arm_cpu_do_interrupt_aarch64(cs);
+
+    arm_call_el_change_hook(cpu);
+
+    cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
     qemu_mutex_unlock_iothread();
 
     return false;