Re: [Qemu-devel] [RFC PATCH 01/11] semihosting: move semihosting configuration into its own directory

[Philippe Mathieu-Daudé]

On 5/14/19 5:52 PM, Alex Bennée wrote:
> In preparation for having some more common semihosting code let's
> excise the current config magic from vl.c into its own file. We shall
> later add more conditionals to the build configurations so we can
> avoid building this if we don't need it.
> 
> Signed-off-by: Alex Bennée <address@hidden>

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
Tested-by: Philippe Mathieu-Daudé <address@hidden>

> ---
>  gdbstub.c                                   |   2 +-
>  hw/Makefile.objs                            |   1 +
>  hw/mips/mips_malta.c                        |   2 +-
>  hw/semihosting/Makefile.objs                |   1 +
>  hw/semihosting/config.c                     | 160 ++++++++++++++++++++
>  include/{exec => hw/semihosting}/semihost.h |  10 +-
>  include/sysemu/sysemu.h                     |   1 +
>  target/arm/arm-semi.c                       |   2 +-
>  target/arm/helper.c                         |   2 +-
>  target/arm/translate-a64.c                  |   2 +-
>  target/arm/translate.c                      |   2 +-
>  target/lm32/helper.c                        |   2 +-
>  target/m68k/op_helper.c                     |   2 +-
>  target/mips/mips-semi.c                     |   2 +-
>  target/mips/translate.c                     |   2 +-
>  target/nios2/helper.c                       |   2 +-
>  target/xtensa/translate.c                   |   2 +-
>  target/xtensa/xtensa-semi.c                 |   2 +-
>  vl.c                                        | 126 +--------------
>  19 files changed, 186 insertions(+), 139 deletions(-)
>  create mode 100644 hw/semihosting/Makefile.objs
>  create mode 100644 hw/semihosting/config.c
>  rename include/{exec => hw/semihosting}/semihost.h (85%)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index d54abd17cc2..793218bb43a 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -37,7 +37,7 @@
>  #include "qemu/sockets.h"
>  #include "sysemu/hw_accel.h"
>  #include "sysemu/kvm.h"
> -#include "exec/semihost.h"
> +#include "hw/semihosting/semihost.h"
>  #include "exec/exec-all.h"
>  
>  #ifdef CONFIG_USER_ONLY
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 82aa7fab8e4..d770926ba96 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -36,6 +36,7 @@ devices-dirs-$(CONFIG_SOFTMMU) += watchdog/
>  devices-dirs-$(CONFIG_SOFTMMU) += xen/
>  devices-dirs-$(CONFIG_MEM_DEVICE) += mem/
>  devices-dirs-$(CONFIG_SOFTMMU) += smbios/
> +devices-dirs-y += semihosting/
>  devices-dirs-y += core/
>  common-obj-y += $(devices-dirs-y)
>  obj-y += $(devices-dirs-y)
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 439665ab45e..3b009fda25f 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -55,7 +55,7 @@
>  #include "qemu/error-report.h"
>  #include "hw/empty_slot.h"
>  #include "sysemu/kvm.h"
> -#include "exec/semihost.h"
> +#include "hw/semihosting/semihost.h"
>  #include "hw/mips/cps.h"
>  
>  #define ENVP_ADDR            0x80002000l
> diff --git a/hw/semihosting/Makefile.objs b/hw/semihosting/Makefile.objs
> new file mode 100644
> index 00000000000..546954f4ff4
> --- /dev/null
> +++ b/hw/semihosting/Makefile.objs
> @@ -0,0 +1 @@
> +common-obj-$(CONFIG_SOFTMMU) += config.o
> diff --git a/hw/semihosting/config.c b/hw/semihosting/config.c
> new file mode 100644
> index 00000000000..f1d3fe1e4c6
> --- /dev/null
> +++ b/hw/semihosting/config.c
> @@ -0,0 +1,160 @@
> +/*
> + * Semihosting configuration
> + *
> + * Copyright (c) 2015 Imagination Technologies
> + * Copyright (c) 2019 Linaro Ltd
> + *
> + * This controls the configuration of semihosting for all guest
> + * targets that support it. Architecture specific handling is handled
> + * in target/HW/HW-semi.c
> + *
> + * Semihosting is sightly strange in that it is also supported by some
> + * linux-user targets. However in that use case no configuration of
> + * the outputs and command lines is supported.
> + *
> + * The config module is common to all softmmu targets however as vl.c
> + * needs to link against the helpers.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/option.h"
> +#include "qemu/config-file.h"
> +#include "qemu/error-report.h"
> +#include "hw/semihosting/semihost.h"
> +
> +QemuOptsList qemu_semihosting_config_opts = {
> +    .name = "semihosting-config",
> +    .implied_opt_name = "enable",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_semihosting_config_opts.head),
> +    .desc = {
> +        {
> +            .name = "enable",
> +            .type = QEMU_OPT_BOOL,
> +        }, {
> +            .name = "target",
> +            .type = QEMU_OPT_STRING,
> +        }, {
> +            .name = "arg",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
> +typedef struct SemihostingConfig {
> +    bool enabled;
> +    SemihostingTarget target;
> +    const char **argv;
> +    int argc;
> +    const char *cmdline; /* concatenated argv */
> +} SemihostingConfig;
> +
> +static SemihostingConfig semihosting;
> +
> +bool semihosting_enabled(void)
> +{
> +    return semihosting.enabled;
> +}
> +
> +SemihostingTarget semihosting_get_target(void)
> +{
> +    return semihosting.target;
> +}
> +
> +const char *semihosting_get_arg(int i)
> +{
> +    if (i >= semihosting.argc) {
> +        return NULL;
> +    }
> +    return semihosting.argv[i];
> +}
> +
> +int semihosting_get_argc(void)
> +{
> +    return semihosting.argc;
> +}
> +
> +const char *semihosting_get_cmdline(void)
> +{
> +    if (semihosting.cmdline == NULL && semihosting.argc > 0) {
> +        semihosting.cmdline = g_strjoinv(" ", (gchar **)semihosting.argv);
> +    }
> +    return semihosting.cmdline;
> +}
> +
> +static int add_semihosting_arg(void *opaque,
> +                               const char *name, const char *val,
> +                               Error **errp)
> +{
> +    SemihostingConfig *s = opaque;
> +    if (strcmp(name, "arg") == 0) {
> +        s->argc++;
> +        /* one extra element as g_strjoinv() expects NULL-terminated array */
> +        s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *));
> +        s->argv[s->argc - 1] = val;
> +        s->argv[s->argc] = NULL;
> +    }
> +    return 0;
> +}
> +
> +/* Use strings passed via -kernel/-append to initialize semihosting.argv[] */
> +void semihosting_arg_fallback(const char *file, const char *cmd)
> +{
> +    char *cmd_token;
> +
> +    /* argv[0] */
> +    add_semihosting_arg(&semihosting, "arg", file, NULL);
> +
> +    /* split -append and initialize argv[1..n] */
> +    cmd_token = strtok(g_strdup(cmd), " ");
> +    while (cmd_token) {
> +        add_semihosting_arg(&semihosting, "arg", cmd_token, NULL);
> +        cmd_token = strtok(NULL, " ");
> +    }
> +}
> +
> +void qemu_semihosting_enable(void)
> +{
> +    semihosting.enabled = true;
> +    semihosting.target = SEMIHOSTING_TARGET_AUTO;
> +}
> +
> +int qemu_semihosting_config_options(const char *optarg)
> +{
> +    QemuOptsList *opt_list = qemu_find_opts("semihosting-config");
> +    QemuOpts *opts = qemu_opts_parse_noisily(opt_list, optarg, false);
> +
> +    semihosting.enabled = true;
> +
> +    if (opts != NULL) {
> +        semihosting.enabled = qemu_opt_get_bool(opts, "enable",
> +                                                true);
> +        const char *target = qemu_opt_get(opts, "target");
> +        if (target != NULL) {
> +            if (strcmp("native", target) == 0) {
> +                semihosting.target = SEMIHOSTING_TARGET_NATIVE;
> +            } else if (strcmp("gdb", target) == 0) {
> +                semihosting.target = SEMIHOSTING_TARGET_GDB;
> +            } else  if (strcmp("auto", target) == 0) {
> +                semihosting.target = SEMIHOSTING_TARGET_AUTO;
> +            } else {
> +                error_report("unsupported semihosting-config %s",
> +                             optarg);
> +                return 1;
> +            }
> +        } else {
> +            semihosting.target = SEMIHOSTING_TARGET_AUTO;
> +        }
> +        /* Set semihosting argument count and vector */
> +        qemu_opt_foreach(opts, add_semihosting_arg,
> +                         &semihosting, NULL);
> +    } else {
> +        error_report("unsupported semihosting-config %s", optarg);
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> diff --git a/include/exec/semihost.h b/include/hw/semihosting/semihost.h
> similarity index 85%
> rename from include/exec/semihost.h
> rename to include/hw/semihosting/semihost.h
> index 5980939c7b8..07ea40a322f 100644
> --- a/include/exec/semihost.h
> +++ b/include/hw/semihosting/semihost.h
> @@ -51,12 +51,16 @@ static inline const char *semihosting_get_cmdline(void)
>  {
>      return NULL;
>  }
> -#else
> +#else /* !CONFIG_USER_ONLY */
>  bool semihosting_enabled(void);
>  SemihostingTarget semihosting_get_target(void);
>  const char *semihosting_get_arg(int i);
>  int semihosting_get_argc(void);
>  const char *semihosting_get_cmdline(void);
> -#endif
> +void semihosting_arg_fallback(const char *file, const char *cmd);
> +/* for vl.c hooks */
> +void qemu_semihosting_enable(void);
> +int qemu_semihosting_config_options(const char *opt);
> +#endif /* CONFIG_USER_ONLY */
>  
> -#endif
> +#endif /* SEMIHOST_H */
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 5f133cae837..61579ae71ef 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -193,5 +193,6 @@ extern QemuOptsList qemu_nic_opts;
>  extern QemuOptsList qemu_net_opts;
>  extern QemuOptsList qemu_global_opts;
>  extern QemuOptsList qemu_mon_opts;
> +extern QemuOptsList qemu_semihosting_config_opts;
>  
>  #endif
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 8b5fd7bc6e3..3273306c8ea 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -21,7 +21,7 @@
>  #include "qemu/osdep.h"
>  
>  #include "cpu.h"
> -#include "exec/semihost.h"
> +#include "hw/semihosting/semihost.h"
>  #ifdef CONFIG_USER_ONLY
>  #include "qemu.h"
>  
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 1e6eb0d0f36..bc20cb3b9e2 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -16,7 +16,7 @@
>  #include "exec/cpu_ldst.h"
>  #include "arm_ldst.h"
>  #include <zlib.h> /* For crc32 */
> -#include "exec/semihost.h"
> +#include "hw/semihosting/semihost.h"
>  #include "sysemu/cpus.h"
>  #include "sysemu/kvm.h"
>  #include "fpu/softfloat.h"
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index b7c5a928b4a..8844a75e53d 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -29,7 +29,7 @@
>  #include "qemu/host-utils.h"
>  #include "qemu/qemu-print.h"
>  
> -#include "exec/semihost.h"
> +#include "hw/semihosting/semihost.h"
>  #include "exec/gen-icount.h"
>  
>  #include "exec/helper-proto.h"
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index dd053c80d62..f2a65c0b5b0 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -30,7 +30,7 @@
>  #include "qemu/bitops.h"
>  #include "qemu/qemu-print.h"
>  #include "arm_ldst.h"
> -#include "exec/semihost.h"
> +#include "hw/semihosting/semihost.h"
>  
>  #include "exec/helper-proto.h"
>  #include "exec/helper-gen.h"
> diff --git a/target/lm32/helper.c b/target/lm32/helper.c
> index a039a993ffe..e83cdf43182 100644
> --- a/target/lm32/helper.c
> +++ b/target/lm32/helper.c
> @@ -22,7 +22,7 @@
>  #include "exec/exec-all.h"
>  #include "qemu/host-utils.h"
>  #include "sysemu/sysemu.h"
> -#include "exec/semihost.h"
> +#include "hw/semihosting/semihost.h"
>  #include "exec/log.h"
>  
>  int lm32_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw,
> diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
> index 76f439985a0..ee578f8d60d 100644
> --- a/target/m68k/op_helper.c
> +++ b/target/m68k/op_helper.c
> @@ -21,7 +21,7 @@
>  #include "exec/helper-proto.h"
>  #include "exec/exec-all.h"
>  #include "exec/cpu_ldst.h"
> -#include "exec/semihost.h"
> +#include "hw/semihosting/semihost.h"
>  
>  #if defined(CONFIG_USER_ONLY)
>  
> diff --git a/target/mips/mips-semi.c b/target/mips/mips-semi.c
> index a7aefbaefc8..eac8374fb34 100644
> --- a/target/mips/mips-semi.c
> +++ b/target/mips/mips-semi.c
> @@ -22,7 +22,7 @@
>  #include "qemu/log.h"
>  #include "exec/helper-proto.h"
>  #include "exec/softmmu-semi.h"
> -#include "exec/semihost.h"
> +#include "hw/semihosting/semihost.h"
>  
>  typedef enum UHIOp {
>      UHI_exit = 1,
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index f96c0d01ef1..3cd5b11b16b 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -32,7 +32,7 @@
>  
>  #include "exec/helper-proto.h"
>  #include "exec/helper-gen.h"
> -#include "exec/semihost.h"
> +#include "hw/semihosting/semihost.h"
>  
>  #include "target/mips/trace.h"
>  #include "trace-tcg.h"
> diff --git a/target/nios2/helper.c b/target/nios2/helper.c
> index e01fc1ff3e7..a3bd93e483d 100644
> --- a/target/nios2/helper.c
> +++ b/target/nios2/helper.c
> @@ -26,7 +26,7 @@
>  #include "exec/cpu_ldst.h"
>  #include "exec/log.h"
>  #include "exec/helper-proto.h"
> -#include "exec/semihost.h"
> +#include "hw/semihosting/semihost.h"
>  
>  #if defined(CONFIG_USER_ONLY)
>  
> diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
> index b063fa85f26..6bdc244b583 100644
> --- a/target/xtensa/translate.c
> +++ b/target/xtensa/translate.c
> @@ -38,7 +38,7 @@
>  #include "qemu/qemu-print.h"
>  #include "sysemu/sysemu.h"
>  #include "exec/cpu_ldst.h"
> -#include "exec/semihost.h"
> +#include "hw/semihosting/semihost.h"
>  #include "exec/translator.h"
>  
>  #include "exec/helper-proto.h"
> diff --git a/target/xtensa/xtensa-semi.c b/target/xtensa/xtensa-semi.c
> index 5f5ce4f344c..38efa3485af 100644
> --- a/target/xtensa/xtensa-semi.c
> +++ b/target/xtensa/xtensa-semi.c
> @@ -29,7 +29,7 @@
>  #include "cpu.h"
>  #include "chardev/char-fe.h"
>  #include "exec/helper-proto.h"
> -#include "exec/semihost.h"
> +#include "hw/semihosting/semihost.h"
>  #include "qapi/error.h"
>  #include "qemu/log.h"
>  #include "sysemu/sysemu.h"
> diff --git a/vl.c b/vl.c
> index b6709514c1b..fba48b565c9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -116,7 +116,7 @@ int main(int argc, char **argv)
>  #include "qapi/opts-visitor.h"
>  #include "qapi/clone-visitor.h"
>  #include "qom/object_interfaces.h"
> -#include "exec/semihost.h"
> +#include "hw/semihosting/semihost.h"
>  #include "crypto/init.h"
>  #include "sysemu/replay.h"
>  #include "qapi/qapi-events-run-state.h"
> @@ -500,25 +500,6 @@ static QemuOptsList qemu_icount_opts = {
>      },
>  };
>  
> -static QemuOptsList qemu_semihosting_config_opts = {
> -    .name = "semihosting-config",
> -    .implied_opt_name = "enable",
> -    .head = QTAILQ_HEAD_INITIALIZER(qemu_semihosting_config_opts.head),
> -    .desc = {
> -        {
> -            .name = "enable",
> -            .type = QEMU_OPT_BOOL,
> -        }, {
> -            .name = "target",
> -            .type = QEMU_OPT_STRING,
> -        }, {
> -            .name = "arg",
> -            .type = QEMU_OPT_STRING,
> -        },
> -        { /* end of list */ }
> -    },
> -};
> -
>  static QemuOptsList qemu_fw_cfg_opts = {
>      .name = "fw_cfg",
>      .implied_opt_name = "name",
> @@ -1350,80 +1331,6 @@ static void configure_msg(QemuOpts *opts)
>      enable_timestamp_msg = qemu_opt_get_bool(opts, "timestamp", true);
>  }
>  
> -/***********************************************************/
> -/* Semihosting */
> -
> -typedef struct SemihostingConfig {
> -    bool enabled;
> -    SemihostingTarget target;
> -    const char **argv;
> -    int argc;
> -    const char *cmdline; /* concatenated argv */
> -} SemihostingConfig;
> -
> -static SemihostingConfig semihosting;
> -
> -bool semihosting_enabled(void)
> -{
> -    return semihosting.enabled;
> -}
> -
> -SemihostingTarget semihosting_get_target(void)
> -{
> -    return semihosting.target;
> -}
> -
> -const char *semihosting_get_arg(int i)
> -{
> -    if (i >= semihosting.argc) {
> -        return NULL;
> -    }
> -    return semihosting.argv[i];
> -}
> -
> -int semihosting_get_argc(void)
> -{
> -    return semihosting.argc;
> -}
> -
> -const char *semihosting_get_cmdline(void)
> -{
> -    if (semihosting.cmdline == NULL && semihosting.argc > 0) {
> -        semihosting.cmdline = g_strjoinv(" ", (gchar **)semihosting.argv);
> -    }
> -    return semihosting.cmdline;
> -}
> -
> -static int add_semihosting_arg(void *opaque,
> -                               const char *name, const char *val,
> -                               Error **errp)
> -{
> -    SemihostingConfig *s = opaque;
> -    if (strcmp(name, "arg") == 0) {
> -        s->argc++;
> -        /* one extra element as g_strjoinv() expects NULL-terminated array */
> -        s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *));
> -        s->argv[s->argc - 1] = val;
> -        s->argv[s->argc] = NULL;
> -    }
> -    return 0;
> -}
> -
> -/* Use strings passed via -kernel/-append to initialize semihosting.argv[] */
> -static inline void semihosting_arg_fallback(const char *file, const char 
> *cmd)
> -{
> -    char *cmd_token;
> -
> -    /* argv[0] */
> -    add_semihosting_arg(&semihosting, "arg", file, NULL);
> -
> -    /* split -append and initialize argv[1..n] */
> -    cmd_token = strtok(g_strdup(cmd), " ");
> -    while (cmd_token) {
> -        add_semihosting_arg(&semihosting, "arg", cmd_token, NULL);
> -        cmd_token = strtok(NULL, " ");
> -    }
> -}
>  
>  /* Now we still need this for compatibility with XEN. */
>  bool has_igd_gfx_passthru;
> @@ -3733,37 +3640,10 @@ int main(int argc, char **argv, char **envp)
>                  nb_option_roms++;
>                  break;
>              case QEMU_OPTION_semihosting:
> -                semihosting.enabled = true;
> -                semihosting.target = SEMIHOSTING_TARGET_AUTO;
> +                qemu_semihosting_enable();
>                  break;
>              case QEMU_OPTION_semihosting_config:
> -                semihosting.enabled = true;
> -                opts = 
> qemu_opts_parse_noisily(qemu_find_opts("semihosting-config"),
> -                                               optarg, false);
> -                if (opts != NULL) {
> -                    semihosting.enabled = qemu_opt_get_bool(opts, "enable",
> -                                                            true);
> -                    const char *target = qemu_opt_get(opts, "target");
> -                    if (target != NULL) {
> -                        if (strcmp("native", target) == 0) {
> -                            semihosting.target = SEMIHOSTING_TARGET_NATIVE;
> -                        } else if (strcmp("gdb", target) == 0) {
> -                            semihosting.target = SEMIHOSTING_TARGET_GDB;
> -                        } else  if (strcmp("auto", target) == 0) {
> -                            semihosting.target = SEMIHOSTING_TARGET_AUTO;
> -                        } else {
> -                            error_report("unsupported semihosting-config %s",
> -                                         optarg);
> -                            exit(1);
> -                        }
> -                    } else {
> -                        semihosting.target = SEMIHOSTING_TARGET_AUTO;
> -                    }
> -                    /* Set semihosting argument count and vector */
> -                    qemu_opt_foreach(opts, add_semihosting_arg,
> -                                     &semihosting, NULL);
> -                } else {
> -                    error_report("unsupported semihosting-config %s", 
> optarg);
> +                if (qemu_semihosting_config_options(optarg) != 0) {
>                      exit(1);
>                  }
>                  break;
>