Re: [PATCH v2 4/5] system/vl: Restrict icount to TCG emulation

[Philippe Mathieu-Daudé]

On 7/12/23 23:38, Richard Henderson wrote:
> On 12/7/23 07:45, Philippe Mathieu-Daudé wrote:
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   stubs/icount.c | 6 ------
> >   system/vl.c    | 6 +++++-
> >   2 files changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/stubs/icount.c b/stubs/icount.c
> > index a5202e2dd9..b060b03a73 100644
> > --- a/stubs/icount.c
> > +++ b/stubs/icount.c
> > @@ -1,5 +1,4 @@
> >   #include "qemu/osdep.h"
> > -#include "qapi/error.h"
> >   #include "sysemu/cpu-timers.h"
> >   /* icount - Instruction Counter API */
> > @@ -10,11 +9,6 @@ void icount_update(CPUState *cpu)
> >   {
> >       abort();
> >   }
> > -void icount_configure(QemuOpts *opts, Error **errp)
> > -{
> > -    /* signal error */
> > -    error_setg(errp, "cannot configure icount, TCG support not 
> > available");
> > -}
> >   int64_t icount_get_raw(void)
> >   {
> >       abort();
> > diff --git a/system/vl.c b/system/vl.c
> > index 2bcd9efb9a..8c99c5f681 100644
> > --- a/system/vl.c
> > +++ b/system/vl.c
> > @@ -2270,7 +2270,11 @@ static void user_register_global_props(void)
> >   static int do_configure_icount(void *opaque, QemuOpts *opts, Error 
> > **errp)
> >   {
> > -    icount_configure(opts, errp);
> > +    if (tcg_enabled()) {
> > +        icount_configure(opts, errp);
> > +    } else {
> > +        error_setg(errp, "cannot configure icount, TCG support not 
> > available");
> > +    }
> >       return 0;
> >   }
>
> This is called before the accelerator is chosen -- even before the set 
> of available accelerators is even found.  Indeed, that's the very next 
> thing that configure_accelerators does.
>
> OTOH, I don't see why icount_configure is being called so early.

See commit 7f8b6126e7:

    vl: move icount configuration earlier

    Once qemu_tcg_configure is turned into a QOM property setter,
    it will not be able to set a default value for mttcg_enabled.
    Setting the default will move to the TCG instance_init function,
    which currently runs before "-icount" is processed.

    However, it is harmless to do configure_icount for all accelerators;
    we will just fail later if a non-TCG accelerator is selected.  So do
    that.

But few commits after we have 28a0961757 ("vl: merge -accel processing
into configure_accelerators"), so it shouldn't be an issue now. I'll
consolidate.