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

2023-12-08 Thread 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é 
---
  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.



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

2023-12-07 Thread Richard Henderson

On 12/7/23 07:45, Philippe Mathieu-Daudé wrote:

Signed-off-by: Philippe Mathieu-Daudé 
---
  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.


r~