Re: [Qemu-devel] [PATCH 01/15] target-tricore: Add target stubs and qom-cpu

[Peter Maydell <peter.maydell@linaro.org>]

On 7 July 2014 19:13, Bastian Koppelmann <kbastian@mail.uni-paderborn.de> wrote:
> Add TriCore target stubs, QOM cpu and basic machine.
>
> Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> ---
>  arch_init.c                         |   2 +
>  configure                           |  13 ++
>  cpu-exec.c                          |  11 +-
>  cpus.c                              |   6 +
>  default-configs/tricore-softmmu.mak |   3 +
>  include/elf.h                       |   2 +
>  include/sysemu/arch_init.h          |   1 +
>  target-tricore/Makefile.objs        |   2 +
>  target-tricore/cpu-qom.h            |  71 +++++++
>  target-tricore/cpu.c                | 121 ++++++++++++
>  target-tricore/cpu.h                | 380 ++++++++++++++++++++++++++++++++++++
>  target-tricore/helper.c             |  36 ++++
>  target-tricore/helper.h             |   0
>  target-tricore/machine.c            |  21 ++
>  target-tricore/op_helper.c          |  27 +++
>  target-tricore/translate.c          | 108 ++++++++++
>  target-tricore/translate_init.c     |  21 ++
>  target-tricore/tricore-defs.h       |  28 +++
>  user-exec.c                         |  17 ++
>  19 files changed, 869 insertions(+), 1 deletion(-)
>  create mode 100644 default-configs/tricore-softmmu.mak
>  create mode 100644 target-tricore/Makefile.objs
>  create mode 100644 target-tricore/cpu-qom.h
>  create mode 100644 target-tricore/cpu.c
>  create mode 100644 target-tricore/cpu.h
>  create mode 100644 target-tricore/helper.c
>  create mode 100644 target-tricore/helper.h
>  create mode 100644 target-tricore/machine.c
>  create mode 100644 target-tricore/op_helper.c
>  create mode 100644 target-tricore/translate.c
>  create mode 100644 target-tricore/translate_init.c
>  create mode 100644 target-tricore/tricore-defs.h

I think you probably want the configure and default-configs changes
to be at the end of the patch series (or at least part way through it),
not at the start.
The general approach is:
 * some patches which implement the basic absolute minimum
    functionality, usefully split up
 * then add the configure and default-configs changes that allow
    the user to select the target and include it in the default target
    list

> diff --git a/configure b/configure
> index 7dd43fd..a976862 100755
> --- a/configure
> +++ b/configure
> @@ -495,6 +495,8 @@ elif check_define __mips__ ; then
>    cpu="mips"
>  elif check_define __ia64__ ; then
>    cpu="ia64"
> +elif check_define __tricore__ ; then
> +  cpu="tricore"
>  elif check_define __s390__ ; then
>    if check_define __s390x__ ; then
>      cpu="s390x"

This is for supporting compilation of QEMU on a tricore
*host*. That is a bit pointless since you don't provide a
TCG target backend. In any case if you want to add support
for that I suggest restricting it to its own separate patchset.

> @@ -533,6 +535,9 @@ case "$cpu" in
>    mips*)
>      cpu="mips"
>    ;;
> +  tricore*)
> +    cpu="tricore"
> +  ;;

Also host CPU related. (I suggest looking at where the
configure script handles the "moxie" target type, since that's
an example of a CPU type we handle only as a guest CPU
and not as a host CPU, so it's a good indication of where you
need to add tricore cases.)

>    sparc|sun4[cdmuv])
>      cpu="sparc"
>    ;;
> @@ -4958,6 +4963,10 @@ case "$target_name" in
>      TARGET_BASE_ARCH=mips
>      echo "TARGET_ABI_MIPSN64=y" >> $config_target_mak
>    ;;
> +  tricore)
> +    TARGET_ARCH=tricore

You don't need this line, because the default value for
TARGET_ARCH is the target name; only those cases in this
case statement where the CPU target name might be different
need to set TARGET_ARCH specifically.

> +    target_phys_bits=32
> +  ;;
>    moxie)
>    ;;
>    or32)
> @@ -5155,6 +5164,10 @@ for i in $ARCH $TARGET_BASE_ARCH ; do
>      echo "CONFIG_MIPS_DIS=y"  >> $config_target_mak
>      echo "CONFIG_MIPS_DIS=y"  >> config-all-disas.mak
>    ;;
> +  tricore*)
> +    echo "CONFIG_TRICORE_DIS=y" >> $config_target_mak
> +    echo "CONFIG_TRICORE_DIS=y" >> config-all-disas.mak
> +  ;;

Diffstat says you haven't got a disassembler in this patch,
so what is this for? Don't add it until/unless you add the
disassembler code.

> --- /dev/null
> +++ b/target-tricore/machine.c
> @@ -0,0 +1,21 @@
> +/*
> + *  Copyright (c) 2012-2014 Bastian Koppelmann C-Lab/University Paderborn
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "hw/hw.h"
> +#include "hw/boards.h"
> +
> +#include "cpu.h"

This file has no content; don't add it until it's got some useful code in it...

> diff --git a/user-exec.c b/user-exec.c
> index 1ff8673..beba4d5 100644
> --- a/user-exec.c
> +++ b/user-exec.c
> @@ -614,6 +614,23 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>                               is_write, &uc->uc_sigmask, puc);
>  }
>
> +#elif defined(__tricore__)
> +
> +int cpu_signal_handler(int host_signum, void *pinfo,
> +                       void *puc)
> +{
> +    printf("cpu_signal_handler\n");
> +  /*  siginfo_t *info = pinfo;
> +    struct ucontext *uc = puc;
> +    greg_t pc = uc->uc_mcontext.pc;
> +    int is_write;
> +    is_write = 0;
> +    return handle_cpu_signal(pc, (unsigned long)info->si_addr,
> +                             is_write, &uc->uc_sigmask, puc);*/
> +    return 0;
> +}
> +
> +

This is also host-CPU support; don't put it in this patchset.
(Also, don't include commented out code like that...)

thanks
-- PMM