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

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

[Claudio Fontana]

cc->do_interrupt is a TCG callback used in accel/tcg only,
call instead directly the arm_cpu_do_interrupt for the
injection of exeptions from KVM, so that

do_interrupt can be exported to TCG-only operations in the CPUClass.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 target/arm/helper.c | 4 ++++
 target/arm/kvm64.c  | 4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 38cd35c049..bebaabf525 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9895,6 +9895,10 @@ static void handle_semihosting(CPUState *cs)
  * Do any appropriate logging, handle PSCI calls, and then hand off
  * to the AArch64-entry or AArch32-entry function depending on the
  * target exception level's register width.
+ *
+ * Note: this is used for both TCG (as the do_interrupt tcg op),
+ *       and KVM to re-inject guest debug exceptions, and to
+ *       inject a Synchronous-External-Abort.
  */
 void arm_cpu_do_interrupt(CPUState *cs)
 {
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index f74bac2457..2b17e4203d 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -960,7 +960,7 @@ static void kvm_inject_arm_sea(CPUState *c)
 
     env->exception.syndrome = esr;
 
-    cc->do_interrupt(c);
+    arm_cpu_do_interrupt(c);
 }
 
 #define AARCH64_CORE_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
@@ -1545,7 +1545,7 @@ 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);
+    arm_cpu_do_interrupt(cs);
     qemu_mutex_unlock_iothread();
 
     return false;
-- 
2.26.2

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

[Philippe Mathieu-Daudé]

On 12/7/20 9:40 AM, Claudio Fontana wrote:
> cc->do_interrupt is a TCG callback used in accel/tcg only,
> call instead directly the arm_cpu_do_interrupt for the
> injection of exeptions from KVM, so that
> 
> do_interrupt can be exported to TCG-only operations in the CPUClass.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  target/arm/helper.c | 4 ++++
>  target/arm/kvm64.c  | 4 ++--
>  2 files changed, 6 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

[Alex Bennée]

Claudio Fontana <cfontana@suse.de> writes:

> cc->do_interrupt is a TCG callback used in accel/tcg only,
> call instead directly the arm_cpu_do_interrupt for the
> injection of exeptions from KVM, so that
>
> do_interrupt can be exported to TCG-only operations in the CPUClass.
>
> Signed-off-by: Claudio Fontana <cfontana@suse.de>

LGTM:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

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

[Eduardo Habkost]

On Mon, Dec 07, 2020 at 09:40:42AM +0100, Claudio Fontana wrote:
> cc->do_interrupt is a TCG callback used in accel/tcg only,
> call instead directly the arm_cpu_do_interrupt for the
> injection of exeptions from KVM, so that
> 
> do_interrupt can be exported to TCG-only operations in the CPUClass.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  target/arm/helper.c | 4 ++++
>  target/arm/kvm64.c  | 4 ++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 38cd35c049..bebaabf525 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9895,6 +9895,10 @@ static void handle_semihosting(CPUState *cs)
>   * Do any appropriate logging, handle PSCI calls, and then hand off
>   * to the AArch64-entry or AArch32-entry function depending on the
>   * target exception level's register width.
> + *
> + * Note: this is used for both TCG (as the do_interrupt tcg op),
> + *       and KVM to re-inject guest debug exceptions, and to
> + *       inject a Synchronous-External-Abort.
>   */
>  void arm_cpu_do_interrupt(CPUState *cs)
>  {
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index f74bac2457..2b17e4203d 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -960,7 +960,7 @@ static void kvm_inject_arm_sea(CPUState *c)
>  
>      env->exception.syndrome = esr;
>  
> -    cc->do_interrupt(c);
> +    arm_cpu_do_interrupt(c);

How can we be sure cc->do_interrupt always points to
arm_cpu_do_interrupt today?

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.  What exactly is the expected behavior
if using, e.g., "-cpu cortex-m33 -accel kvm"?


>  }
>  
>  #define AARCH64_CORE_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
> @@ -1545,7 +1545,7 @@ 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);
> +    arm_cpu_do_interrupt(cs);
>      qemu_mutex_unlock_iothread();
>  
>      return false;
> -- 
> 2.26.2
> 

