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

[BALATON Zoltan]

On Mon, 30 Mar 2020, 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.)

Even though there's only a single CPU on sam460ex and PCIe is mostly 
unused, with this patch I could no more reproduce a problem that we had 
before with some programs crashing within guest under AmigaOS for unknown 
reason. That problem happened randomly (although I could reproduce it 
before) so I'm not sure if this fixed it or something else (more likely 
commit 235352ee6e) or will just resurface later but at least this seems to 
work so

Tested-by: BALATON Zoltan <address@hidden>

Thanks for fixing it.

> 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 <address@hidden>
> ---
> 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.

I think we needed to add locking here because it asserted otherwise but I 
don't remember the details now.

Regards,
BALATON Zoltan

> ---
> 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);