Re: [PATCH 11/16] tcg: convert "-accel threads" to a QOM property

[Marc-André Lureau]

On Wed, Nov 13, 2019 at 6:49 PM Paolo Bonzini <address@hidden> wrote:
>
> Replace the ad-hoc qemu_tcg_configure with generic code invoking
> QOM property getters and setters.  This will be extended in the
> next patches, which will turn accelerator-related "-machine"
> options into QOM properties as well.
>
> Signed-off-by: Paolo Bonzini <address@hidden>

Reviewed-by: Marc-André Lureau <address@hidden>

> ---
>  accel/tcg/tcg-all.c   | 111 
> +++++++++++++++++++++++++++++++++++++++++++++++++-
>  cpus.c                |  72 --------------------------------
>  include/sysemu/cpus.h |   2 -
>  vl.c                  |  32 ++++++++-------
>  4 files changed, 126 insertions(+), 91 deletions(-)
>
> diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
> index c59d5b0..7829f02 100644
> --- a/accel/tcg/tcg-all.c
> +++ b/accel/tcg/tcg-all.c
> @@ -30,6 +30,21 @@
>  #include "cpu.h"
>  #include "sysemu/cpus.h"
>  #include "qemu/main-loop.h"
> +#include "tcg/tcg.h"
> +#include "include/qapi/error.h"
> +#include "include/qemu/error-report.h"
> +#include "include/hw/boards.h"
> +
> +typedef struct TCGState {
> +    AccelState parent_obj;
> +
> +    bool mttcg_enabled;
> +} TCGState;
> +
> +#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
> +
> +#define TCG_STATE(obj) \
> +        OBJECT_CHECK(TCGState, (obj), TYPE_TCG_ACCEL)
>
>  unsigned long tcg_tb_size;
>
> @@ -58,27 +73,119 @@ static void tcg_handle_interrupt(CPUState *cpu, int mask)
>      }
>  }
>
> +/*
> + * We default to false if we know other options have been enabled
> + * which are currently incompatible with MTTCG. Otherwise when each
> + * guest (target) has been updated to support:
> + *   - atomic instructions
> + *   - memory ordering primitives (barriers)
> + * they can set the appropriate CONFIG flags in ${target}-softmmu.mak
> + *
> + * Once a guest architecture has been converted to the new primitives
> + * there are two remaining limitations to check.
> + *
> + * - The guest can't be oversized (e.g. 64 bit guest on 32 bit host)
> + * - The host must have a stronger memory order than the guest
> + *
> + * It may be possible in future to support strong guests on weak hosts
> + * but that will require tagging all load/stores in a guest with their
> + * implicit memory order requirements which would likely slow things
> + * down a lot.
> + */
> +
> +static bool check_tcg_memory_orders_compatible(void)
> +{
> +#if defined(TCG_GUEST_DEFAULT_MO) && defined(TCG_TARGET_DEFAULT_MO)
> +    return (TCG_GUEST_DEFAULT_MO & ~TCG_TARGET_DEFAULT_MO) == 0;
> +#else
> +    return false;
> +#endif
> +}
> +
> +static bool default_mttcg_enabled(void)
> +{
> +    if (use_icount || TCG_OVERSIZED_GUEST) {
> +        return false;
> +    } else {
> +#ifdef TARGET_SUPPORTS_MTTCG
> +        return check_tcg_memory_orders_compatible();
> +#else
> +        return false;
> +#endif
> +    }
> +}
> +
> +static void tcg_accel_instance_init(Object *obj)
> +{
> +    TCGState *s = TCG_STATE(obj);
> +
> +    s->mttcg_enabled = default_mttcg_enabled();
> +}
> +
>  static int tcg_init(MachineState *ms)
>  {
> +    TCGState *s = TCG_STATE(current_machine->accelerator);
> +
>      tcg_exec_init(tcg_tb_size * 1024 * 1024);
>      cpu_interrupt_handler = tcg_handle_interrupt;
> +    mttcg_enabled = s->mttcg_enabled;
>      return 0;
>  }
>
> +static char *tcg_get_thread(Object *obj, Error **errp)
> +{
> +    TCGState *s = TCG_STATE(obj);
> +
> +    return g_strdup(s->mttcg_enabled ? "multi" : "single");
> +}
> +
> +static void tcg_set_thread(Object *obj, const char *value, Error **errp)
> +{
> +    TCGState *s = TCG_STATE(obj);
> +
> +    if (strcmp(value, "multi") == 0) {
> +        if (TCG_OVERSIZED_GUEST) {
> +            error_setg(errp, "No MTTCG when guest word size > hosts");
> +        } else if (use_icount) {
> +            error_setg(errp, "No MTTCG when icount is enabled");
> +        } else {
> +#ifndef TARGET_SUPPORTS_MTTCG
> +            warn_report("Guest not yet converted to MTTCG - "
> +                        "you may get unexpected results");
> +#endif
> +            if (!check_tcg_memory_orders_compatible()) {
> +                warn_report("Guest expects a stronger memory ordering "
> +                            "than the host provides");
> +                error_printf("This may cause strange/hard to debug 
> errors\n");
> +            }
> +            s->mttcg_enabled = true;
> +        }
> +    } else if (strcmp(value, "single") == 0) {
> +        s->mttcg_enabled = false;
> +    } else {
> +        error_setg(errp, "Invalid 'thread' setting %s", value);
> +    }
> +}
> +
>  static void tcg_accel_class_init(ObjectClass *oc, void *data)
>  {
>      AccelClass *ac = ACCEL_CLASS(oc);
>      ac->name = "tcg";
>      ac->init_machine = tcg_init;
>      ac->allowed = &tcg_allowed;
> -}
>
> -#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
> +    object_class_property_add_str(oc, "thread",
> +                                  tcg_get_thread,
> +                                  tcg_set_thread,
> +                                  NULL);
> +}
>
>  static const TypeInfo tcg_accel_type = {
>      .name = TYPE_TCG_ACCEL,
>      .parent = TYPE_ACCEL,
> +    .instance_init = tcg_accel_instance_init,
>      .class_init = tcg_accel_class_init,
> +    .instance_size = sizeof(TCGState),
>  };
>
>  static void register_accel_types(void)
> diff --git a/cpus.c b/cpus.c
> index fabbeca..69d4f6a 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -165,78 +165,6 @@ typedef struct TimersState {
>  static TimersState timers_state;
>  bool mttcg_enabled;
>
> -/*
> - * We default to false if we know other options have been enabled
> - * which are currently incompatible with MTTCG. Otherwise when each
> - * guest (target) has been updated to support:
> - *   - atomic instructions
> - *   - memory ordering primitives (barriers)
> - * they can set the appropriate CONFIG flags in ${target}-softmmu.mak
> - *
> - * Once a guest architecture has been converted to the new primitives
> - * there are two remaining limitations to check.
> - *
> - * - The guest can't be oversized (e.g. 64 bit guest on 32 bit host)
> - * - The host must have a stronger memory order than the guest
> - *
> - * It may be possible in future to support strong guests on weak hosts
> - * but that will require tagging all load/stores in a guest with their
> - * implicit memory order requirements which would likely slow things
> - * down a lot.
> - */
> -
> -static bool check_tcg_memory_orders_compatible(void)
> -{
> -#if defined(TCG_GUEST_DEFAULT_MO) && defined(TCG_TARGET_DEFAULT_MO)
> -    return (TCG_GUEST_DEFAULT_MO & ~TCG_TARGET_DEFAULT_MO) == 0;
> -#else
> -    return false;
> -#endif
> -}
> -
> -static bool default_mttcg_enabled(void)
> -{
> -    if (use_icount || TCG_OVERSIZED_GUEST) {
> -        return false;
> -    } else {
> -#ifdef TARGET_SUPPORTS_MTTCG
> -        return check_tcg_memory_orders_compatible();
> -#else
> -        return false;
> -#endif
> -    }
> -}
> -
> -void qemu_tcg_configure(QemuOpts *opts, Error **errp)
> -{
> -    const char *t = qemu_opt_get(opts, "thread");
> -    if (t) {
> -        if (strcmp(t, "multi") == 0) {
> -            if (TCG_OVERSIZED_GUEST) {
> -                error_setg(errp, "No MTTCG when guest word size > hosts");
> -            } else if (use_icount) {
> -                error_setg(errp, "No MTTCG when icount is enabled");
> -            } else {
> -#ifndef TARGET_SUPPORTS_MTTCG
> -                warn_report("Guest not yet converted to MTTCG - "
> -                            "you may get unexpected results");
> -#endif
> -                if (!check_tcg_memory_orders_compatible()) {
> -                    warn_report("Guest expects a stronger memory ordering "
> -                                "than the host provides");
> -                    error_printf("This may cause strange/hard to debug 
> errors\n");
> -                }
> -                mttcg_enabled = true;
> -            }
> -        } else if (strcmp(t, "single") == 0) {
> -            mttcg_enabled = false;
> -        } else {
> -            error_setg(errp, "Invalid 'thread' setting %s", t);
> -        }
> -    } else {
> -        mttcg_enabled = default_mttcg_enabled();
> -    }
> -}
>
>  /* The current number of executed instructions is based on what we
>   * originally budgeted minus the current state of the decrementing
> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> index 32c05f2..3c1da6a 100644
> --- a/include/sysemu/cpus.h
> +++ b/include/sysemu/cpus.h
> @@ -40,6 +40,4 @@ extern int smp_threads;
>
>  void list_cpus(const char *optarg);
>
> -void qemu_tcg_configure(QemuOpts *opts, Error **errp);
> -
>  #endif
> diff --git a/vl.c b/vl.c
> index c8ec906..2ea94c7 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -294,17 +294,12 @@ static QemuOptsList qemu_accel_opts = {
>      .implied_opt_name = "accel",
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_accel_opts.head),
>      .desc = {
> -        {
> -            .name = "accel",
> -            .type = QEMU_OPT_STRING,
> -            .help = "Select the type of accelerator",
> -        },
> -        {
> -            .name = "thread",
> -            .type = QEMU_OPT_STRING,
> -            .help = "Enable/disable multi-threaded TCG",
> -        },
> -        { /* end of list */ }
> +        /*
> +         * no elements => accept any
> +         * sanity checking will happen later
> +         * when setting accelerator properties
> +         */
> +        { }
>      },
>  };
>
> @@ -2841,6 +2836,13 @@ static int do_configure_icount(void *opaque, QemuOpts 
> *opts, Error **errp)
>      return 0;
>  }
>
> +static int accelerator_set_property(void *opaque,
> +                                const char *name, const char *value,
> +                                Error **errp)
> +{
> +    return object_parse_property_opt(opaque, name, value, "accel", errp);
> +}
> +
>  static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error 
> **errp)
>  {
>      bool *p_init_failed = opaque;
> @@ -2855,6 +2857,10 @@ static int do_configure_accelerator(void *opaque, 
> QemuOpts *opts, Error **errp)
>          return 0;
>      }
>      accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
> +    qemu_opt_foreach(opts, accelerator_set_property,
> +                     accel,
> +                     &error_fatal);
> +
>      ret = accel_init_machine(accel, current_machine);
>      if (ret < 0) {
>          *p_init_failed = true;
> @@ -2862,10 +2868,6 @@ static int do_configure_accelerator(void *opaque, 
> QemuOpts *opts, Error **errp)
>                       acc, strerror(-ret));
>          return 0;
>      }
> -
> -    if (tcg_enabled()) {
> -        qemu_tcg_configure(opts, &error_fatal);
> -    }
>      return 1;
>  }
>
> --
> 1.8.3.1
>
>
>


-- 
Marc-André Lureau