[Qemu-devel] [PATCH v2 0/2] BQL patches for the lm32 target

[Qemu-devel] [PATCH v2 0/2] BQL patches for the lm32 target

[Michael Walle]

Whoops, forget the include patch chunk for the second patch.

I'll send a pull request next week if there are no comments on the
patches.

since v1:
  add missing #include

Michael Walle (2):
  lm32: take BQL before writing IP/IM register
  target/lm32: hold BQL in gdbstub

 target/lm32/gdbstub.c   | 5 +++++
 target/lm32/op_helper.c | 4 ++++
 2 files changed, 9 insertions(+)

-- 
2.11.0

[Qemu-devel] [PATCH v2 1/2] lm32: take BQL before writing IP/IM register

[Michael Walle]

Writing to these registers may raise an interrupt request. Actually,
this prevents the milkymist board from starting.

Cc: qemu-stable@nongnu.org
Signed-off-by: Michael Walle <michael@walle.cc>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/lm32/op_helper.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/lm32/op_helper.c b/target/lm32/op_helper.c
index 577f8306e3..234d55e056 100644
--- a/target/lm32/op_helper.c
+++ b/target/lm32/op_helper.c
@@ -102,12 +102,16 @@ void HELPER(wcsr_dc)(CPULM32State *env, uint32_t dc)
 
 void HELPER(wcsr_im)(CPULM32State *env, uint32_t im)
 {
+    qemu_mutex_lock_iothread();
     lm32_pic_set_im(env->pic_state, im);
+    qemu_mutex_unlock_iothread();
 }
 
 void HELPER(wcsr_ip)(CPULM32State *env, uint32_t im)
 {
+    qemu_mutex_lock_iothread();
     lm32_pic_set_ip(env->pic_state, im);
+    qemu_mutex_unlock_iothread();
 }
 
 void HELPER(wcsr_jtx)(CPULM32State *env, uint32_t jtx)
-- 
2.11.0

[Qemu-devel] [PATCH v2 2/2] target/lm32: hold BQL in gdbstub

[Michael Walle]

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

Re: [Qemu-devel] [PATCH v2 2/2] target/lm32: hold BQL in gdbstub

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/2] target/lm32: hold BQL in gdbstub

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v2 2/2] target/lm32: hold BQL in gdbstub

[Michael Walle]

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