1. Patchew
  2. QEMU
  3. View series

[PATCH] target/i386: raise FERR interrupt with iothread locked

Paolo Bonzini posted 1 patch 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230829152604.101542-1-pbonzini@redhat.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>
There is a newer version of this series
target/i386/tcg/sysemu/fpu_helper.c | 2 ++
1 file changed, 2 insertions(+)
[PATCH] target/i386: raise FERR interrupt with iothread locked
Posted by Paolo Bonzini 8 months ago 
Otherwise tcg_handle_interrupt() triggers an assertion failure:

  #5  0x0000555555c97369 in tcg_handle_interrupt (cpu=0x555557434cb0, mask=2) at ../accel/tcg/tcg-accel-ops.c:83
  #6  tcg_handle_interrupt (cpu=0x555557434cb0, mask=2) at ../accel/tcg/tcg-accel-ops.c:81
  #7  0x0000555555b4d58b in pic_irq_request (opaque=<optimized out>, irq=<optimized out>, level=1) at ../hw/i386/x86.c:555
  #8  0x0000555555b4f218 in gsi_handler (opaque=0x5555579423d0, n=13, level=1) at ../hw/i386/x86.c:611
  #9  0x00007fffa42bde14 in code_gen_buffer ()
  #10 0x0000555555c724bb in cpu_tb_exec (cpu=cpu@entry=0x555557434cb0, itb=<optimized out>, tb_exit=tb_exit@entry=0x7fffe9bfd658) at ../accel/tcg/cpu-exec.c:457

Cc: qemu-stable@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1808
Reported-by: NyanCatTW1 <https://gitlab.com/a0939712328>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/sysemu/fpu_helper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/i386/tcg/sysemu/fpu_helper.c b/target/i386/tcg/sysemu/fpu_helper.c
index 1c3610da3b9..fd8cc72a026 100644
--- a/target/i386/tcg/sysemu/fpu_helper.c
+++ b/target/i386/tcg/sysemu/fpu_helper.c
@@ -31,7 +31,9 @@ void x86_register_ferr_irq(qemu_irq irq)
 void fpu_check_raise_ferr_irq(CPUX86State *env)
 {
     if (ferr_irq && !(env->hflags2 & HF2_IGNNE_MASK)) {
+        qemu_mutex_lock_iothread();
         qemu_irq_raise(ferr_irq);
+        qemu_mutex_unlock_iothread();
         return;
     }
 }
-- 
2.41.0
Re: [PATCH] target/i386: raise FERR interrupt with iothread locked
Posted by Philippe Mathieu-Daudé 8 months ago 
On 29/8/23 17:25, Paolo Bonzini wrote:
> Otherwise tcg_handle_interrupt() triggers an assertion failure:
> 
>    #5  0x0000555555c97369 in tcg_handle_interrupt (cpu=0x555557434cb0, mask=2) at ../accel/tcg/tcg-accel-ops.c:83
>    #6  tcg_handle_interrupt (cpu=0x555557434cb0, mask=2) at ../accel/tcg/tcg-accel-ops.c:81
>    #7  0x0000555555b4d58b in pic_irq_request (opaque=<optimized out>, irq=<optimized out>, level=1) at ../hw/i386/x86.c:555
>    #8  0x0000555555b4f218 in gsi_handler (opaque=0x5555579423d0, n=13, level=1) at ../hw/i386/x86.c:611
>    #9  0x00007fffa42bde14 in code_gen_buffer ()
>    #10 0x0000555555c724bb in cpu_tb_exec (cpu=cpu@entry=0x555557434cb0, itb=<optimized out>, tb_exit=tb_exit@entry=0x7fffe9bfd658) at ../accel/tcg/cpu-exec.c:457
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1808
> Reported-by: NyanCatTW1 <https://gitlab.com/a0939712328>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/tcg/sysemu/fpu_helper.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/target/i386/tcg/sysemu/fpu_helper.c b/target/i386/tcg/sysemu/fpu_helper.c
> index 1c3610da3b9..fd8cc72a026 100644
> --- a/target/i386/tcg/sysemu/fpu_helper.c
> +++ b/target/i386/tcg/sysemu/fpu_helper.c
> @@ -31,7 +31,9 @@ void x86_register_ferr_irq(qemu_irq irq)
>   void fpu_check_raise_ferr_irq(CPUX86State *env)
>   {
>       if (ferr_irq && !(env->hflags2 & HF2_IGNNE_MASK)) {
> +        qemu_mutex_lock_iothread();
>           qemu_irq_raise(ferr_irq);
> +        qemu_mutex_unlock_iothread();
>           return;
>       }
>   }

