Re: [PATCH] target/arm: do not use cc->do_interrupt for KVM directly

[Claudio Fontana]

On 12/8/20 5:28 PM, Eduardo Habkost wrote:
> On Tue, Dec 08, 2020 at 03:34:03PM +0100, Philippe Mathieu-Daudé wrote:
>> On 12/8/20 2:55 PM, Claudio Fontana wrote:
>>> On 12/8/20 2:51 PM, Claudio Fontana wrote:
>>>> On 12/8/20 2:27 PM, Philippe Mathieu-Daudé wrote:
>>>>> On 12/7/20 10:50 PM, Peter Maydell wrote:
>>>>>> On Mon, 7 Dec 2020 at 21:26, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>>>> My understanding is that there's no reason for ARM KVM to use
>>>>>>> another approach, and that CPUClass.do_interrupt is not really
>>>>>>> TCG-specific.
>>>>>>>
>>>>>>> Do we have any case where the CPUClass.do_interrupt
>>>>>>> implementation is really TCG-specific, or it is just a
>>>>>>> coincidence that most other accelerators simply don't to call the
>>>>>>> method?  It looks like the only cases where the
>>>>>>> CPUClass.do_interrupt assignment is conditional on CONFIG_TCG are
>>>>>>> i386 and s390x.
>>>>>>
>>>>>> Looking at PPC, its kvm_handle_debug() function does a
>>>>>> direct call to ppc_cpu_do_interrupt(). So the code of
>>>>>> its do_interrupt method must be ok-for-KVM, it's just that
>>>>>> it doesn't use the method pointer. (It's doing the same thing
>>>>>> Arm is -- if a debug event turns out not to be for QEMU itself,
>>>>>> inject a suitable exception into the guest.)
>>>>>>
>>>>>> So of our 5 KVM-supporting architectures:
>>>>>>
>>>>>>  * i386 and s390x have kernel APIs for "inject suitable
>>>>>>    exception", don't need to call do_interrupt, and make
>>>>>>    the cc->do_interrupt assignment only ifdef CONFIG_TCG,
>>>>>>    so that the code for do_interrupt need not be compiled
>>>>>>    into a KVM-only binary. (In both cases the code for the
>>>>>>    function is in a source file that the meson.build puts
>>>>>>    into the source list only if CONFIG_TCG)
>>>>>>  * ppc and arm both need to use this code even in a KVM
>>>>>>    only binary. Neither of them #ifdef the cc->do_interrupt
>>>>>>    assignment, because there's not much point at the moment
>>>>>>    if you're not going to try to compile out the code.
>>>>>>    ppc happens to do a direct function call, and arm happens
>>>>>>    to go via the cc->do_interrupt pointer, but I don't
>>>>>>    think there's much significance in the choice either way.
>>>>>>    In both cases, the only places making the call are within
>>>>>>    architecture-specific KVM code.
>>>>>>  * mips KVM does neither of these things, probably because it is
>>>>>>    not sufficiently featureful to have run into the cases
>>>>>>    where you might want to re-inject an exception and it's
>>>>>>    not being sufficiently used in production for anybody to
>>>>>>    have looked at minimising the amount of code in a
>>>>>>    KVM-only QEMU binary for it.
>>>>>>
>>>>>> So in conclusion we have a basically 50:50 split between
>>>>>> "use the same do_interrupt code as TCG" and "have a kernel
>>>>>> API to make the kernel do the work", plus one arch that
>>>>>> probably hasn't had to make the choice yet.   ¯\_(ツ)_/¯
>>>>>
>>>>> Why not introduce KVMCpuOperations similar to TCGCpuOperations
>>>>> Claudio is introducing, and declare the do_interrupt(CPUState*)
>>>>> in both structures?
>>>>>
>>>>> Then we can assign the same handler to both fields, TCG keeps
>>>>> calling cc->tcg->do_interrupt(), KVM calls cc->kvm->do_interrupt().
>>>>> This allow building with a particular accelerator, while staying
>>>>> compliant with the current 50:50 split...
>>>>
>>>>
>>>> Hi Philippe,
>>>>
>>>> in principle interesting, but KVMCpuOperations would end up currently 
>>>> containing do_interrupt only..
>>>> seems a bit overkill for just one method.
>>
>> I don't see this being a problem, if this makes code clearer
>> (think about maintainability).
>>
>>> I mean, all the others in CPUClass are common between TCG and KVM, I don't 
>>> see a lot that is KVM-only there that would warrant a KVMCPUOps structure
>>>
>>>> Or where you thinking of ways to refactor current kvm code to use methods 
>>>> in CPUClass similarly to what Tcg does?
>>>>
>>>
>>> But maybe this is where you were going with this?
>>
>> No, not really. I'm looking for a design to enforce correctness,
>> while keeping the 2 choices Peter mentioned available.
>>
>> - "use the same do_interrupt code as TCG":
>>
>> cc->tcg.do_interrupt = x86_cpu_do_interrupt;
>> cc->kvm.do_interrupt = NULL;
>>
>> cc->tcg.do_interrupt = s390_cpu_do_interrupt;
>> cc->kvm.do_interrupt = NULL;
>>
>> - "have a kernel API to make the kernel do the work"
>>
>> cc->tcg.do_interrupt = arm_cpu_do_interrupt;
>> cc->kvm.do_interrupt = arm_cpu_do_interrupt;
>>
>> cc->tcg.do_interrupt = ppc_cpu_do_interrupt;
>> cc->kvm.do_interrupt = ppc_cpu_do_interrupt;
>>
>> Looks easy to review, hard to misplace #ifdef'ry.
> 
> So, methods that have accel-specific implementations, which is
> exactly why we have the CpusAccel struct (renamed to

CpusAccel (in the new series "AccelOpsClass") is (currently) about 
arch-independent, softmmu accel operations, used in the softmmu/cpus.c module.
The "Ops" in AccelOpsClass are create_vcpu_thread, kick_vcpu_thread, 
synchronize_*, handle_interrupt, get_virtual_clock, get_elapsed_ticks. These 
things are accel-dependent, but not arch-dependent.

> AccelCpuClass in Claudio's cleanup series).


AccelCPUClass is (in the cleanup series) instead about Arch-dependent 
specialization of the CPUClass inits (constructors, class initializers, realize 
functions, ...),
working around the problem with the fact that we cannot easily subclass cpus 
for accelerators, without changing the existing object hierarchies, which I got 
negative feedback about.

So we have there arch-dependent cpu object constructors, class initializers 
etc, used for both user-mode and softmmu.

So in AccelCPUClass we currently have cpu_class_init, cpu_instance_init, 
cpu_realizefn.

> 
> Is there any reason to not move CPUClass.do_interrupt to
> AccelCpuClass.do_interrupt?
> 

This seems an interesting idea to me.

Ciao,

Claudio