Re: [Qemu-devel] [PATCH v2 3/6] hw/mips_int: hold BQL for all interrupt requests

2018-04-04 Thread Paolo Bonzini
On 04/04/2018 15:44, Alex Bennée wrote:
 Signed-off-by: Miodrag Dinic 
 Signed-off-by: Aleksandar Markovic 
>> Is this actually necessary?  What paths are not taking the lock?
> Helpers functions have to manually take the lock. AIUI from this patch
> the if (locked) dance allows a single function to be used which may
> trigger an IRQ from both helpers (no automatic locking) and hw emulation
> (locked by default).

This makes it harder to understand which paths actually need to take the
lock, and to split the lock in the future.  We do it in some cases, but
in general defining "*_locked" or "*_unlocked" functions is easier on
the brain.

Thanks,

Paolo



Re: [Qemu-devel] [PATCH v2 3/6] hw/mips_int: hold BQL for all interrupt requests

2018-04-04 Thread Alex Bennée

Paolo Bonzini  writes:

> On 04/04/2018 12:23, Alex Bennée wrote:
>>
>>> From: Aleksandar Markovic 
>>>
>>> Make sure BQL is held for all interrupt requests.
>>>
>>> For MTTCG-enabled configurations, handling soft and hard interrupts
>>> between vCPUs must be properly locked. By acquiring BQL, make sure
>>> all paths triggering an IRQ are synchronized.
>>>
>>> Signed-off-by: Miodrag Dinic 
>>> Signed-off-by: Aleksandar Markovic 
>
> Is this actually necessary?  What paths are not taking the lock?

Helpers functions have to manually take the lock. AIUI from this patch
the if (locked) dance allows a single function to be used which may
trigger an IRQ from both helpers (no automatic locking) and hw emulation
(locked by default).

>
> Thanks,
>
> Paolo


--
Alex Bennée



Re: [Qemu-devel] [PATCH v2 3/6] hw/mips_int: hold BQL for all interrupt requests

2018-04-04 Thread Paolo Bonzini
On 04/04/2018 12:23, Alex Bennée wrote:
> 
>> From: Aleksandar Markovic 
>>
>> Make sure BQL is held for all interrupt requests.
>>
>> For MTTCG-enabled configurations, handling soft and hard interrupts
>> between vCPUs must be properly locked. By acquiring BQL, make sure
>> all paths triggering an IRQ are synchronized.
>>
>> Signed-off-by: Miodrag Dinic 
>> Signed-off-by: Aleksandar Markovic 

Is this actually necessary?  What paths are not taking the lock?

Thanks,

Paolo



Re: [Qemu-devel] [PATCH v2 3/6] hw/mips_int: hold BQL for all interrupt requests

2018-04-04 Thread Alex Bennée

Aleksandar Markovic  writes:

> From: Aleksandar Markovic 
>
> Make sure BQL is held for all interrupt requests.
>
> For MTTCG-enabled configurations, handling soft and hard interrupts
> between vCPUs must be properly locked. By acquiring BQL, make sure
> all paths triggering an IRQ are synchronized.
>
> Signed-off-by: Miodrag Dinic 
> Signed-off-by: Aleksandar Markovic 

Reviewed-by: Alex Bennée 