-- 
Eduardo

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

[Peter Maydell]

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

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?

thanks
-- PMM

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

[Claudio Fontana]

On 12/7/20 7:07 PM, 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
> 
> 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?
> 
> thanks
> -- PMM
> 

Hi Peter,

the initial attempt there was to mark do_interrupt as a TCG-only operation,
to move to a separate tcg_ops structure in CPUClass like for the others,

but countrary to the other ops I noticed that ARM is the only target that is calling cc->do_interrupt() for KVM too.

I might have to either leave do_interrupt() out of the refactoring, or find a way to avoid accessing cc->do_interrupt on KVM.

Thanks,

Claudio

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

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

[Peter Maydell]

On Mon, 7 Dec 2020 at 18:28, Eduardo Habkost <ehabkost@redhat.com> wrote:
> 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.

So it's TCG-specific except that we call it from KVM.
That doesn't sound very TCG-specific :-)

> 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?

So, the purpose of the do_interrupt method is "set the guest
CPU state up in the way that the architecture specifies
happens when an interrupt is taken" (set the program counter,
set things like the syndrome register that says what type
of exception happens, etc, etc). For TCG we obviously need
to do this for every interrupt/exception that happens.
For KVM, in most cases the kernel is responsible for
delivering an exception to the guest, because it's the
kernel that determines that it needs to do this.
The two oddball cases[*] in target/arm are for situations
where it is userspace code that has identified that it
needs to deliver an exception to the guest. The kernel's
KVM API doesn't provide a "please deliver an exception to
the guest" function, on the grounds that userspace could
do the work itself. So we need to do that work (setting the
PC, setting syndrome register, etc, etc). In theory we
could have a special version of the function for KVM
CPUs only, but since in fact the same code works just
fine in KVM and TCG we reuse it.

I know that the macOS Hypervisor.Framework APIs are rather
lower-level than KVM (they do less work in the kernel and
push more of it onto userspace); it's possible that there
we might find more situations where userspace needs to do
"make the guest CPU take an exception"; I haven't investigated.

[*] The two special cases are:
 (1) the vcpu thread got a SIGBUS indicating a memory error,
     and we need to deliver a synchronous external abort
     exception to the guest to let it know about the error
 (2) the kernel told us about a debug exception (breakpoint,
     watchpoint, etc) but it turns out not to be for one of
     QEMU's own gdbstub breakpoints/watchpoints, so it
     must be one the guest itself has set up, and so we need
     to deliver it to the guest

These are fairly obscure, and it wouldn't surprise me if
other target archs had picked a different separation of
concerns between userspace and the kernel such that userspace
didn't need to manually deliver an exception.

thanks
-- PMM

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

[Peter Maydell]

On Mon, 7 Dec 2020 at 20:56, Peter Maydell <peter.maydell@linaro.org> wrote:
> These are fairly obscure, and it wouldn't surprise me if
> other target archs had picked a different separation of
> concerns between userspace and the kernel such that userspace
> didn't need to manually deliver an exception.

