Changing the IP/IM registers may cause interrupts, so hold the BQL.

Cc: qemu-stable@nongnu.org
Signed-off-by: Michael Walle <michael@walle.cc>
---
 target/lm32/gdbstub.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/lm32/gdbstub.c b/target/lm32/gdbstub.c
index cf929dd392..dac9418a2b 100644
--- a/target/lm32/gdbstub.c
+++ b/target/lm32/gdbstub.c
@@ -18,6 +18,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "qemu-common.h"
 #include "cpu.h"
 #include "exec/gdbstub.h"
@@ -82,10 +83,14 @@ int lm32_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
             env->ie = tmp;
             break;
         case 37:
+            qemu_mutex_lock_iothread();
             lm32_pic_set_im(env->pic_state, tmp);
+            qemu_mutex_unlock_iothread();
             break;
         case 38:
+            qemu_mutex_lock_iothread();
             lm32_pic_set_ip(env->pic_state, tmp);
+            qemu_mutex_unlock_iothread();
             break;
         }
     }
-- 
2.11.0

On 21 May 2018 at 13:21, Michael Walle <michael@walle.cc> wrote:
> Changing the IP/IM registers may cause interrupts, so hold the BQL.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  target/lm32/gdbstub.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/target/lm32/gdbstub.c b/target/lm32/gdbstub.c
> index cf929dd392..dac9418a2b 100644
> --- a/target/lm32/gdbstub.c
> +++ b/target/lm32/gdbstub.c
> @@ -18,6 +18,7 @@
>   * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
>  #include "qemu-common.h"
>  #include "cpu.h"
>  #include "exec/gdbstub.h"
> @@ -82,10 +83,14 @@ int lm32_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>              env->ie = tmp;
>              break;
>          case 37:
> +            qemu_mutex_lock_iothread();
>              lm32_pic_set_im(env->pic_state, tmp);
> +            qemu_mutex_unlock_iothread();
>              break;
>          case 38:
> +            qemu_mutex_lock_iothread();
>              lm32_pic_set_ip(env->pic_state, tmp);
> +            qemu_mutex_unlock_iothread();
>              break;
>          }
>      }

Are you sure this is necessary? I would have expected the gdbstub to
be operating under the qemu lock anyway.

thanks
-- PMM

On 21 May 2018 at 13:25, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 21 May 2018 at 13:21, Michael Walle <michael@walle.cc> wrote:
>> Changing the IP/IM registers may cause interrupts, so hold the BQL.
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  target/lm32/gdbstub.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/target/lm32/gdbstub.c b/target/lm32/gdbstub.c
>> index cf929dd392..dac9418a2b 100644
>> --- a/target/lm32/gdbstub.c
>> +++ b/target/lm32/gdbstub.c
>> @@ -18,6 +18,7 @@
>>   * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>   */
>>  #include "qemu/osdep.h"
>> +#include "qemu/main-loop.h"
>>  #include "qemu-common.h"
>>  #include "cpu.h"
>>  #include "exec/gdbstub.h"
>> @@ -82,10 +83,14 @@ int lm32_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>>              env->ie = tmp;
>>              break;
>>          case 37:
>> +            qemu_mutex_lock_iothread();
>>              lm32_pic_set_im(env->pic_state, tmp);
>> +            qemu_mutex_unlock_iothread();
>>              break;
>>          case 38:
>> +            qemu_mutex_lock_iothread();
>>              lm32_pic_set_ip(env->pic_state, tmp);
>> +            qemu_mutex_unlock_iothread();
>>              break;
>>          }
>>      }
>
> Are you sure this is necessary? I would have expected the gdbstub to
> be operating under the qemu lock anyway.

...experimentation suggests that the gdbstub is called via
chardev write events which are triggered by glib_pollfds_poll(),
which is called by os_host_main_loop_wait() only when it holds
the iothread lock.

thanks
-- PMM

Am 2018-05-21 14:25, schrieb Peter Maydell:
> On 21 May 2018 at 13:21, Michael Walle <michael@walle.cc> wrote:
>> Changing the IP/IM registers may cause interrupts, so hold the BQL.
>> 
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  target/lm32/gdbstub.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/target/lm32/gdbstub.c b/target/lm32/gdbstub.c
>> index cf929dd392..dac9418a2b 100644
>> --- a/target/lm32/gdbstub.c
>> +++ b/target/lm32/gdbstub.c
>> @@ -18,6 +18,7 @@
>>   * License along with this library; if not, see 
>> <http://www.gnu.org/licenses/>.
>>   */
>>  #include "qemu/osdep.h"
>> +#include "qemu/main-loop.h"
>>  #include "qemu-common.h"
>>  #include "cpu.h"
>>  #include "exec/gdbstub.h"
>> @@ -82,10 +83,14 @@ int lm32_cpu_gdb_write_register(CPUState *cs, 
>> uint8_t *mem_buf, int n)
>>              env->ie = tmp;
>>              break;
>>          case 37:
>> +            qemu_mutex_lock_iothread();
>>              lm32_pic_set_im(env->pic_state, tmp);
>> +            qemu_mutex_unlock_iothread();
>>              break;
>>          case 38:
>> +            qemu_mutex_lock_iothread();
>>              lm32_pic_set_ip(env->pic_state, tmp);
>> +            qemu_mutex_unlock_iothread();
>>              break;
>>          }
>>      }
> 
> Are you sure this is necessary? I would have expected the gdbstub to
> be operating under the qemu lock anyway.


You're right. The gdbstub is already holding the lock. So i'll drop this 
and send the pull request right now.

-michael