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

[Eduardo Habkost]

On Mon, Dec 07, 2020 at 06:07:32PM +0000, Peter Maydell wrote:
> On Mon, 7 Dec 2020 at 17:49, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > arm_v7m_class_init() (used by cortex-* CPU models) overrides it.
> > Those CPU models as "TCG CPUs" in the code, but I don't see what
> > makes them TCG-specific.
> 
> They're TCG specific because the Arm Virtualization Extension
> is for A-profile only and only supports virtualization of
> A-profile CPUs. You can't accelerate an M-profile CPU with it.
> (Similarly, you can't use the Virtualization Extension to
> accelerate a pre-v7 CPU, which is why CPUs like the arm1176
> are also TCG-only, and you can't use it to accelerate a CPU
> which has TrustZone enabled.)
> 
> (M-profile CPUs override cc->do_interrupt() because their
> exception and interrupt handling logic is totally different
> to A-profile.)
> 
> > What exactly is the expected behavior
> > if using, e.g., "-cpu cortex-m33 -accel kvm"?
> 
> It ought to print a useful error message telling you
> that that CPU type isn't compatible with KVM.
> 
> As it happens, you get an assertion failure for cortex-m33:
> 
> $ ./build/all/qemu-system-aarch64 -accel kvm -M mps2-an505 -display none
> qemu-system-aarch64: ../../softmmu/physmem.c:745:
> cpu_address_space_init: Assertion `asidx == 0 || !kvm_enabled()'
> failed.
> Aborted
> 
> because the M33 has TrustZone enabled (which is not compatible
> with KVM) and we don't check that before we hit the assertion.
> We should fix that :-)
> 
> If you try it with a non-TrustZone M-profile core like the M3 then
> you do get a better error message:
> 
> $ ./build/all/qemu-system-aarch64 -accel kvm -M mps2-an385 -display none
> qemu-system-aarch64: KVM is not supported for this guest CPU type
> qemu-system-aarch64: kvm_init_vcpu: kvm_arch_init_vcpu failed (0):
> Invalid argument
> 
> If you try this with the "virt" board then you'll run into the
> virt board's own sanity checking of CPU types instead:
> 
> $ ./build/all/qemu-system-aarch64 -accel kvm -M virt -cpu cortex-m33
> -display none
> qemu-system-aarch64: mach-virt: CPU type cortex-m33-arm-cpu not supported

Thanks!

So this patch seem correct, but we still have the questions
below:

> 
> All of that said, I think I agree with you -- we have this
> indirect mechanism for invoking class methods on the CPU
> object, why is it necessary for this KVM-specific code to
> call the implementation function directly?

All signs seem to indicate that CPUClass.do_interrupt is
TCG-specific, except for those two lines of code in
target/arm/kvm64.c.  The point of this patch would be to allow us
to separate TCG-specific code from accel-independent code later.

Maybe those signs are misleading us, and CPUClass.do_interrupt
shouldn't be TCG-specific.  If that's the case, why arm is the
only architecture that uses CPUClass.do_interrupt outside
TCG-specific code?

-- 
Eduardo