...looking at the target/i386 code, it looks like the
approach taken on that architecture is that userspace just
asks the kernel to take the exception using the KVM_SET_VCPU_EVENTS
ioctl. (For Arm we only use that ioctl to handle SError,
because the information that an SError is pending is a piece
of state that's external to the CPU.)

thanks
-- PMM

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

[Claudio Fontana]

On 12/7/20 9:56 PM, Peter Maydell wrote:
> On Mon, 7 Dec 2020 at 18:28, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> 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.
> 
> So it's TCG-specific except that we call it from KVM.
> That doesn't sound very TCG-specific :-)
> 
>> 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?
> 
> So, the purpose of the do_interrupt method is "set the guest
> CPU state up in the way that the architecture specifies
> happens when an interrupt is taken" (set the program counter,
> set things like the syndrome register that says what type
> of exception happens, etc, etc). For TCG we obviously need
> to do this for every interrupt/exception that happens.
> For KVM, in most cases the kernel is responsible for
> delivering an exception to the guest, because it's the
> kernel that determines that it needs to do this.
> The two oddball cases[*] in target/arm are for situations
> where it is userspace code that has identified that it
> needs to deliver an exception to the guest. The kernel's
> KVM API doesn't provide a "please deliver an exception to
> the guest" function, on the grounds that userspace could
> do the work itself. So we need to do that work (setting the
> PC, setting syndrome register, etc, etc). In theory we
> could have a special version of the function for KVM
> CPUs only, but since in fact the same code works just
> fine in KVM and TCG we reuse it.
> 
> I know that the macOS Hypervisor.Framework APIs are rather
> lower-level than KVM (they do less work in the kernel and
> push more of it onto userspace); it's possible that there
> we might find more situations where userspace needs to do
> "make the guest CPU take an exception"; I haven't investigated.
> 
> [*] The two special cases are:
>  (1) the vcpu thread got a SIGBUS indicating a memory error,
>      and we need to deliver a synchronous external abort
>      exception to the guest to let it know about the error
>  (2) the kernel told us about a debug exception (breakpoint,
>      watchpoint, etc) but it turns out not to be for one of
>      QEMU's own gdbstub breakpoints/watchpoints, so it
>      must be one the guest itself has set up, and so we need
>      to deliver it to the guest
> 
> These are fairly obscure, and it wouldn't surprise me if
> other target archs had picked a different separation of
> concerns between userspace and the kernel such that userspace
> didn't need to manually deliver an exception.
> 
> thanks
> -- PMM
> 

Hello Peter,

thank you for the explanation, interesting read.

As I understand it, for the purpose of code separation,
we could:

1) skip do_interrupt move to the separate tcg_ops structure, wait until KVM/ARM uses another approach (if ever)
2) do the move, and just call arm_cpu_do_interrupt() directly, since for KVM64 it is the only one that can be assigned to cc->do_interrupt().

Which way would you suggest?

Thanks,

Claudio

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

[Peter Maydell]

On Mon, 7 Dec 2020 at 21:06, Claudio Fontana <cfontana@suse.de> wrote:
> As I understand it, for the purpose of code separation,
> we could:
>
> 1) skip do_interrupt move to the separate tcg_ops structure, wait until KVM/ARM uses another approach (if ever)
> 2) do the move, and just call arm_cpu_do_interrupt() directly, since for KVM64 it is the only one that can be assigned to cc->do_interrupt().
>
> Which way would you suggest?

So what's the intention here? To put tcg-only methods in their
own struct so that on a KVM-only QEMU they're compiled out
and attempts to use them are a compile error? In that case
I guess if Arm really is the only user of do_interrupt outside
TCG then we should do what this patch does and do a direct call.

thanks
-- PMM

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

[Claudio Fontana]

On 12/7/20 10:12 PM, Peter Maydell wrote:
> On Mon, 7 Dec 2020 at 21:06, Claudio Fontana <cfontana@suse.de> wrote:
>> As I understand it, for the purpose of code separation,
>> we could:
>>
>> 1) skip do_interrupt move to the separate tcg_ops structure, wait until KVM/ARM uses another approach (if ever)
>> 2) do the move, and just call arm_cpu_do_interrupt() directly, since for KVM64 it is the only one that can be assigned to cc->do_interrupt().
>>
>> Which way would you suggest?
> 
> So what's the intention here? To put tcg-only methods in their
> own struct so that on a KVM-only QEMU they're compiled out
> and attempts to use them are a compile error?

Yes.

> In that case
> I guess if Arm really is the only user of do_interrupt outside
> TCG then we should do what this patch does and do a direct call.
> 
> thanks
> -- PMM
> 