> ---
>  hw/mips/mips_int.c  | 12 
>  target/mips/op_helper.c | 21 +++--
>  2 files changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
> index 48192d2..5ddeb15 100644
> --- a/hw/mips/mips_int.c
> +++ b/hw/mips/mips_int.c
> @@ -21,6 +21,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
>  #include "hw/hw.h"
>  #include "hw/mips/cpudevs.h"
>  #include "cpu.h"
> @@ -32,10 +33,17 @@ static void cpu_mips_irq_request(void *opaque, int irq, 
> int level)
>  MIPSCPU *cpu = opaque;
>  CPUMIPSState *env = &cpu->env;
>  CPUState *cs = CPU(cpu);
> +bool locked = false;
>
>  if (irq < 0 || irq > 7)
>  return;
>
> +/* Make sure locking works even if BQL is already held by the caller */
> +if (!qemu_mutex_iothread_locked()) {
> +locked = true;
> +qemu_mutex_lock_iothread();
> +}
> +
>  if (level) {
>  env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
>
> @@ -56,6 +64,10 @@ static void cpu_mips_irq_request(void *opaque, int irq, 
> int level)
>  } else {
>  cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
>  }
> +
> +if (locked) {
> +qemu_mutex_unlock_iothread();
> +}
>  }
>
>  void cpu_mips_irq_init_cpu(MIPSCPU *cpu)
> diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
> index 44a9b06..6bd8e59 100644
> --- a/target/mips/op_helper.c
> +++ b/target/mips/op_helper.c
> @@ -17,7 +17,6 @@
>   * License along with this library; if not, see 
> .
>   */
>  #include "qemu/osdep.h"
> -#include "qemu/main-loop.h"
>  #include "cpu.h"
>  #include "internal.h"
>  #include "qemu/host-utils.h"
> @@ -809,11 +808,7 @@ target_ulong helper_mftc0_tcschefback(CPUMIPSState *env)
>
>  target_ulong helper_mfc0_count(CPUMIPSState *env)
>  {
> -int32_t count;
> -qemu_mutex_lock_iothread();
> -count = (int32_t) cpu_mips_get_count(env);
> -qemu_mutex_unlock_iothread();
> -return count;
> +return (int32_t)cpu_mips_get_count(env);
>  }
>
>  target_ulong helper_mftc0_entryhi(CPUMIPSState *env)
> @@ -1388,9 +1383,7 @@ void helper_mtc0_hwrena(CPUMIPSState *env, target_ulong 
> arg1)
>
>  void helper_mtc0_count(CPUMIPSState *env, target_ulong arg1)
>  {
> -qemu_mutex_lock_iothread();
>  cpu_mips_store_count(env, arg1);
> -qemu_mutex_unlock_iothread();
>  }
>
>  void helper_mtc0_entryhi(CPUMIPSState *env, target_ulong arg1)
> @@ -1439,9 +1432,7 @@ void helper_mttc0_entryhi(CPUMIPSState *env, 
> target_ulong arg1)
>
>  void helper_mtc0_compare(CPUMIPSState *env, target_ulong arg1)
>  {
> -qemu_mutex_lock_iothread();
>  cpu_mips_store_compare(env, arg1);
> -qemu_mutex_unlock_iothread();
>  }
>
>  void helper_mtc0_status(CPUMIPSState *env, target_ulong arg1)
> @@ -1495,9 +1486,7 @@ void helper_mtc0_srsctl(CPUMIPSState *env, target_ulong 
> arg1)
>
>  void helper_mtc0_cause(CPUMIPSState *env, target_ulong arg1)
>  {
> -qemu_mutex_lock_iothread();
>  cpu_mips_store_cause(env, arg1);
> -qemu_mutex_unlock_iothread();
>  }
>
>  void helper_mttc0_cause(CPUMIPSState *env, target_ulong arg1)
> @@ -2339,16 +2328,12 @@ target_ulong helper_rdhwr_synci_step(CPUMIPSState 
> *env)
>
>  target_ulong helper_rdhwr_cc(CPUMIPSState *env)
>  {
> -int32_t count;
>  check_hwrena(env, 2, GETPC());
>  #ifdef CONFIG_USER_ONLY
> -count = env->CP0_Count;
> +return env->CP0_Count;
>  #else
> -qemu_mutex_lock_iothread();
> -count = (int32_t)cpu_mips_get_count(env);
> -qemu_mutex_unlock_iothread();
> +return (int32_t)cpu_mips_get_count(env);
>  #endif
> -return count;
>  }
>
>  target_ulong helper_rdhwr_ccres(CPUMIPSState *env)


--
Alex Bennée