Re: [Qemu-devel] [PATCH 2/4] bsd-user: move arch/OS dependent code out of main.c

[Peter Maydell]

On 20 June 2014 01:19, Sean Bruno <address@hidden> wrote:
> From: Stacey Son <address@hidden>
>
> This change moves the cpu initialization and main loop code from
> main.c to the OS and arch dependent directories. This eliminates
> many of the #ifdef's in main.c. The cpu initialization and loop
> code is now located in the arch directory along with target arch
> support code.
>
> Signed-off-by: Sean Bruno <address@hidden>
> ---
>  bsd-user/Makefile.objs                 |   2 +-
>  bsd-user/elfload.c                     |   2 +-
>  bsd-user/freebsd/host_os.h             |  46 ++
>  bsd-user/freebsd/target_os_vmparam.h   |  23 +
>  bsd-user/i386/target_arch.h            |  13 +
>  bsd-user/i386/target_arch_cpu.c        |  79 +++
>  bsd-user/i386/target_arch_cpu.h        | 300 +++++++++++
>  bsd-user/i386/target_arch_vmparam.h    |  28 +
>  bsd-user/i386/target_signal.h          |   6 -
>  bsd-user/main.c                        | 927 
> +++++++--------------------------
>  bsd-user/mmap.c                        |   2 +-
>  bsd-user/netbsd/host_os.h              |  31 ++
>  bsd-user/netbsd/os-strace.h            |   2 +-
>  bsd-user/netbsd/target_os_vmparam.h    |  23 +
>  bsd-user/openbsd/host_os.h             |  31 ++
>  bsd-user/openbsd/os-strace.h           |   2 +-
>  bsd-user/openbsd/target_os_vmparam.h   |  23 +
>  bsd-user/qemu.h                        |  21 +-
>  bsd-user/sparc/target_arch.h           |  11 +
>  bsd-user/sparc/target_arch_cpu.c       | 113 ++++
>  bsd-user/sparc/target_arch_cpu.h       | 158 ++++++
>  bsd-user/sparc/target_arch_vmparam.h   |  37 ++
>  bsd-user/sparc/target_signal.h         |   5 -
>  bsd-user/sparc64/target_arch.h         |  11 +
>  bsd-user/sparc64/target_arch_cpu.c     | 118 +++++
>  bsd-user/sparc64/target_arch_cpu.h     | 191 +++++++
>  bsd-user/sparc64/target_arch_vmparam.h |  37 ++
>  bsd-user/sparc64/target_signal.h       |   5 -
>  bsd-user/x86_64/target_arch.h          |  13 +
>  bsd-user/x86_64/target_arch_cpu.c      |  79 +++
>  bsd-user/x86_64/target_arch_cpu.h      | 322 ++++++++++++
>  bsd-user/x86_64/target_arch_vmparam.h  |  28 +
>  bsd-user/x86_64/target_signal.h        |   5 -
>  33 files changed, 1930 insertions(+), 764 deletions(-)

Oof. I'm afraid this still needs to be separated out into
separate patches a bit more. This patch seems to have:
 * some things which are unrelated to the code-refactoring
   it claims to be doing (I've noted a few below)
 * movement of multiple different functions (most notably,
   this patch deals with both the main cpu loop and also
   the target-cpu-init) which could be done in separate patches
 * some minor style/whitespace changes

> +#if (TARGET_LONG_BITS == 32) && (HOST_LONG_BITS == 64)
> +/*
> + * When running 32-on-64 we should make sure we can fit all of the possible
> + * guest address space into a contiguous chunk of virtual host memory.
> + *
> + * This way we will never overlap with our own libraries or binaries or stack
> + * or anything else that QEMU maps.
> + */
> +unsigned long reserved_va = TARGET_RESERVED_VA;
> +#else
>  unsigned long reserved_va;
>  #endif
> +#endif /* CONFIG_USE_GUEST_BASE */

For instance, this is a separate change -- it's moving into
line with linux-user for the default reserved address, which
is good but not related to moving the arch/OS dependent code.

> +/* Helper routines for implementing atomic operations. */
>
> +/*
> + * To implement exclusive operations we force all cpus to synchronize.
> + * We don't require a full sync, only that no cpus are executing guest code.
> + * The alternative is to map target atomic ops onto host eqivalents,
> + * which requires quite a lot of per host/target work.
> + */
> +static pthread_mutex_t cpu_list_mutex = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_mutex_t exclusive_lock = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_cond_t exclusive_cond = PTHREAD_COND_INITIALIZER;
> +static pthread_cond_t exclusive_resume = PTHREAD_COND_INITIALIZER;
> +static int pending_cpus;

This atomic-operations stuff also looks like a new feature
that should go in its own patch.

> +
> +#if defined(CONFIG_USE_NPTL)

Anything using CONFIG_USE_NPTL needs fixing, because
configure no longer sets that at all: using NPTL became the
default once we converted all the Linux archs to it.

> +void gemu_log(const char *fmt, ...)
> +{
> +    va_list ap;
> +
> +    va_start(ap, fmt);
> +    vfprintf(stderr, fmt, ap);
> +    va_end(ap);
> +}

This function just randomly moved location; seems a bit
pointless, but if you want to do it don't stuff it into this patch.

> @@ -767,7 +362,7 @@ int main(int argc, char **argv)
>  #endif
>
>      optind = 1;
> -    for(;;) {
> +    for (;;) {

Stray style/whitespace only change, don't bother or put it in
its own patch.

> -    if (loader_exec(filename, argv+optind, target_environ, regs, info) != 0) 
> {
> +    if (loader_exec(filename, argv+optind, target_environ, regs, info)) {

Another style-only change.

> @@ -25,7 +25,7 @@ static inline void do_os_print_sysarch(const struct 
> syscallname *name,
>          abi_long arg5, abi_long arg6)
>  {
>      qemu_log("qemu: Unsupported syscall %s\n", __func__);
> -    return -TARGET_ENOSYS;
> +    return;
>  }

Oh look, here's the fix for a bug I pointed out in review of
patch 1/4 :-)

thanks
-- PMM