Thanks, will do!

Claudio

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

[Eduardo Habkost]

On Mon, Dec 07, 2020 at 09:12:34PM +0000, Peter Maydell wrote:
> On Mon, 7 Dec 2020 at 21:06, Claudio Fontana <cfontana@suse.de> wrote:
> > As I understand it, for the purpose of code separation,
> > we could:
> >
> > 1) skip do_interrupt move to the separate tcg_ops structure, wait until KVM/ARM uses another approach (if ever)
> > 2) do the move, and just call arm_cpu_do_interrupt() directly, since for KVM64 it is the only one that can be assigned to cc->do_interrupt().
> >
> > Which way would you suggest?
> 
> So what's the intention here? To put tcg-only methods in their
> own struct so that on a KVM-only QEMU they're compiled out
> and attempts to use them are a compile error? In that case
> I guess if Arm really is the only user of do_interrupt outside
> TCG then we should do what this patch does and do a direct call.

Oh, I thought you were arguing that CPUClass.do_interrupt is
not TCG_specific.

If you agree with doing what this patch does, the plan sounds
good to me.

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Dec 07, 2020 at 10:06:55PM +0100, Claudio Fontana wrote:
> On 12/7/20 9:56 PM, Peter Maydell wrote:
> > On Mon, 7 Dec 2020 at 18:28, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> 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.
> > 
> > So it's TCG-specific except that we call it from KVM.
> > That doesn't sound very TCG-specific :-)
> > 
> >> 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?
> > 
> > So, the purpose of the do_interrupt method is "set the guest
> > CPU state up in the way that the architecture specifies
> > happens when an interrupt is taken" (set the program counter,
> > set things like the syndrome register that says what type
> > of exception happens, etc, etc). For TCG we obviously need
> > to do this for every interrupt/exception that happens.
> > For KVM, in most cases the kernel is responsible for
> > delivering an exception to the guest, because it's the
> > kernel that determines that it needs to do this.
> > The two oddball cases[*] in target/arm are for situations
> > where it is userspace code that has identified that it
> > needs to deliver an exception to the guest. The kernel's
> > KVM API doesn't provide a "please deliver an exception to
> > the guest" function, on the grounds that userspace could
> > do the work itself. So we need to do that work (setting the
> > PC, setting syndrome register, etc, etc). In theory we
> > could have a special version of the function for KVM
> > CPUs only, but since in fact the same code works just
> > fine in KVM and TCG we reuse it.
> > 
> > I know that the macOS Hypervisor.Framework APIs are rather
> > lower-level than KVM (they do less work in the kernel and
> > push more of it onto userspace); it's possible that there
> > we might find more situations where userspace needs to do
> > "make the guest CPU take an exception"; I haven't investigated.
> > 
> > [*] The two special cases are:
> >  (1) the vcpu thread got a SIGBUS indicating a memory error,
> >      and we need to deliver a synchronous external abort
> >      exception to the guest to let it know about the error
> >  (2) the kernel told us about a debug exception (breakpoint,
> >      watchpoint, etc) but it turns out not to be for one of
> >      QEMU's own gdbstub breakpoints/watchpoints, so it
> >      must be one the guest itself has set up, and so we need
> >      to deliver it to the guest
> > 
> > These are fairly obscure, and it wouldn't surprise me if
> > other target archs had picked a different separation of
> > concerns between userspace and the kernel such that userspace
> > didn't need to manually deliver an exception.
> > 
> > thanks
> > -- PMM
> > 
> 
> Hello Peter,
> 
> thank you for the explanation, interesting read.
> 
> As I understand it, for the purpose of code separation,
> we could:
> 
> 1) skip do_interrupt move to the separate tcg_ops structure,
> wait until KVM/ARM uses another approach (if ever)

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.


