Re: [Qemu-devel] [PATCH 0/6] Drop the irredeemably racy cpu_unlink_tb()

[Peter Maydell]

Ping!

thanks
-- PMM

On 23 February 2013 02:09, Peter Maydell <address@hidden> wrote:
> This patch series gets rid of cpu_unlink_tb(), which is irredeemably
> racy, since it modifies the TB graph with no locking from other
> threads, signal handlers, etc etc. (The signal handler case is
> why you can't just fix this with more locks.) Instead we take the
> much simpler approach of setting a flag for the CPU when we want
> it to stop executing TBs, and generate code to check the flag at
> the start of every TB. The raciness is easiest to provoke with
> multithreaded linux-user guests but it is I think also a risk
> in system emulation mode.
>
> This fixes the crashes seen in LP:668799; however there are another
> class of crashes described in LP:1098729 which stem from the fact
> that in linux-user with a multithreaded guest all threads will
> use and modify the same global TCG date structures (including the
> generated code buffer) without any kind of locking. This means that
> multithreaded guest binaries are still in the "unsupported" category.
>
> Patch 1 has been on the list before, but I improved the comment
> a bit [no code changes from the previously posted version] which
> is why I haven't given it rth's reviewed-by tag.
>
> The ENV_OFFSET macros patch is one from an old patch series
> of Andreas'; we need some way in generic code to find the offset
> of a CPUState field from the CPUArchState pointer. (The commit
> message is mine, though.)
>
> I've tested this in various ways, and also ran a popular embedded
> benchmark. This is the worst case for this change, since it is
> compute intensive code in a situation that shouldn't have too
> many interrupts; the slowdown varied from benchmark to benchmark
> but was generally something like 3-5%. I think that's an acceptably
> small hit to gain actual correctness and non-crashiness :-)
>
>
> Andreas Färber (1):
>   cpu: Introduce ENV_OFFSET macros
>
> Peter Maydell (5):
>   tcg: Document tcg_qemu_tb_exec() and provide constants for low bit uses
>   cpu-exec: wrap tcg_qemu_tb_exec() in a fn to restore the PC
>   Handle CPU interrupts by inline checking of a flag
>   translate-all.c: Remove cpu_unlink_tb()
>   gen-icount.h: Rename gen_icount_start/end to gen_tb_start/end
>
>  cpu-exec.c                    |   59 +++++++++++++++++++++++++--------
>  exec.c                        |    2 +-
>  include/exec/gen-icount.h     |   18 ++++++++--
>  include/qom/cpu.h             |    3 ++
>  target-alpha/cpu-qom.h        |    1 +
>  target-alpha/translate.c      |    4 +--
>  target-arm/cpu-qom.h          |    2 ++
>  target-arm/translate.c        |    4 +--
>  target-cris/cpu-qom.h         |    1 +
>  target-cris/translate.c       |    4 +--
>  target-i386/cpu-qom.h         |    1 +
>  target-i386/translate.c       |    4 +--
>  target-lm32/cpu-qom.h         |    1 +
>  target-lm32/translate.c       |    4 +--
>  target-m68k/cpu-qom.h         |    1 +
>  target-m68k/translate.c       |    4 +--
>  target-microblaze/cpu-qom.h   |    1 +
>  target-microblaze/translate.c |    4 +--
>  target-mips/cpu-qom.h         |    1 +
>  target-mips/translate.c       |    4 +--
>  target-openrisc/cpu.h         |    2 ++
>  target-openrisc/translate.c   |    4 +--
>  target-ppc/cpu-qom.h          |    3 +-
>  target-ppc/translate.c        |    4 +--
>  target-s390x/cpu-qom.h        |    1 +
>  target-s390x/translate.c      |    4 +--
>  target-sh4/cpu-qom.h          |    1 +
>  target-sh4/translate.c        |    4 +--
>  target-sparc/cpu-qom.h        |    1 +
>  target-sparc/translate.c      |    4 +--
>  target-unicore32/cpu-qom.h    |    1 +
>  target-unicore32/translate.c  |    4 +--
>  target-xtensa/cpu-qom.h       |    1 +
>  target-xtensa/translate.c     |    4 +--
>  tcg/tcg.h                     |   49 ++++++++++++++++++++++++++-
>  translate-all.c               |   73 
> ++---------------------------------------
>  36 files changed, 162 insertions(+), 121 deletions(-)
>
> --
> 1.7.9.5
>
>