OK, so:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Maybe for completeness, squash this? :

-- >8 --
diff --git a/target/i386/tcg/sysemu/fpu_helper.c 
b/target/i386/tcg/sysemu/fpu_helper.c
index 1c3610da3b..320bd69f43 100644
--- a/target/i386/tcg/sysemu/fpu_helper.c
+++ b/target/i386/tcg/sysemu/fpu_helper.c
@@ -45,6 +45,8 @@ void cpu_clear_ignne(void)
  void cpu_set_ignne(void)
  {
      CPUX86State *env = &X86_CPU(first_cpu)->env;
+
+    g_assert(qemu_mutex_iothread_locked());
      env->hflags2 |= HF2_IGNNE_MASK;
      /*
---

Somehow similar pattern so what about MIPS MTC0?:

mtc0_compare() ->
   helper_mtc0_compare() ->
     cpu_mips_store_compare() ->
       qemu_irq_lower()

Regards,

Phil.
Re: [PATCH] target/i386: raise FERR interrupt with iothread locked
Posted by Paolo Bonzini 8 months ago 
On Tue, Aug 29, 2023 at 5:46 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 29/8/23 17:25, Paolo Bonzini wrote:
> > Otherwise tcg_handle_interrupt() triggers an assertion failure:
> >
> >    #5  0x0000555555c97369 in tcg_handle_interrupt (cpu=0x555557434cb0, mask=2) at ../accel/tcg/tcg-accel-ops.c:83
> >    #6  tcg_handle_interrupt (cpu=0x555557434cb0, mask=2) at ../accel/tcg/tcg-accel-ops.c:81
> >    #7  0x0000555555b4d58b in pic_irq_request (opaque=<optimized out>, irq=<optimized out>, level=1) at ../hw/i386/x86.c:555
> >    #8  0x0000555555b4f218 in gsi_handler (opaque=0x5555579423d0, n=13, level=1) at ../hw/i386/x86.c:611
> >    #9  0x00007fffa42bde14 in code_gen_buffer ()
> >    #10 0x0000555555c724bb in cpu_tb_exec (cpu=cpu@entry=0x555557434cb0, itb=<optimized out>, tb_exit=tb_exit@entry=0x7fffe9bfd658) at ../accel/tcg/cpu-exec.c:457
> >
> > Cc: qemu-stable@nongnu.org
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1808
> > Reported-by: NyanCatTW1 <https://gitlab.com/a0939712328>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   target/i386/tcg/sysemu/fpu_helper.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/target/i386/tcg/sysemu/fpu_helper.c b/target/i386/tcg/sysemu/fpu_helper.c
> > index 1c3610da3b9..fd8cc72a026 100644
> > --- a/target/i386/tcg/sysemu/fpu_helper.c
> > +++ b/target/i386/tcg/sysemu/fpu_helper.c
> > @@ -31,7 +31,9 @@ void x86_register_ferr_irq(qemu_irq irq)
> >   void fpu_check_raise_ferr_irq(CPUX86State *env)
> >   {
> >       if (ferr_irq && !(env->hflags2 & HF2_IGNNE_MASK)) {
> > +        qemu_mutex_lock_iothread();
> >           qemu_irq_raise(ferr_irq);
> > +        qemu_mutex_unlock_iothread();
> >           return;
> >       }
> >   }
>
> OK, so:
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Actually Richard has a better (though also slightly incomplete) patch
in the bug, so I'll send v2.

>   void cpu_set_ignne(void)
>   {
>       CPUX86State *env = &X86_CPU(first_cpu)->env;
> +
> +    g_assert(qemu_mutex_iothread_locked());

qemu_irq_lower() is fine because it doesn't result in a call to
tcg_handle_interrupt().

Paolo

> Somehow similar pattern so what about MIPS MTC0?:
>
> mtc0_compare() ->
>    helper_mtc0_compare() ->
>      cpu_mips_store_compare() ->
>        qemu_irq_lower()

Lower is a bit safer
Re: [PATCH] target/i386: raise FERR interrupt with iothread locked
Posted by Peter Maydell 8 months ago 
On Tue, 29 Aug 2023 at 17:59, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On Tue, Aug 29, 2023 at 5:46 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
> >
> > On 29/8/23 17:25, Paolo Bonzini wrote:
> > > Otherwise tcg_handle_interrupt() triggers an assertion failure:
> > >
> > >    #5  0x0000555555c97369 in tcg_handle_interrupt (cpu=0x555557434cb0, mask=2) at ../accel/tcg/tcg-accel-ops.c:83
> > >    #6  tcg_handle_interrupt (cpu=0x555557434cb0, mask=2) at ../accel/tcg/tcg-accel-ops.c:81
> > >    #7  0x0000555555b4d58b in pic_irq_request (opaque=<optimized out>, irq=<optimized out>, level=1) at ../hw/i386/x86.c:555
> > >    #8  0x0000555555b4f218 in gsi_handler (opaque=0x5555579423d0, n=13, level=1) at ../hw/i386/x86.c:611
> > >    #9  0x00007fffa42bde14 in code_gen_buffer ()
> > >    #10 0x0000555555c724bb in cpu_tb_exec (cpu=cpu@entry=0x555557434cb0, itb=<optimized out>, tb_exit=tb_exit@entry=0x7fffe9bfd658) at ../accel/tcg/cpu-exec.c:457
> > >
> > > Cc: qemu-stable@nongnu.org
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1808
> > > Reported-by: NyanCatTW1 <https://gitlab.com/a0939712328>
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > ---
> > >   target/i386/tcg/sysemu/fpu_helper.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > >
> > > diff --git a/target/i386/tcg/sysemu/fpu_helper.c b/target/i386/tcg/sysemu/fpu_helper.c
> > > index 1c3610da3b9..fd8cc72a026 100644
> > > --- a/target/i386/tcg/sysemu/fpu_helper.c
> > > +++ b/target/i386/tcg/sysemu/fpu_helper.c
> > > @@ -31,7 +31,9 @@ void x86_register_ferr_irq(qemu_irq irq)
> > >   void fpu_check_raise_ferr_irq(CPUX86State *env)
> > >   {
> > >       if (ferr_irq && !(env->hflags2 & HF2_IGNNE_MASK)) {
> > > +        qemu_mutex_lock_iothread();
> > >           qemu_irq_raise(ferr_irq);
> > > +        qemu_mutex_unlock_iothread();
> > >           return;
> > >       }
> > >   }
> >
> > OK, so:
> >
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> Actually Richard has a better (though also slightly incomplete) patch
> in the bug, so I'll send v2.
>
> >   void cpu_set_ignne(void)
> >   {
> >       CPUX86State *env = &X86_CPU(first_cpu)->env;
> > +
> > +    g_assert(qemu_mutex_iothread_locked());
>
> qemu_irq_lower() is fine because it doesn't result in a call to
> tcg_handle_interrupt().

It does potentially result in calling code in a device model
that assumes that it is protected by the iothread lock,
though...

-- PMM

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.