> 2) do the move, and just call arm_cpu_do_interrupt() directly,
> since for KVM64 it is the only one that can be assigned to
> cc->do_interrupt().
> 
> Which way would you suggest?
> 
> Thanks,
> 
> Claudio
> 
> 
> 
> 
> 

-- 
Eduardo

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

[Peter Maydell]

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.   ¯\_(ツ)_/¯

> Oh, I thought you were arguing that CPUClass.do_interrupt is
> not TCG_specific.

Well, I don't think it really is TCG-specific. But as
a pragmatic thing, if these two lines in the Arm code
are getting in the way of stopping us from having a
useful compile-time check that code that's not supposed
to call this method isn't calling it, I think the balance
maybe leans towards just making the direct function call.
I guess it depends whether you think people are likely to
accidentally make cc->do_interrupt calls in non-target-specific
code that gets used by KVM (which currently would crash if that
code path is exercised on x86 or s390x, but under the
proposed change would become a compile error).

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

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...

> 
>> Oh, I thought you were arguing that CPUClass.do_interrupt is
>> not TCG_specific.
> 
> Well, I don't think it really is TCG-specific. But as
> a pragmatic thing, if these two lines in the Arm code
> are getting in the way of stopping us from having a
> useful compile-time check that code that's not supposed
> to call this method isn't calling it, I think the balance
> maybe leans towards just making the direct function call.
> I guess it depends whether you think people are likely to
> accidentally make cc->do_interrupt calls in non-target-specific
> code that gets used by KVM (which currently would crash if that
> code path is exercised on x86 or s390x, but under the
> proposed change would become a compile error).
> 
> thanks
> -- PMM
> 

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

[Claudio Fontana]

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.
Or where you thinking of ways to refactor current kvm code to use methods in CPUClass similarly to what Tcg does?

Ciao,

Claudio


> 
>>
>>> Oh, I thought you were arguing that CPUClass.do_interrupt is
>>> not TCG_specific.
>>
>> Well, I don't think it really is TCG-specific. But as
>> a pragmatic thing, if these two lines in the Arm code
>> are getting in the way of stopping us from having a
>> useful compile-time check that code that's not supposed
>> to call this method isn't calling it, I think the balance
>> maybe leans towards just making the direct function call.
>> I guess it depends whether you think people are likely to
>> accidentally make cc->do_interrupt calls in non-target-specific
>> code that gets used by KVM (which currently would crash if that
>> code path is exercised on x86 or s390x, but under the
>> proposed change would become a compile error).
>>
>> thanks
>> -- PMM
>>
> 

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

[Claudio Fontana]

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 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?

Ciao,

C

> Ciao,
> 
> Claudio
> 
> 
>>
>>>
>>>> Oh, I thought you were arguing that CPUClass.do_interrupt is
>>>> not TCG_specific.
>>>
>>> Well, I don't think it really is TCG-specific. But as
>>> a pragmatic thing, if these two lines in the Arm code
>>> are getting in the way of stopping us from having a
>>> useful compile-time check that code that's not supposed
>>> to call this method isn't calling it, I think the balance
>>> maybe leans towards just making the direct function call.
>>> I guess it depends whether you think people are likely to
>>> accidentally make cc->do_interrupt calls in non-target-specific
>>> code that gets used by KVM (which currently would crash if that
>>> code path is exercised on x86 or s390x, but under the
>>> proposed change would become a compile error).
>>>
>>> thanks
>>> -- PMM
>>>
>>
> 

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

[Philippe Mathieu-Daudé]

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.

> 
> Ciao,
> 
> C

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

[Claudio Fontana]

On 12/8/20 3:34 PM, 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.

to limit feature creep in the series even less error prone would be to put do_interrupt only for #tcg for now,
I think, and call the function directly in the only place it is necessary (arm/kvm64.c),
and then revisit this when we do the actual code split between tcg and kvm (on arm there are already quite a few things to do I think).

How does it sound?

Ciao,

Claudio

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

[Eduardo Habkost]

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
AccelCpuClass in Claudio's cleanup series).

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

