Re: [PATCH for-5.0] hw/ppc/ppc440_uc.c: Remove incorrect iothread locking from dcr_write_pcie()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2395 bytes --]

On Mon, Mar 30, 2020 at 01:52:28PM +0100, Peter Maydell wrote:
> In dcr_write_pcie() we take the iothread lock around a call to
> pcie_host_mmcfg_udpate().  This is an incorrect attempt to deal with
> the bug fixed in commit 235352ee6e73d7716, where we were not taking
> the iothread lock before calling device dcr read/write functions.
> (It's not sufficient locking, because although the other cases in the
> switch statement won't assert, there is no locking which prevents
> multiple guest CPUs from trying to access the PPC460EXPCIEState
> struct at the same time and corrupting data.)
> 
> Unfortunately with commit 235352ee6e73d7716 we are now trying
> to recursively take the iothread lock, which will assert:
> 
>   $ qemu-system-ppc -M sam460ex --display none
>   **
>   ERROR:/home/petmay01/linaro/qemu-from-laptop/qemu/cpus.c:1830:qemu_mutex_lock_iothread_impl: assertion failed: (!qemu_mutex_iothread_locked())
>   Aborted (core dumped)
> 
> Remove the locking within dcr_write_pcie().
> 
> Fixes: 235352ee6e73d7716
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I did a grep of hw/ppc and didn't see anything else that was doing
> its own locking inside a dcr read/write fn.
> ---

Applied to ppc-for-5.0, thanks.

>  hw/ppc/ppc440_uc.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
> index d5ea962249f..b30e093cbb0 100644
> --- a/hw/ppc/ppc440_uc.c
> +++ b/hw/ppc/ppc440_uc.c
> @@ -13,7 +13,6 @@
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "qemu/log.h"
> -#include "qemu/main-loop.h"
>  #include "qemu/module.h"
>  #include "cpu.h"
>  #include "hw/irq.h"
> @@ -1183,9 +1182,7 @@ static void dcr_write_pcie(void *opaque, int dcrn, uint32_t val)
>      case PEGPL_CFGMSK:
>          s->cfg_mask = val;
>          size = ~(val & 0xfffffffe) + 1;
> -        qemu_mutex_lock_iothread();
>          pcie_host_mmcfg_update(PCIE_HOST_BRIDGE(s), val & 1, s->cfg_base, size);
> -        qemu_mutex_unlock_iothread();
>          break;
>      case PEGPL_MSGBAH:
>          s->msg_base = ((uint64_t)val << 32) | (s->msg_base & 0xffffffff);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]