-- 
Eduardo

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

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

[Claudio Fontana]

On 12/7/20 6:49 PM, Eduardo Habkost wrote:
> On Mon, Dec 07, 2020 at 09:40:42AM +0100, Claudio Fontana wrote:
>> cc->do_interrupt is a TCG callback used in accel/tcg only,
>> call instead directly the arm_cpu_do_interrupt for the
>> injection of exeptions from KVM, so that
>>
>> do_interrupt can be exported to TCG-only operations in the CPUClass.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  target/arm/helper.c | 4 ++++
>>  target/arm/kvm64.c  | 4 ++--
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 38cd35c049..bebaabf525 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -9895,6 +9895,10 @@ static void handle_semihosting(CPUState *cs)
>>   * Do any appropriate logging, handle PSCI calls, and then hand off
>>   * to the AArch64-entry or AArch32-entry function depending on the
>>   * target exception level's register width.
>> + *
>> + * Note: this is used for both TCG (as the do_interrupt tcg op),
>> + *       and KVM to re-inject guest debug exceptions, and to
>> + *       inject a Synchronous-External-Abort.
>>   */
>>  void arm_cpu_do_interrupt(CPUState *cs)
>>  {
>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>> index f74bac2457..2b17e4203d 100644
>> --- a/target/arm/kvm64.c
>> +++ b/target/arm/kvm64.c
>> @@ -960,7 +960,7 @@ static void kvm_inject_arm_sea(CPUState *c)
>>  
>>      env->exception.syndrome = esr;
>>  
>> -    cc->do_interrupt(c);
>> +    arm_cpu_do_interrupt(c);
> 
> How can we be sure cc->do_interrupt always points to
> arm_cpu_do_interrupt today?

You are right, I am currently looking at the same problem.

> 
> 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.  What exactly is the expected behavior
> if using, e.g., "-cpu cortex-m33 -accel kvm"?

I agree that's a problem, and I am looking to fix it,
but also:

what about also the existing code with qemu-arm (user mode)?

In that case do_interrupt is not set at all in target/arm/cpu.c, since it's protected by #ifndef CONFIG_USER_ONLY

Did we have a potential NULL pointer trying to be dereferenced there?

Commit 0adf7d3cc3f724e1e9ce5aaa008bd9daeb490f19 says:

 target-arm: do not set do_interrupt handlers for ARM and AArch64 user modes
    
 User mode emulation should never get interrupts and thus should not
 use the system emulation exception handler function.

--

But this was 2014. Is the comment above true today?

Looking at this commit in 2017, it does not seem to me to be the case:

commit 17b50b0c299f1266578b01f7134810362418ac2e
Author: Pavel Dovgaluk <Pavel.Dovgaluk@ispras.ru>
Date:   Tue Nov 14 11:18:18 2017 +0300

    cpu-exec: avoid cpu_exec_nocache infinite loop with record/replay
    
    This patch
    [...]
    Second, try to cause the exception at the beginning of
    cpu_handle_exception, and exit immediately if the TB cannot
    execute.  With this change, interrupts are processed and
    cpu_exec_nocache can make process.

--

So to me it seems like this creates the potential for a NULL pointer deref, today, in arm user mode,
since the handler is set only for !CONFIG_USER_ONLY, but it is potentially used also in user mode.

Is cc->do_interrupt supposed to be !CONFIG_USER_ONLY or not?

I am not sure at this point.


> 
> 
>>  }
>>  
>>  #define AARCH64_CORE_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
>> @@ -1545,7 +1545,7 @@ 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);
>> +    arm_cpu_do_interrupt(cs);
>>      qemu_mutex_unlock_iothread();
>>  
>>      return false;
>> -- 
>> 2.26.2
>>
> 

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

[Peter Maydell]

On Mon, 7 Dec 2020 at 18:08, Claudio Fontana <cfontana@suse.de> wrote:
> what about also the existing code with qemu-arm (user mode)?
>
> In that case do_interrupt is not set at all in target/arm/cpu.c, since it's protected by #ifndef CONFIG_USER_ONLY
>
> Did we have a potential NULL pointer trying to be dereferenced there?

No, because in user-mode there are never any interrupts or
exceptions invoked this way. The code in these methods is
strictly system-emulation only.

> Commit 0adf7d3cc3f724e1e9ce5aaa008bd9daeb90f19 says:
>
>  target-arm: do not set do_interrupt handlers for ARM and AArch64 user modes
>
>  User mode emulation should never get interrupts and thus should not
>  use the system emulation exception handler function.
>
> --
>
> But this was 2014. Is the comment above true today?

Yes.

> Looking at this commit in 2017, it does not seem to me to be the case:
>
> commit 17b50b0c299f1266578b01f7134810362418ac2e
> Author: Pavel Dovgaluk <Pavel.Dovgaluk@ispras.ru>
> Date:   Tue Nov 14 11:18:18 2017 +0300
>
>     cpu-exec: avoid cpu_exec_nocache infinite loop with record/replay
>
>     This patch
>     [...]
>     Second, try to cause the exception at the beginning of
>     cpu_handle_exception, and exit immediately if the TB cannot
>     execute.  With this change, interrupts are processed and
>     cpu_exec_nocache can make process.

This code only invokes cc->do_interrupt() in CONFIG_USER_ONLY
if TARGET_I386 is true. i386 does this stuff in a weird way
that's different to all the other target architectures.
(One day we should fix this inconsistency I suppose.)

> Is cc->do_interrupt supposed to be !CONFIG_USER_ONLY or not?

It's !CONFIG_USER_ONLY.

thanks
-- PMM

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

[Claudio Fontana]

On 12/7/20 7:14 PM, Peter Maydell wrote:
> On Mon, 7 Dec 2020 at 18:08, Claudio Fontana <cfontana@suse.de> wrote:
>> what about also the existing code with qemu-arm (user mode)?
>>
>> In that case do_interrupt is not set at all in target/arm/cpu.c, since it's protected by #ifndef CONFIG_USER_ONLY
>>
>> Did we have a potential NULL pointer trying to be dereferenced there?
> 
> No, because in user-mode there are never any interrupts or
> exceptions invoked this way. The code in these methods is
> strictly system-emulation only.
> 
>> Commit 0adf7d3cc3f724e1e9ce5aaa008bd9daeb90f19 says:
>>
>>  target-arm: do not set do_interrupt handlers for ARM and AArch64 user modes
>>
>>  User mode emulation should never get interrupts and thus should not
>>  use the system emulation exception handler function.
>>
>> --
>>
>> But this was 2014. Is the comment above true today?
> 
> Yes.
> 
>> Looking at this commit in 2017, it does not seem to me to be the case:
>>
>> commit 17b50b0c299f1266578b01f7134810362418ac2e
>> Author: Pavel Dovgaluk <Pavel.Dovgaluk@ispras.ru>
>> Date:   Tue Nov 14 11:18:18 2017 +0300
>>
>>     cpu-exec: avoid cpu_exec_nocache infinite loop with record/replay
>>
>>     This patch
>>     [...]
>>     Second, try to cause the exception at the beginning of
>>     cpu_handle_exception, and exit immediately if the TB cannot
>>     execute.  With this change, interrupts are processed and
>>     cpu_exec_nocache can make process.
> 
> This code only invokes cc->do_interrupt() in CONFIG_USER_ONLY
> if TARGET_I386 is true. i386 does this stuff in a weird way
> that's different to all the other target architectures.
> (One day we should fix this inconsistency I suppose.)
> 
>> Is cc->do_interrupt supposed to be !CONFIG_USER_ONLY or not?
> 
> It's !CONFIG_USER_ONLY.
> 
> thanks
> -- PMM
> 

Ah right, I somehow missed the #if defined(TARGET_I386),

thanks!