[PATCH v2 0/1] Improve module accelerator error message

[PATCH v2 0/1] Improve module accelerator error message

[Jose R. Ziviani]

v1 -> v2:
* Moved the code to module.c
* Simplified a lot by using current module DB to get info

The main objective is to improve the error message when trying to
load a not found/not installed module TCG.

For example:

$ qemu-system-x86_64 -accel tcg
ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: 
(ops != NULL)
Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: 
assertion failed: (ops != NULL)
[1]    31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...

To:
$ qemu-system-x86_64 -accel tcg
accel-tcg-x86_64 module is missing, install the package or config the library 
path correctly.

Jose R. Ziviani (1):
  modules: Improve error message when module is not found

 accel/accel-softmmu.c |  5 ++++-
 util/module.c         | 14 ++++++++------
 2 files changed, 12 insertions(+), 7 deletions(-)

-- 
2.32.0

[PATCH v2 1/1] modules: Improve error message when module is not found

[Jose R. Ziviani]

When a module is not found, specially accelerators, QEMU displays
a error message that not easy to understand[1]. This patch improves
the readability by offering a user-friendly message[2].

This patch also moves the accelerator ops check to runtine (instead
of the original g_assert) because it works better with dynamic
modules.

[1] qemu-system-x86_64 -accel tcg
ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed:
(ops != NULL)
Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces:
assertion failed: (ops != NULL)
    31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...

[2] qemu-system-x86_64 -accel tcg
accel-tcg-x86_64 module is missing, install the package or config the library path correctly.

Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
---
 accel/accel-softmmu.c |  5 ++++-
 util/module.c         | 14 ++++++++------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
index 67276e4f52..52449ac2d0 100644
--- a/accel/accel-softmmu.c
+++ b/accel/accel-softmmu.c
@@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
      * all accelerators need to define ops, providing at least a mandatory
      * non-NULL create_vcpu_thread operation.
      */
-    g_assert(ops != NULL);
+    if (ops == NULL) {
+        exit(1);
+    }
+
     if (ops->ops_init) {
         ops->ops_init(ops);
     }
diff --git a/util/module.c b/util/module.c
index 6bb4ad915a..268a8563fd 100644
--- a/util/module.c
+++ b/util/module.c
@@ -206,13 +206,10 @@ static int module_load_file(const char *fname, bool mayfail, bool export_symbols
 out:
     return ret;
 }
-#endif
 
 bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
 {
     bool success = false;
-
-#ifdef CONFIG_MODULES
     char *fname = NULL;
 #ifdef CONFIG_MODULE_UPGRADES
     char *version_dir;
@@ -300,6 +297,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
 
     if (!success) {
         g_hash_table_remove(loaded_modules, module_name);
+        fprintf(stderr, "%s module is missing, install the "
+                        "package or config the library path "
+                        "correctly.\n", module_name);
         g_free(module_name);
     }
 
@@ -307,12 +307,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
         g_free(dirs[i]);
     }
 
-#endif
     return success;
 }
 
-#ifdef CONFIG_MODULES
-
 static bool module_loaded_qom_all;
 
 void module_load_qom_one(const char *type)
@@ -384,4 +381,9 @@ void qemu_load_module_for_opts(const char *group) {}
 void module_load_qom_one(const char *type) {}
 void module_load_qom_all(void) {}
 
+bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
+{
+    return false;
+}
+
 #endif
-- 
2.32.0

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Gerd Hoffmann]

  Hi,

> --- a/accel/accel-softmmu.c
> +++ b/accel/accel-softmmu.c
> @@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
>       * all accelerators need to define ops, providing at least a mandatory
>       * non-NULL create_vcpu_thread operation.
>       */
> -    g_assert(ops != NULL);
> +    if (ops == NULL) {

Error message here?
Also split accel bits into a separate patch?

>          g_hash_table_remove(loaded_modules, module_name);
> +        fprintf(stderr, "%s module is missing, install the "
> +                        "package or config the library path "
> +                        "correctly.\n", module_name);

This should be error_report(), or maybe warn_report() as this
isn't a fatal error in all cases.  Otherwise looks good to me.

take care,
  Gerd

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Claudio Fontana]

On 7/23/21 8:25 AM, Gerd Hoffmann wrote:
>   Hi,
> 
>> --- a/accel/accel-softmmu.c
>> +++ b/accel/accel-softmmu.c
>> @@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
>>       * all accelerators need to define ops, providing at least a mandatory
>>       * non-NULL create_vcpu_thread operation.
>>       */
>> -    g_assert(ops != NULL);
>> +    if (ops == NULL) {
> 
> Error message here?> Also split accel bits into a separate patch?

If ops is NULL there is something seriously wrong with the code assumptions.
Asserts are in my view the proper way to handle these.

I do not think there is any reason to change this.

> 
>>          g_hash_table_remove(loaded_modules, module_name);
>> +        fprintf(stderr, "%s module is missing, install the "
>> +                        "package or config the library path "
>> +                        "correctly.\n", module_name);
> 
> This should be error_report(), or maybe warn_report() as this
> isn't a fatal error in all cases.  Otherwise looks good to me.
> 
> take care,
>   Gerd
> 

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Markus Armbruster]

"Jose R. Ziviani" <jziviani@suse.de> writes:

> When a module is not found, specially accelerators, QEMU displays
> a error message that not easy to understand[1]. This patch improves
> the readability by offering a user-friendly message[2].
>
> This patch also moves the accelerator ops check to runtine (instead
> of the original g_assert) because it works better with dynamic
> modules.
>
> [1] qemu-system-x86_64 -accel tcg
> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed:
> (ops != NULL)
> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces:
> assertion failed: (ops != NULL)
>     31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...

This isn't an error message, it's a crash :)

> [2] qemu-system-x86_64 -accel tcg
> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.

s/config/configure/

Also drop the period.

>
> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
> ---
>  accel/accel-softmmu.c |  5 ++++-
>  util/module.c         | 14 ++++++++------
>  2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
> index 67276e4f52..52449ac2d0 100644
> --- a/accel/accel-softmmu.c
> +++ b/accel/accel-softmmu.c
> @@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
>       * all accelerators need to define ops, providing at least a mandatory
>       * non-NULL create_vcpu_thread operation.
>       */
> -    g_assert(ops != NULL);
> +    if (ops == NULL) {
> +        exit(1);
> +    }
> +

Not your patch's fault: I'm not sure the comment makes sense.

>      if (ops->ops_init) {
>          ops->ops_init(ops);
>      }
> diff --git a/util/module.c b/util/module.c
> index 6bb4ad915a..268a8563fd 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -206,13 +206,10 @@ static int module_load_file(const char *fname, bool mayfail, bool export_symbols
>  out:
>      return ret;
>  }
> -#endif

Why do you need to mess with the ifdeffery?

>  
>  bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>  {
>      bool success = false;
> -
> -#ifdef CONFIG_MODULES
>      char *fname = NULL;
>  #ifdef CONFIG_MODULE_UPGRADES
>      char *version_dir;
> @@ -300,6 +297,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>  
>      if (!success) {
>          g_hash_table_remove(loaded_modules, module_name);
> +        fprintf(stderr, "%s module is missing, install the "
> +                        "package or config the library path "
> +                        "correctly.\n", module_name);
>          g_free(module_name);
>      }
>  

Again, not your patch's fault: reporting to stderr with fprintf() is
almost always wrong.

When the thing we report is an error, we should use error_report() for
correct formatting.  Likewise, warn_report() for warnings, info_report()
for informational messages.

When the module load is triggered by a monitor command, we probably want
to report problems to the monitor.  error_report() & friends do the
right thing for HMP.  For QMP, you have to use the Error API, i.e. have
the function take an Error ** argument, which the callers propagate all
the way to the QMP core.

To fix this issue, we first need to decide what kind of message this is:
error, warning, something else.

> @@ -307,12 +307,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>          g_free(dirs[i]);
>      }
>  
> -#endif
>      return success;
>  }
>  
> -#ifdef CONFIG_MODULES
> -
>  static bool module_loaded_qom_all;
>  
>  void module_load_qom_one(const char *type)
> @@ -384,4 +381,9 @@ void qemu_load_module_for_opts(const char *group) {}
>  void module_load_qom_one(const char *type) {}
>  void module_load_qom_all(void) {}
>  
> +bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
> +{
> +    return false;
> +}
> +
>  #endif

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Claudio Fontana]

Hello Markus

On 7/23/21 10:28 AM, Markus Armbruster wrote:
> "Jose R. Ziviani" <jziviani@suse.de> writes:
> 
>> When a module is not found, specially accelerators, QEMU displays
>> a error message that not easy to understand[1]. This patch improves
>> the readability by offering a user-friendly message[2].
>>
>> This patch also moves the accelerator ops check to runtine (instead
>> of the original g_assert) because it works better with dynamic
>> modules.
>>
>> [1] qemu-system-x86_64 -accel tcg
>> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed:
>> (ops != NULL)
>> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces:
>> assertion failed: (ops != NULL)
>>     31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
> 
> This isn't an error message, it's a crash :)


Yes, and the reason of the crash is that the code that needs to initialize ops is missing.

The assert needs to _stay there_,
this is a symptom, not the cause of the problem,

we should not be blindly removing asserts.


> 
>> [2] qemu-system-x86_64 -accel tcg
>> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> 
> s/config/configure/
> 
> Also drop the period.
> 
>>
>> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
>> ---
>>  accel/accel-softmmu.c |  5 ++++-
>>  util/module.c         | 14 ++++++++------
>>  2 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
>> index 67276e4f52..52449ac2d0 100644
>> --- a/accel/accel-softmmu.c
>> +++ b/accel/accel-softmmu.c
>> @@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
>>       * all accelerators need to define ops, providing at least a mandatory
>>       * non-NULL create_vcpu_thread operation.
>>       */
>> -    g_assert(ops != NULL);
>> +    if (ops == NULL) {
>> +        exit(1);
>> +    }
>> +
> 
> Not your patch's fault: I'm not sure the comment makes sense.


Yes, the comment makes sense.


> 
>>      if (ops->ops_init) {
>>          ops->ops_init(ops);
>>      }
>> diff --git a/util/module.c b/util/module.c
>> index 6bb4ad915a..268a8563fd 100644
>> --- a/util/module.c
>> +++ b/util/module.c
>> @@ -206,13 +206,10 @@ static int module_load_file(const char *fname, bool mayfail, bool export_symbols
>>  out:
>>      return ret;
>>  }
>> -#endif
> 
> Why do you need to mess with the ifdeffery?
> 
>>  
>>  bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>>  {
>>      bool success = false;
>> -
>> -#ifdef CONFIG_MODULES
>>      char *fname = NULL;
>>  #ifdef CONFIG_MODULE_UPGRADES
>>      char *version_dir;
>> @@ -300,6 +297,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>>  
>>      if (!success) {
>>          g_hash_table_remove(loaded_modules, module_name);
>> +        fprintf(stderr, "%s module is missing, install the "
>> +                        "package or config the library path "
>> +                        "correctly.\n", module_name);
>>          g_free(module_name);
>>      }
>>  
> 
> Again, not your patch's fault: reporting to stderr with fprintf() is
> almost always wrong.
> 
> When the thing we report is an error, we should use error_report() for
> correct formatting.  Likewise, warn_report() for warnings, info_report()
> for informational messages.
> 
> When the module load is triggered by a monitor command, we probably want
> to report problems to the monitor.  error_report() & friends do the
> right thing for HMP.  For QMP, you have to use the Error API, i.e. have
> the function take an Error ** argument, which the callers propagate all
> the way to the QMP core.
> 
> To fix this issue, we first need to decide what kind of message this is:
> error, warning, something else.
> 
>> @@ -307,12 +307,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>>          g_free(dirs[i]);
>>      }
>>  
>> -#endif
>>      return success;
>>  }
>>  
>> -#ifdef CONFIG_MODULES
>> -
>>  static bool module_loaded_qom_all;
>>  
>>  void module_load_qom_one(const char *type)
>> @@ -384,4 +381,9 @@ void qemu_load_module_for_opts(const char *group) {}
>>  void module_load_qom_one(const char *type) {}
>>  void module_load_qom_all(void) {}
>>  
>> +bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>> +{
>> +    return false;
>> +}
>> +
>>  #endif
> 

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Claudio Fontana]

On 7/23/21 12:09 AM, Jose R. Ziviani wrote:
> When a module is not found, specially accelerators, QEMU displays
> a error message that not easy to understand[1]. This patch improves
> the readability by offering a user-friendly message[2].
> 
> This patch also moves the accelerator ops check to runtine (instead
> of the original g_assert) because it works better with dynamic
> modules.
> 
> [1] qemu-system-x86_64 -accel tcg
> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed:
> (ops != NULL)
> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces:
> assertion failed: (ops != NULL)
>     31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
> 
> [2] qemu-system-x86_64 -accel tcg
> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> 
> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
> ---
>  accel/accel-softmmu.c |  5 ++++-
>  util/module.c         | 14 ++++++++------
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
> index 67276e4f52..52449ac2d0 100644
> --- a/accel/accel-softmmu.c
> +++ b/accel/accel-softmmu.c
> @@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
>       * all accelerators need to define ops, providing at least a mandatory
>       * non-NULL create_vcpu_thread operation.
>       */
> -    g_assert(ops != NULL);
> +    if (ops == NULL) {
> +        exit(1);
> +    }
> +


Ah, again, why?
This change looks wrong to me, 

the ops code should be present when ops interfaces are initialized:
it should be a code level assertion, as it has to do with the proper order of initializations in QEMU,

why would we want to do anything else but to assert here?

Am I blind to something obvious?

>      if (ops->ops_init) {
>          ops->ops_init(ops);
>      }
> diff --git a/util/module.c b/util/module.c
> index 6bb4ad915a..268a8563fd 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -206,13 +206,10 @@ static int module_load_file(const char *fname, bool mayfail, bool export_symbols
>  out:
>      return ret;
>  }
> -#endif
>  
>  bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>  {
>      bool success = false;
> -
> -#ifdef CONFIG_MODULES
>      char *fname = NULL;
>  #ifdef CONFIG_MODULE_UPGRADES
>      char *version_dir;
> @@ -300,6 +297,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>  
>      if (!success) {
>          g_hash_table_remove(loaded_modules, module_name);
> +        fprintf(stderr, "%s module is missing, install the "
> +                        "package or config the library path "
> +                        "correctly.\n", module_name);
>          g_free(module_name);
>      }
>  
> @@ -307,12 +307,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>          g_free(dirs[i]);
>      }
>  
> -#endif
>      return success;
>  }
>  
> -#ifdef CONFIG_MODULES
> -
>  static bool module_loaded_qom_all;
>  
>  void module_load_qom_one(const char *type)
> @@ -384,4 +381,9 @@ void qemu_load_module_for_opts(const char *group) {}
>  void module_load_qom_one(const char *type) {}
>  void module_load_qom_all(void) {}
>  
> +bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
> +{
> +    return false;
> +}
> +
>  #endif
> 

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Gerd Hoffmann]

> > -    g_assert(ops != NULL);
> > +    if (ops == NULL) {
> > +        exit(1);
> > +    }
> > +
> 
> 
> Ah, again, why?
> This change looks wrong to me, 
> 
> the ops code should be present when ops interfaces are initialized:
> it should be a code level assertion, as it has to do with the proper order of initializations in QEMU,
> 
> why would we want to do anything else but to assert here?
> 
> Am I blind to something obvious?

Building tcg accel ops modular moves that from coding error to possible
user error (user wants use tcg but has qemu-accel-tcg-$arch.rpm not
installed).

The second part of the patch makes qemu print a message on the failed
module load, so the user would have a chance to figure where the assert
comes from, but replacing the assert with a more friendly message still
makes sense to me.

take care,
  Gerd

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Claudio Fontana]

On 7/23/21 11:52 AM, Gerd Hoffmann wrote:
>>> -    g_assert(ops != NULL);
>>> +    if (ops == NULL) {
>>> +        exit(1);
>>> +    }
>>> +
>>
>>
>> Ah, again, why?
>> This change looks wrong to me, 
>>
>> the ops code should be present when ops interfaces are initialized:
>> it should be a code level assertion, as it has to do with the proper order of initializations in QEMU,
>>
>> why would we want to do anything else but to assert here?
>>
>> Am I blind to something obvious?
> 
> Building tcg accel ops modular moves that from coding error to possible
> user error (user wants use tcg but has qemu-accel-tcg-$arch.rpm not
> installed).

Sorry but without more background I don't buy it.

If ops is null at the time accel_init_interfaces is called,
it means that we are trying to initialize the board (for softmmu)
with an accelerator already selected, and without an accelerator actually available.

The problem has happened already a long time before we get here.

When we check for viable accelerators, in configure_accelerators,
we should check that the code is actually there, before choosing it as a viable accelerator.

If we march on and start initializing the machine with an accelerator that is not available,
of course things will start failing left and right.

If things like:

bool have_tcg = accel_find("tcg");

return true when the code is actually not there, there seems to be a larger issue to solve.

I think we need to think more broadly about this.

Thanks,

Claudio


> 
> The second part of the patch makes qemu print a message on the failed
> module load, so the user would have a chance to figure where the assert
> comes from, but replacing the assert with a more friendly message still
> makes sense to me.
> 
> take care,
>   Gerd
> 

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Claudio Fontana]

On 7/23/21 1:20 PM, Claudio Fontana wrote:
> On 7/23/21 11:52 AM, Gerd Hoffmann wrote:
>>>> -    g_assert(ops != NULL);
>>>> +    if (ops == NULL) {
>>>> +        exit(1);
>>>> +    }
>>>> +
>>>
>>>
>>> Ah, again, why?
>>> This change looks wrong to me, 
>>>
>>> the ops code should be present when ops interfaces are initialized:
>>> it should be a code level assertion, as it has to do with the proper order of initializations in QEMU,
>>>
>>> why would we want to do anything else but to assert here?
>>>
>>> Am I blind to something obvious?
>>
>> Building tcg accel ops modular moves that from coding error to possible
>> user error (user wants use tcg but has qemu-accel-tcg-$arch.rpm not
>> installed).
> 
> Sorry but without more background I don't buy it.
> 
> If ops is null at the time accel_init_interfaces is called,
> it means that we are trying to initialize the board (for softmmu)
> with an accelerator already selected, and without an accelerator actually available.
> 
> The problem has happened already a long time before we get here.
> 
> When we check for viable accelerators, in configure_accelerators,
> we should check that the code is actually there, before choosing it as a viable accelerator.
> 
> If we march on and start initializing the machine with an accelerator that is not available,
> of course things will start failing left and right.
> 
> If things like:
> 
> bool have_tcg = accel_find("tcg");
> 
> return true when the code is actually not there, there seems to be a larger issue to solve.
> 
> I think we need to think more broadly about this.

Overall, building the whole code base to be modular,
and then _not_ including unwanted modules in the base distro package,

is the whole point of going through this at all.

QEMU should gracefully figure out that indeed, the module is not there -> TCG is not there.

So we need more work to make this actually work right.

> Thanks,
> 
> Claudio
> 
> 
>>
>> The second part of the patch makes qemu print a message on the failed
>> module load, so the user would have a chance to figure where the assert
>> comes from, but replacing the assert with a more friendly message still
>> makes sense to me.
>>
>> take care,
>>   Gerd
>>
> 
> 

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Gerd Hoffmann]

> > bool have_tcg = accel_find("tcg");
> > 
> > return true when the code is actually not there, there seems to be a larger issue to solve.
> > 
> > I think we need to think more broadly about this.
> 
> Overall, building the whole code base to be modular,
> and then _not_ including unwanted modules in the base distro package,
> is the whole point of going through this at all.

Yes.

Right now we are only half-through.  tcg-accel-ops is modular, but
tcg-accel is not (yet).  Which I guess is why the assert() can trigger
now.

So add a patch with error message and a FIXME comment, which we can
revert when isn't needed any more?

> So we need more work to make this actually work right.

Yes.  I want have all of tcg in the tcg accel module, not only parts of
it, but that needs some more refactoring.  I'll go start looking at this
once I managed to wade through my vacation backlog.

take care,
  Gerd

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Jose R. Ziviani]

[-- Attachment #1: Type: text/plain, Size: 5177 bytes --]

On Fri, Jul 23, 2021 at 11:41:19AM +0200, Claudio Fontana wrote:
> On 7/23/21 12:09 AM, Jose R. Ziviani wrote:
> > When a module is not found, specially accelerators, QEMU displays
> > a error message that not easy to understand[1]. This patch improves
> > the readability by offering a user-friendly message[2].
> > 
> > This patch also moves the accelerator ops check to runtine (instead
> > of the original g_assert) because it works better with dynamic
> > modules.
> > 
> > [1] qemu-system-x86_64 -accel tcg
> > ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed:
> > (ops != NULL)
> > Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces:
> > assertion failed: (ops != NULL)
> >     31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
> > 
> > [2] qemu-system-x86_64 -accel tcg
> > accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> > 
> > Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
> > ---
> >  accel/accel-softmmu.c |  5 ++++-
> >  util/module.c         | 14 ++++++++------
> >  2 files changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
> > index 67276e4f52..52449ac2d0 100644
> > --- a/accel/accel-softmmu.c
> > +++ b/accel/accel-softmmu.c
> > @@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
> >       * all accelerators need to define ops, providing at least a mandatory
> >       * non-NULL create_vcpu_thread operation.
> >       */
> > -    g_assert(ops != NULL);
> > +    if (ops == NULL) {
> > +        exit(1);
> > +    }
> > +
> 
> 
> Ah, again, why?
> This change looks wrong to me, 
> 
> the ops code should be present when ops interfaces are initialized:
> it should be a code level assertion, as it has to do with the proper order of initializations in QEMU,
> 
> why would we want to do anything else but to assert here?
> 
> Am I blind to something obvious?

Hello!

Thank you for reviewing it!

The problem is that if your TCG module is not installed and you start
QEMU like:

./qemu-system-x86_64 -accel tcg

You'll get the error message + a crash with a core dump:

accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
**
ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
[1]    5740 IOT instruction (core dumped)  ./qemu-system-x86_64 -accel tcg

I was digging a little bit more in order to move this responsibility to
module.c but there isn't enough information there to safely exit() in
all situations that a module may be loaded. As Gerd mentioned, more work
is needed in order to achieve that.

However, it's not nice to have a crash due to an optional module missing.
It's specially confusing because TCG has always been native. Considering
also that we're already in hard freeze for 6.1, I thought to have this
simpler check instead.

What do you think if we have something like:

/* FIXME: this isn't the right place to handle a missing module and
   must be reverted when the module refactoring is completely done */
#ifdef CONFIG_MODULES
if (ops == NULL) {
    exit(1);
}
#else
g_assert(ops != NULL);
#endif

Regards!

> 
> >      if (ops->ops_init) {
> >          ops->ops_init(ops);
> >      }
> > diff --git a/util/module.c b/util/module.c
> > index 6bb4ad915a..268a8563fd 100644
> > --- a/util/module.c
> > +++ b/util/module.c
> > @@ -206,13 +206,10 @@ static int module_load_file(const char *fname, bool mayfail, bool export_symbols
> >  out:
> >      return ret;
> >  }
> > -#endif
> >  
> >  bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
> >  {
> >      bool success = false;
> > -
> > -#ifdef CONFIG_MODULES
> >      char *fname = NULL;
> >  #ifdef CONFIG_MODULE_UPGRADES
> >      char *version_dir;
> > @@ -300,6 +297,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
> >  
> >      if (!success) {
> >          g_hash_table_remove(loaded_modules, module_name);
> > +        fprintf(stderr, "%s module is missing, install the "
> > +                        "package or config the library path "
> > +                        "correctly.\n", module_name);
> >          g_free(module_name);
> >      }
> >  
> > @@ -307,12 +307,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
> >          g_free(dirs[i]);
> >      }
> >  
> > -#endif
> >      return success;
> >  }
> >  
> > -#ifdef CONFIG_MODULES
> > -
> >  static bool module_loaded_qom_all;
> >  
> >  void module_load_qom_one(const char *type)
> > @@ -384,4 +381,9 @@ void qemu_load_module_for_opts(const char *group) {}
> >  void module_load_qom_one(const char *type) {}
> >  void module_load_qom_all(void) {}
> >  
> > +bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
> > +{
> > +    return false;
> > +}
> > +
> >  #endif
> > 
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Claudio Fontana]

On 7/23/21 3:50 PM, Jose R. Ziviani wrote:
> On Fri, Jul 23, 2021 at 11:41:19AM +0200, Claudio Fontana wrote:
>> On 7/23/21 12:09 AM, Jose R. Ziviani wrote:
>>> When a module is not found, specially accelerators, QEMU displays
>>> a error message that not easy to understand[1]. This patch improves
>>> the readability by offering a user-friendly message[2].
>>>
>>> This patch also moves the accelerator ops check to runtine (instead
>>> of the original g_assert) because it works better with dynamic
>>> modules.
>>>
>>> [1] qemu-system-x86_64 -accel tcg
>>> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed:
>>> (ops != NULL)
>>> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces:
>>> assertion failed: (ops != NULL)
>>>     31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
>>>
>>> [2] qemu-system-x86_64 -accel tcg
>>> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
>>>
>>> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
>>> ---
>>>  accel/accel-softmmu.c |  5 ++++-
>>>  util/module.c         | 14 ++++++++------
>>>  2 files changed, 12 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
>>> index 67276e4f52..52449ac2d0 100644
>>> --- a/accel/accel-softmmu.c
>>> +++ b/accel/accel-softmmu.c
>>> @@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
>>>       * all accelerators need to define ops, providing at least a mandatory
>>>       * non-NULL create_vcpu_thread operation.
>>>       */
>>> -    g_assert(ops != NULL);
>>> +    if (ops == NULL) {
>>> +        exit(1);
>>> +    }
>>> +
>>
>>
>> Ah, again, why?
>> This change looks wrong to me, 
>>
>> the ops code should be present when ops interfaces are initialized:
>> it should be a code level assertion, as it has to do with the proper order of initializations in QEMU,
>>
>> why would we want to do anything else but to assert here?
>>
>> Am I blind to something obvious?
> 
> Hello!
> 
> Thank you for reviewing it!
> 
> The problem is that if your TCG module is not installed and you start
> QEMU like:
> 
> ./qemu-system-x86_64 -accel tcg
> 
> You'll get the error message + a crash with a core dump:
> 
> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> **
> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
> [1]    5740 IOT instruction (core dumped)  ./qemu-system-x86_64 -accel tcg
> 
> I was digging a little bit more in order to move this responsibility to
> module.c but there isn't enough information there to safely exit() in
> all situations that a module may be loaded. As Gerd mentioned, more work
> is needed in order to achieve that.
> 
> However, it's not nice to have a crash due to an optional module missing.
> It's specially confusing because TCG has always been native. Considering
> also that we're already in hard freeze for 6.1, I thought to have this
> simpler check instead.
> 
> What do you think if we have something like:
> 
> /* FIXME: this isn't the right place to handle a missing module and
>    must be reverted when the module refactoring is completely done */
> #ifdef CONFIG_MODULES
> if (ops == NULL) {
>     exit(1);
> }
> #else
> g_assert(ops != NULL);
> #endif
> 
> Regards!


For the normal builds (without modular tcg), this issue does not appear right?
So maybe there is no pressure to change anything for 6.1, and we can work on the right solution on master?

Not sure how we consider this feature for 6.1, I guess it is still not a supported option,
(is there any CI for this? Probably not right?),

so I would consider building modular tcg in 6.1 as "experimental", and we can proceed to do the right thing on master?

Thanks,

Claudio

> 
>>
>>>      if (ops->ops_init) {
>>>          ops->ops_init(ops);
>>>      }
>>> diff --git a/util/module.c b/util/module.c
>>> index 6bb4ad915a..268a8563fd 100644
>>> --- a/util/module.c
>>> +++ b/util/module.c
>>> @@ -206,13 +206,10 @@ static int module_load_file(const char *fname, bool mayfail, bool export_symbols
>>>  out:
>>>      return ret;
>>>  }
>>> -#endif
>>>  
>>>  bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>>>  {
>>>      bool success = false;
>>> -
>>> -#ifdef CONFIG_MODULES
>>>      char *fname = NULL;
>>>  #ifdef CONFIG_MODULE_UPGRADES
>>>      char *version_dir;
>>> @@ -300,6 +297,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>>>  
>>>      if (!success) {
>>>          g_hash_table_remove(loaded_modules, module_name);
>>> +        fprintf(stderr, "%s module is missing, install the "
>>> +                        "package or config the library path "
>>> +                        "correctly.\n", module_name);
>>>          g_free(module_name);
>>>      }
>>>  
>>> @@ -307,12 +307,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>>>          g_free(dirs[i]);
>>>      }
>>>  
>>> -#endif
>>>      return success;
>>>  }
>>>  
>>> -#ifdef CONFIG_MODULES
>>> -
>>>  static bool module_loaded_qom_all;
>>>  
>>>  void module_load_qom_one(const char *type)
>>> @@ -384,4 +381,9 @@ void qemu_load_module_for_opts(const char *group) {}
>>>  void module_load_qom_one(const char *type) {}
>>>  void module_load_qom_all(void) {}
>>>  
>>> +bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>>> +{
>>> +    return false;
>>> +}
>>> +
>>>  #endif
>>>
>>

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Philippe Mathieu-Daudé]

On 7/23/21 4:02 PM, Claudio Fontana wrote:
> On 7/23/21 3:50 PM, Jose R. Ziviani wrote:
>> On Fri, Jul 23, 2021 at 11:41:19AM +0200, Claudio Fontana wrote:
>>> On 7/23/21 12:09 AM, Jose R. Ziviani wrote:
>>>> When a module is not found, specially accelerators, QEMU displays
>>>> a error message that not easy to understand[1]. This patch improves
>>>> the readability by offering a user-friendly message[2].
>>>>
>>>> This patch also moves the accelerator ops check to runtine (instead
>>>> of the original g_assert) because it works better with dynamic
>>>> modules.
>>>>
>>>> [1] qemu-system-x86_64 -accel tcg
>>>> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed:
>>>> (ops != NULL)
>>>> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces:
>>>> assertion failed: (ops != NULL)
>>>>     31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
>>>>
>>>> [2] qemu-system-x86_64 -accel tcg
>>>> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
>>>>
>>>> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
>>>> ---
>>>>  accel/accel-softmmu.c |  5 ++++-
>>>>  util/module.c         | 14 ++++++++------
>>>>  2 files changed, 12 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
>>>> index 67276e4f52..52449ac2d0 100644
>>>> --- a/accel/accel-softmmu.c
>>>> +++ b/accel/accel-softmmu.c
>>>> @@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
>>>>       * all accelerators need to define ops, providing at least a mandatory
>>>>       * non-NULL create_vcpu_thread operation.
>>>>       */
>>>> -    g_assert(ops != NULL);
>>>> +    if (ops == NULL) {
>>>> +        exit(1);
>>>> +    }
>>>> +
>>>
>>>
>>> Ah, again, why?
>>> This change looks wrong to me, 
>>>
>>> the ops code should be present when ops interfaces are initialized:
>>> it should be a code level assertion, as it has to do with the proper order of initializations in QEMU,
>>>
>>> why would we want to do anything else but to assert here?
>>>
>>> Am I blind to something obvious?
>>
>> Hello!
>>
>> Thank you for reviewing it!
>>
>> The problem is that if your TCG module is not installed and you start
>> QEMU like:
>>
>> ./qemu-system-x86_64 -accel tcg
>>
>> You'll get the error message + a crash with a core dump:
>>
>> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
>> **
>> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
>> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
>> [1]    5740 IOT instruction (core dumped)  ./qemu-system-x86_64 -accel tcg
>>
>> I was digging a little bit more in order to move this responsibility to
>> module.c but there isn't enough information there to safely exit() in
>> all situations that a module may be loaded. As Gerd mentioned, more work
>> is needed in order to achieve that.
>>
>> However, it's not nice to have a crash due to an optional module missing.
>> It's specially confusing because TCG has always been native. Considering
>> also that we're already in hard freeze for 6.1, I thought to have this
>> simpler check instead.
>>
>> What do you think if we have something like:
>>
>> /* FIXME: this isn't the right place to handle a missing module and
>>    must be reverted when the module refactoring is completely done */
>> #ifdef CONFIG_MODULES
>> if (ops == NULL) {
>>     exit(1);
>> }
>> #else
>> g_assert(ops != NULL);
>> #endif
>>
>> Regards!
> 
> 
> For the normal builds (without modular tcg), this issue does not appear right?
> So maybe there is no pressure to change anything for 6.1, and we can work on the right solution on master?
> 
> Not sure how we consider this feature for 6.1, I guess it is still not a supported option,
> (is there any CI for this? Probably not right?),
> 
> so I would consider building modular tcg in 6.1 as "experimental", and we can proceed to do the right thing on master?

+1 as I don't see this feature ready.

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Jose R. Ziviani]

[-- Attachment #1: Type: text/plain, Size: 6654 bytes --]

On Fri, Jul 23, 2021 at 04:02:26PM +0200, Claudio Fontana wrote:
> On 7/23/21 3:50 PM, Jose R. Ziviani wrote:
> > On Fri, Jul 23, 2021 at 11:41:19AM +0200, Claudio Fontana wrote:
> >> On 7/23/21 12:09 AM, Jose R. Ziviani wrote:
> >>> When a module is not found, specially accelerators, QEMU displays
> >>> a error message that not easy to understand[1]. This patch improves
> >>> the readability by offering a user-friendly message[2].
> >>>
> >>> This patch also moves the accelerator ops check to runtine (instead
> >>> of the original g_assert) because it works better with dynamic
> >>> modules.
> >>>
> >>> [1] qemu-system-x86_64 -accel tcg
> >>> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed:
> >>> (ops != NULL)
> >>> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces:
> >>> assertion failed: (ops != NULL)
> >>>     31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
> >>>
> >>> [2] qemu-system-x86_64 -accel tcg
> >>> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> >>>
> >>> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
> >>> ---
> >>>  accel/accel-softmmu.c |  5 ++++-
> >>>  util/module.c         | 14 ++++++++------
> >>>  2 files changed, 12 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
> >>> index 67276e4f52..52449ac2d0 100644
> >>> --- a/accel/accel-softmmu.c
> >>> +++ b/accel/accel-softmmu.c
> >>> @@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
> >>>       * all accelerators need to define ops, providing at least a mandatory
> >>>       * non-NULL create_vcpu_thread operation.
> >>>       */
> >>> -    g_assert(ops != NULL);
> >>> +    if (ops == NULL) {
> >>> +        exit(1);
> >>> +    }
> >>> +
> >>
> >>
> >> Ah, again, why?
> >> This change looks wrong to me, 
> >>
> >> the ops code should be present when ops interfaces are initialized:
> >> it should be a code level assertion, as it has to do with the proper order of initializations in QEMU,
> >>
> >> why would we want to do anything else but to assert here?
> >>
> >> Am I blind to something obvious?
> > 
> > Hello!
> > 
> > Thank you for reviewing it!
> > 
> > The problem is that if your TCG module is not installed and you start
> > QEMU like:
> > 
> > ./qemu-system-x86_64 -accel tcg
> > 
> > You'll get the error message + a crash with a core dump:
> > 
> > accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> > **
> > ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
> > Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
> > [1]    5740 IOT instruction (core dumped)  ./qemu-system-x86_64 -accel tcg
> > 
> > I was digging a little bit more in order to move this responsibility to
> > module.c but there isn't enough information there to safely exit() in
> > all situations that a module may be loaded. As Gerd mentioned, more work
> > is needed in order to achieve that.
> > 
> > However, it's not nice to have a crash due to an optional module missing.
> > It's specially confusing because TCG has always been native. Considering
> > also that we're already in hard freeze for 6.1, I thought to have this
> > simpler check instead.
> > 
> > What do you think if we have something like:
> > 
> > /* FIXME: this isn't the right place to handle a missing module and
> >    must be reverted when the module refactoring is completely done */
> > #ifdef CONFIG_MODULES
> > if (ops == NULL) {
> >     exit(1);
> > }
> > #else
> > g_assert(ops != NULL);
> > #endif
> > 
> > Regards!
> 
> 
> For the normal builds (without modular tcg), this issue does not appear right?

Yes, but OpenSUSE already builds with --enable-modules, we've already been shipping
several modules as optional RPMs, like qemu-hw-display-virtio-gpu for example. I sent
a patch some weeks ago to add "--enable-tcg-builtin" in the build system but there're
more work required in that area as well.

> So maybe there is no pressure to change anything for 6.1, and we can work on the right solution on master?
> 
> Not sure how we consider this feature for 6.1, I guess it is still not a supported option,
> (is there any CI for this? Probably not right?),
> 
> so I would consider building modular tcg in 6.1 as "experimental", and we can proceed to do the right thing on master?

For OpenSUSE Tumbleweed, when we release QEMU 6.1, I can add my patch to
"--enable-tcg-builtin" for downstream only. I'm fine with it too.

Thank you!!!

> 
> Thanks,
> 
> Claudio
> 
> > 
> >>
> >>>      if (ops->ops_init) {
> >>>          ops->ops_init(ops);
> >>>      }
> >>> diff --git a/util/module.c b/util/module.c
> >>> index 6bb4ad915a..268a8563fd 100644
> >>> --- a/util/module.c
> >>> +++ b/util/module.c
> >>> @@ -206,13 +206,10 @@ static int module_load_file(const char *fname, bool mayfail, bool export_symbols
> >>>  out:
> >>>      return ret;
> >>>  }
> >>> -#endif
> >>>  
> >>>  bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
> >>>  {
> >>>      bool success = false;
> >>> -
> >>> -#ifdef CONFIG_MODULES
> >>>      char *fname = NULL;
> >>>  #ifdef CONFIG_MODULE_UPGRADES
> >>>      char *version_dir;
> >>> @@ -300,6 +297,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
> >>>  
> >>>      if (!success) {
> >>>          g_hash_table_remove(loaded_modules, module_name);
> >>> +        fprintf(stderr, "%s module is missing, install the "
> >>> +                        "package or config the library path "
> >>> +                        "correctly.\n", module_name);
> >>>          g_free(module_name);
> >>>      }
> >>>  
> >>> @@ -307,12 +307,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
> >>>          g_free(dirs[i]);
> >>>      }
> >>>  
> >>> -#endif
> >>>      return success;
> >>>  }
> >>>  
> >>> -#ifdef CONFIG_MODULES
> >>> -
> >>>  static bool module_loaded_qom_all;
> >>>  
> >>>  void module_load_qom_one(const char *type)
> >>> @@ -384,4 +381,9 @@ void qemu_load_module_for_opts(const char *group) {}
> >>>  void module_load_qom_one(const char *type) {}
> >>>  void module_load_qom_all(void) {}
> >>>  
> >>> +bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
> >>> +{
> >>> +    return false;
> >>> +}
> >>> +
> >>>  #endif
> >>>
> >>
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Claudio Fontana]

On 7/23/21 4:36 PM, Jose R. Ziviani wrote:
> On Fri, Jul 23, 2021 at 04:02:26PM +0200, Claudio Fontana wrote:
>> On 7/23/21 3:50 PM, Jose R. Ziviani wrote:
>>> On Fri, Jul 23, 2021 at 11:41:19AM +0200, Claudio Fontana wrote:
>>>> On 7/23/21 12:09 AM, Jose R. Ziviani wrote:
>>>>> When a module is not found, specially accelerators, QEMU displays
>>>>> a error message that not easy to understand[1]. This patch improves
>>>>> the readability by offering a user-friendly message[2].
>>>>>
>>>>> This patch also moves the accelerator ops check to runtine (instead
>>>>> of the original g_assert) because it works better with dynamic
>>>>> modules.
>>>>>
>>>>> [1] qemu-system-x86_64 -accel tcg
>>>>> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed:
>>>>> (ops != NULL)
>>>>> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces:
>>>>> assertion failed: (ops != NULL)
>>>>>     31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
>>>>>
>>>>> [2] qemu-system-x86_64 -accel tcg
>>>>> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
>>>>>
>>>>> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
>>>>> ---
>>>>>  accel/accel-softmmu.c |  5 ++++-
>>>>>  util/module.c         | 14 ++++++++------
>>>>>  2 files changed, 12 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
>>>>> index 67276e4f52..52449ac2d0 100644
>>>>> --- a/accel/accel-softmmu.c
>>>>> +++ b/accel/accel-softmmu.c
>>>>> @@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
>>>>>       * all accelerators need to define ops, providing at least a mandatory
>>>>>       * non-NULL create_vcpu_thread operation.
>>>>>       */
>>>>> -    g_assert(ops != NULL);
>>>>> +    if (ops == NULL) {
>>>>> +        exit(1);
>>>>> +    }
>>>>> +
>>>>
>>>>
>>>> Ah, again, why?
>>>> This change looks wrong to me, 
>>>>
>>>> the ops code should be present when ops interfaces are initialized:
>>>> it should be a code level assertion, as it has to do with the proper order of initializations in QEMU,
>>>>
>>>> why would we want to do anything else but to assert here?
>>>>
>>>> Am I blind to something obvious?
>>>
>>> Hello!
>>>
>>> Thank you for reviewing it!
>>>
>>> The problem is that if your TCG module is not installed and you start
>>> QEMU like:
>>>
>>> ./qemu-system-x86_64 -accel tcg
>>>
>>> You'll get the error message + a crash with a core dump:
>>>
>>> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
>>> **
>>> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
>>> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
>>> [1]    5740 IOT instruction (core dumped)  ./qemu-system-x86_64 -accel tcg
>>>
>>> I was digging a little bit more in order to move this responsibility to
>>> module.c but there isn't enough information there to safely exit() in
>>> all situations that a module may be loaded. As Gerd mentioned, more work
>>> is needed in order to achieve that.
>>>
>>> However, it's not nice to have a crash due to an optional module missing.
>>> It's specially confusing because TCG has always been native. Considering
>>> also that we're already in hard freeze for 6.1, I thought to have this
>>> simpler check instead.
>>>
>>> What do you think if we have something like:
>>>
>>> /* FIXME: this isn't the right place to handle a missing module and
>>>    must be reverted when the module refactoring is completely done */
>>> #ifdef CONFIG_MODULES
>>> if (ops == NULL) {
>>>     exit(1);
>>> }
>>> #else
>>> g_assert(ops != NULL);
>>> #endif
>>>
>>> Regards!
>>
>>
>> For the normal builds (without modular tcg), this issue does not appear right?
> 
> Yes, but OpenSUSE already builds with --enable-modules, we've already been shipping
> several modules as optional RPMs, like qemu-hw-display-virtio-gpu for example. I sent
> a patch some weeks ago to add "--enable-tcg-builtin" in the build system but there're
> more work required in that area as well.
> 
>> So maybe there is no pressure to change anything for 6.1, and we can work on the right solution on master?
>>
>> Not sure how we consider this feature for 6.1, I guess it is still not a supported option,
>> (is there any CI for this? Probably not right?),
>>
>> so I would consider building modular tcg in 6.1 as "experimental", and we can proceed to do the right thing on master?
> 
> For OpenSUSE Tumbleweed, when we release QEMU 6.1, I can add my patch to
> "--enable-tcg-builtin" for downstream only. I'm fine with it too.

Hi Jose,

indeed if we need to do something downstream it's fine,
but lets keep the discussion on this list focused on what is best for upstream.

Ciao, thanks :-)

Claudio

> 
> Thank you!!!
> 
>>
>> Thanks,
>>
>> Claudio
>>
>>>
>>>>
>>>>>      if (ops->ops_init) {
>>>>>          ops->ops_init(ops);
>>>>>      }
>>>>> diff --git a/util/module.c b/util/module.c
>>>>> index 6bb4ad915a..268a8563fd 100644
>>>>> --- a/util/module.c
>>>>> +++ b/util/module.c
>>>>> @@ -206,13 +206,10 @@ static int module_load_file(const char *fname, bool mayfail, bool export_symbols
>>>>>  out:
>>>>>      return ret;
>>>>>  }
>>>>> -#endif
>>>>>  
>>>>>  bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>>>>>  {
>>>>>      bool success = false;
>>>>> -
>>>>> -#ifdef CONFIG_MODULES
>>>>>      char *fname = NULL;
>>>>>  #ifdef CONFIG_MODULE_UPGRADES
>>>>>      char *version_dir;
>>>>> @@ -300,6 +297,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>>>>>  
>>>>>      if (!success) {
>>>>>          g_hash_table_remove(loaded_modules, module_name);
>>>>> +        fprintf(stderr, "%s module is missing, install the "
>>>>> +                        "package or config the library path "
>>>>> +                        "correctly.\n", module_name);
>>>>>          g_free(module_name);
>>>>>      }
>>>>>  
>>>>> @@ -307,12 +307,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>>>>>          g_free(dirs[i]);
>>>>>      }
>>>>>  
>>>>> -#endif
>>>>>      return success;
>>>>>  }
>>>>>  
>>>>> -#ifdef CONFIG_MODULES
>>>>> -
>>>>>  static bool module_loaded_qom_all;
>>>>>  
>>>>>  void module_load_qom_one(const char *type)
>>>>> @@ -384,4 +381,9 @@ void qemu_load_module_for_opts(const char *group) {}
>>>>>  void module_load_qom_one(const char *type) {}
>>>>>  void module_load_qom_all(void) {}
>>>>>  
>>>>> +bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>>>>> +{
>>>>> +    return false;
>>>>> +}
>>>>> +
>>>>>  #endif
>>>>>
>>>>
>>

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Jose R. Ziviani]

[-- Attachment #1: Type: text/plain, Size: 7502 bytes --]

On Fri, Jul 23, 2021 at 05:27:25PM +0200, Claudio Fontana wrote:
> On 7/23/21 4:36 PM, Jose R. Ziviani wrote:
> > On Fri, Jul 23, 2021 at 04:02:26PM +0200, Claudio Fontana wrote:
> >> On 7/23/21 3:50 PM, Jose R. Ziviani wrote:
> >>> On Fri, Jul 23, 2021 at 11:41:19AM +0200, Claudio Fontana wrote:
> >>>> On 7/23/21 12:09 AM, Jose R. Ziviani wrote:
> >>>>> When a module is not found, specially accelerators, QEMU displays
> >>>>> a error message that not easy to understand[1]. This patch improves
> >>>>> the readability by offering a user-friendly message[2].
> >>>>>
> >>>>> This patch also moves the accelerator ops check to runtine (instead
> >>>>> of the original g_assert) because it works better with dynamic
> >>>>> modules.
> >>>>>
> >>>>> [1] qemu-system-x86_64 -accel tcg
> >>>>> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed:
> >>>>> (ops != NULL)
> >>>>> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces:
> >>>>> assertion failed: (ops != NULL)
> >>>>>     31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
> >>>>>
> >>>>> [2] qemu-system-x86_64 -accel tcg
> >>>>> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> >>>>>
> >>>>> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
> >>>>> ---
> >>>>>  accel/accel-softmmu.c |  5 ++++-
> >>>>>  util/module.c         | 14 ++++++++------
> >>>>>  2 files changed, 12 insertions(+), 7 deletions(-)
> >>>>>
> >>>>> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
> >>>>> index 67276e4f52..52449ac2d0 100644
> >>>>> --- a/accel/accel-softmmu.c
> >>>>> +++ b/accel/accel-softmmu.c
> >>>>> @@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
> >>>>>       * all accelerators need to define ops, providing at least a mandatory
> >>>>>       * non-NULL create_vcpu_thread operation.
> >>>>>       */
> >>>>> -    g_assert(ops != NULL);
> >>>>> +    if (ops == NULL) {
> >>>>> +        exit(1);
> >>>>> +    }
> >>>>> +
> >>>>
> >>>>
> >>>> Ah, again, why?
> >>>> This change looks wrong to me, 
> >>>>
> >>>> the ops code should be present when ops interfaces are initialized:
> >>>> it should be a code level assertion, as it has to do with the proper order of initializations in QEMU,
> >>>>
> >>>> why would we want to do anything else but to assert here?
> >>>>
> >>>> Am I blind to something obvious?
> >>>
> >>> Hello!
> >>>
> >>> Thank you for reviewing it!
> >>>
> >>> The problem is that if your TCG module is not installed and you start
> >>> QEMU like:
> >>>
> >>> ./qemu-system-x86_64 -accel tcg
> >>>
> >>> You'll get the error message + a crash with a core dump:
> >>>
> >>> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> >>> **
> >>> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
> >>> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
> >>> [1]    5740 IOT instruction (core dumped)  ./qemu-system-x86_64 -accel tcg
> >>>
> >>> I was digging a little bit more in order to move this responsibility to
> >>> module.c but there isn't enough information there to safely exit() in
> >>> all situations that a module may be loaded. As Gerd mentioned, more work
> >>> is needed in order to achieve that.
> >>>
> >>> However, it's not nice to have a crash due to an optional module missing.
> >>> It's specially confusing because TCG has always been native. Considering
> >>> also that we're already in hard freeze for 6.1, I thought to have this
> >>> simpler check instead.
> >>>
> >>> What do you think if we have something like:
> >>>
> >>> /* FIXME: this isn't the right place to handle a missing module and
> >>>    must be reverted when the module refactoring is completely done */
> >>> #ifdef CONFIG_MODULES
> >>> if (ops == NULL) {
> >>>     exit(1);
> >>> }
> >>> #else
> >>> g_assert(ops != NULL);
> >>> #endif
> >>>
> >>> Regards!
> >>
> >>
> >> For the normal builds (without modular tcg), this issue does not appear right?
> > 
> > Yes, but OpenSUSE already builds with --enable-modules, we've already been shipping
> > several modules as optional RPMs, like qemu-hw-display-virtio-gpu for example. I sent
> > a patch some weeks ago to add "--enable-tcg-builtin" in the build system but there're
> > more work required in that area as well.
> > 
> >> So maybe there is no pressure to change anything for 6.1, and we can work on the right solution on master?
> >>
> >> Not sure how we consider this feature for 6.1, I guess it is still not a supported option,
> >> (is there any CI for this? Probably not right?),
> >>
> >> so I would consider building modular tcg in 6.1 as "experimental", and we can proceed to do the right thing on master?
> > 
> > For OpenSUSE Tumbleweed, when we release QEMU 6.1, I can add my patch to
> > "--enable-tcg-builtin" for downstream only. I'm fine with it too.
> 
> Hi Jose,
> 
> indeed if we need to do something downstream it's fine,
> but lets keep the discussion on this list focused on what is best for upstream.

Absolutely! I'm sorry. I just wanted to answer that --enable-modules is
already used. Didn't mean cause any disturbance, sorry.

Thank you!!

> 
> Ciao, thanks :-)
> 
> Claudio
> 
> > 
> > Thank you!!!
> > 
> >>
> >> Thanks,
> >>
> >> Claudio
> >>
> >>>
> >>>>
> >>>>>      if (ops->ops_init) {
> >>>>>          ops->ops_init(ops);
> >>>>>      }
> >>>>> diff --git a/util/module.c b/util/module.c
> >>>>> index 6bb4ad915a..268a8563fd 100644
> >>>>> --- a/util/module.c
> >>>>> +++ b/util/module.c
> >>>>> @@ -206,13 +206,10 @@ static int module_load_file(const char *fname, bool mayfail, bool export_symbols
> >>>>>  out:
> >>>>>      return ret;
> >>>>>  }
> >>>>> -#endif
> >>>>>  
> >>>>>  bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
> >>>>>  {
> >>>>>      bool success = false;
> >>>>> -
> >>>>> -#ifdef CONFIG_MODULES
> >>>>>      char *fname = NULL;
> >>>>>  #ifdef CONFIG_MODULE_UPGRADES
> >>>>>      char *version_dir;
> >>>>> @@ -300,6 +297,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
> >>>>>  
> >>>>>      if (!success) {
> >>>>>          g_hash_table_remove(loaded_modules, module_name);
> >>>>> +        fprintf(stderr, "%s module is missing, install the "
> >>>>> +                        "package or config the library path "
> >>>>> +                        "correctly.\n", module_name);
> >>>>>          g_free(module_name);
> >>>>>      }
> >>>>>  
> >>>>> @@ -307,12 +307,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
> >>>>>          g_free(dirs[i]);
> >>>>>      }
> >>>>>  
> >>>>> -#endif
> >>>>>      return success;
> >>>>>  }
> >>>>>  
> >>>>> -#ifdef CONFIG_MODULES
> >>>>> -
> >>>>>  static bool module_loaded_qom_all;
> >>>>>  
> >>>>>  void module_load_qom_one(const char *type)
> >>>>> @@ -384,4 +381,9 @@ void qemu_load_module_for_opts(const char *group) {}
> >>>>>  void module_load_qom_one(const char *type) {}
> >>>>>  void module_load_qom_all(void) {}
> >>>>>  
> >>>>> +bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
> >>>>> +{
> >>>>> +    return false;
> >>>>> +}
> >>>>> +
> >>>>>  #endif
> >>>>>
> >>>>
> >>
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

modular tcg (was: Re: [PATCH v2 1/1] modules: Improve error message when module is not) found

[Gerd Hoffmann]

  Hi,

> > So we need more work to make this actually work right.
> 
> Yes.  I want have all of tcg in the tcg accel module, not only parts of
> it, but that needs some more refactoring.  I'll go start looking at this
> once I managed to wade through my vacation backlog.

So, changed the meson.build files to build tcg modular
(https://git.kraxel.org/cgit/qemu/log/?h=sirius/modules-tcg-meson),
then checked what unresolved symbols we have:

# -*- symbols -*-
     18 tlb_flush
      8 tb_flush
      6 update_fp_status
      6 cpu_restore_state
      4 x86_register_ferr_irq
      4 update_mxcsr_status
      4 update_mxcsr_from_sse_status
      4 tlb_flush_page
      4 curr_cflags
      3 cpu_cc_compute_all
      2 tlb_reset_dirty
      2 tlb_plugin_lookup
      2 tcg_flush_softmmu_tlb
      2 tcg_exec_unrealizefn
      2 tcg_exec_realizefn
      2 tb_invalidate_phys_range
      2 tb_invalidate_phys_page_range
      2 tb_check_watchpoint
      2 cpu_x86_update_dr7
      2 cpu_set_ignne
# -*- files -*-
     22 softmmu/physmem.c        tlb_flush tlb_flush_page curr_cflags tlb_reset_dirty tb_invalidate_phys_range tb_check_watchpoint cpu_restore_state
     10 target/i386/helper.c     tlb_flush cpu_restore_state
     15 cpu.c                    cpu_cc_compute_all tlb_flush tcg_flush_softmmu_tlb tcg_exec_unrealizefn tcg_exec_realizefn tb_invalidate_phys_page_range tb_flush
      9 target/i386/cpu.h        update_fp_status cpu_cc_compute_all update_mxcsr_status
      8 target/i386/machine.c    update_mxcsr_status update_fp_status tlb_flush cpu_x86_update_dr7
      2 target/i386/gdbstub.c    update_mxcsr_from_sse_status
      2 target/i386/cpu-dump.c   update_mxcsr_from_sse_status
      2 plugins/loader.c         tb_flush
      2 plugins/core.c           tb_flush
      2 plugins/api.c            tlb_plugin_lookup
      2 i386/pc_q35.c            x86_register_ferr_irq
      2 i386/pc_piix.c           x86_register_ferr_irq
      2 i386/pc.c                cpu_set_ignne
      4 gdbstub.c                update_mxcsr_from_sse_status tb_flush
      2 core/cpu-common.c        tcg_flush_softmmu_tlb
      2 accel/tcg/cpu-exec-common.c cpu_restore_state

It's basically two groups:

 * Arch-specific (functions taking CPUX86State as argument), most of the
   unresolved symbols in target/i386/ and i386/ directories go into this
   category.
 * Common (functions taking CPUState as argument).  Everything else.

The common functions could be added TCGCPUOps to allow them being called via
CPUClass->tcg_ops->$name instead of a direct symbol reference.  Not sure this
is a good idea though.  Experimental patch covering tcg_exec_realizefn +
tcg_exec_unrealizefn below.

No idea yet how to handle arch-specific bits best.  Seems there is no existing
infrastructure to untangle target-specific code and tcg, so this probably needs
something new.

Noticed softmmu/physmem.c has lots of CONFIG_TCG #ifdefs, splitting this into
softmmu/physmem-{common,tcg}.c is probably a good idea.

Comments?

take care,
  Gerd

From c0e3f4cbe6aa6889d344beb4cac300b160253f00 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Thu, 29 Jul 2021 09:14:56 +0200
Subject: [PATCH] tcg: add tcg_exec_{realizefn,unrealizefn} to TCGCPUOps

---
 include/hw/core/tcg-cpu-ops.h |  7 +++++++
 cpu.c                         | 23 +++++++++++------------
 target/alpha/cpu.c            |  1 +
 target/arm/cpu.c              |  1 +
 target/arm/cpu_tcg.c          |  1 +
 target/avr/cpu.c              |  1 +
 target/cris/cpu.c             |  1 +
 target/hexagon/cpu.c          |  1 +
 target/hppa/cpu.c             |  1 +
 target/i386/tcg/tcg-cpu.c     |  1 +
 target/m68k/cpu.c             |  1 +
 target/microblaze/cpu.c       |  1 +
 target/mips/cpu.c             |  1 +
 target/nios2/cpu.c            |  1 +
 target/openrisc/cpu.c         |  1 +
 target/ppc/cpu_init.c         |  1 +
 target/riscv/cpu.c            |  1 +
 target/rx/cpu.c               |  1 +
 target/s390x/cpu.c            |  1 +
 target/sh4/cpu.c              |  1 +
 target/sparc/cpu.c            |  1 +
 target/tricore/cpu.c          |  1 +
 target/xtensa/cpu.c           |  1 +
 23 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
index eab27d0c0309..1f33953a4efc 100644
--- a/include/hw/core/tcg-cpu-ops.h
+++ b/include/hw/core/tcg-cpu-ops.h
@@ -107,6 +107,13 @@ struct TCGCPUOps {
 #endif /* CONFIG_SOFTMMU */
 #endif /* NEED_CPU_H */
 
+    void (*exec_realizefn)(CPUState *cpu, Error **errp);
+    void (*exec_unrealizefn)(CPUState *cpu);
 };
 
+#define TCG_CPU_OPS_COMMON \
+    .exec_realizefn = tcg_exec_realizefn, \
+    .exec_unrealizefn = tcg_exec_unrealizefn
+
+
 #endif /* TCG_CPU_OPS_H */
diff --git a/cpu.c b/cpu.c
index e1799a15bcf5..982463ce7418 100644
--- a/cpu.c
+++ b/cpu.c
@@ -40,6 +40,10 @@
 #include "hw/core/accel-cpu.h"
 #include "trace/trace-root.h"
 
+#ifdef CONFIG_TCG
+#include "hw/core/tcg-cpu-ops.h"
+#endif /* CONFIG_TCG */
+
 uintptr_t qemu_host_page_size;
 intptr_t qemu_host_page_mask;
 
@@ -129,20 +133,17 @@ const VMStateDescription vmstate_cpu_common = {
 
 void cpu_exec_realizefn(CPUState *cpu, Error **errp)
 {
-#ifndef CONFIG_USER_ONLY
     CPUClass *cc = CPU_GET_CLASS(cpu);
-#endif
 
     cpu_list_add(cpu);
     if (!accel_cpu_realizefn(cpu, errp)) {
         return;
     }
-#ifdef CONFIG_TCG
+
     /* NB: errp parameter is unused currently */
-    if (tcg_enabled()) {
-        tcg_exec_realizefn(cpu, errp);
+    if (cc->tcg_ops) {
+        cc->tcg_ops->exec_realizefn(cpu, errp);
     }
-#endif /* CONFIG_TCG */
 
 #ifdef CONFIG_USER_ONLY
     assert(qdev_get_vmsd(DEVICE(cpu)) == NULL ||
@@ -159,9 +160,9 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
 
 void cpu_exec_unrealizefn(CPUState *cpu)
 {
-#ifndef CONFIG_USER_ONLY
     CPUClass *cc = CPU_GET_CLASS(cpu);
 
+#ifndef CONFIG_USER_ONLY
     if (cc->sysemu_ops->legacy_vmsd != NULL) {
         vmstate_unregister(NULL, cc->sysemu_ops->legacy_vmsd, cpu);
     }
@@ -169,12 +170,10 @@ void cpu_exec_unrealizefn(CPUState *cpu)
         vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
     }
 #endif
-#ifdef CONFIG_TCG
-    /* NB: errp parameter is unused currently */
-    if (tcg_enabled()) {
-        tcg_exec_unrealizefn(cpu);
+
+    if (cc->tcg_ops) {
+        cc->tcg_ops->exec_unrealizefn(cpu);
     }
-#endif /* CONFIG_TCG */
 
     cpu_list_remove(cpu);
 }
diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index 4871ad0c0a6a..715fb97d6707 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -226,6 +226,7 @@ static const struct TCGCPUOps alpha_tcg_ops = {
     .do_transaction_failed = alpha_cpu_do_transaction_failed,
     .do_unaligned_access = alpha_cpu_do_unaligned_access,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 
 static void alpha_cpu_class_init(ObjectClass *oc, void *data)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 2866dd765882..7f051bd24e0b 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1996,6 +1996,7 @@ static const struct TCGCPUOps arm_tcg_ops = {
     .debug_check_watchpoint = arm_debug_check_watchpoint,
     .debug_check_breakpoint = arm_debug_check_breakpoint,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 #endif /* CONFIG_TCG */
 
diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
index ed444bf436a4..0f8657ede860 100644
--- a/target/arm/cpu_tcg.c
+++ b/target/arm/cpu_tcg.c
@@ -913,6 +913,7 @@ static const struct TCGCPUOps arm_v7m_tcg_ops = {
     .debug_check_watchpoint = arm_debug_check_watchpoint,
     .debug_check_breakpoint = arm_debug_check_breakpoint,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 #endif /* CONFIG_TCG */
 
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index ea14175ca557..8b3261ef832c 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -201,6 +201,7 @@ static const struct TCGCPUOps avr_tcg_ops = {
 #ifndef CONFIG_USER_ONLY
     .do_interrupt = avr_cpu_do_interrupt,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 
 static void avr_cpu_class_init(ObjectClass *oc, void *data)
diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index 70932b1f8c7d..32ca1051f050 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -221,6 +221,7 @@ static const struct TCGCPUOps crisv32_tcg_ops = {
 #ifndef CONFIG_USER_ONLY
     .do_interrupt = cris_cpu_do_interrupt,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 
 static void crisv8_cpu_class_init(ObjectClass *oc, void *data)
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index 3338365c16ec..86c001b67484 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -273,6 +273,7 @@ static const struct TCGCPUOps hexagon_tcg_ops = {
     .initialize = hexagon_translate_init,
     .synchronize_from_tb = hexagon_cpu_synchronize_from_tb,
     .tlb_fill = hexagon_tlb_fill,
+    TCG_CPU_OPS_COMMON,
 };
 
 static void hexagon_cpu_class_init(ObjectClass *c, void *data)
diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index 2eace4ee1240..5163118e986d 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -151,6 +151,7 @@ static const struct TCGCPUOps hppa_tcg_ops = {
     .do_interrupt = hppa_cpu_do_interrupt,
     .do_unaligned_access = hppa_cpu_do_unaligned_access,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 
 static void hppa_cpu_class_init(ObjectClass *oc, void *data)
diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
index 93a79a574154..bf7d91fed6da 100644
--- a/target/i386/tcg/tcg-cpu.c
+++ b/target/i386/tcg/tcg-cpu.c
@@ -79,6 +79,7 @@ static const struct TCGCPUOps x86_tcg_ops = {
     .debug_excp_handler = breakpoint_handler,
     .debug_check_breakpoint = x86_debug_check_breakpoint,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 
 static void tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 72de6e972627..df06c07907d6 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -522,6 +522,7 @@ static const struct TCGCPUOps m68k_tcg_ops = {
     .do_interrupt = m68k_cpu_do_interrupt,
     .do_transaction_failed = m68k_cpu_transaction_failed,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 
 static void m68k_cpu_class_init(ObjectClass *c, void *data)
diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index 72d8f2a0daa6..0e781264ef7b 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -373,6 +373,7 @@ static const struct TCGCPUOps mb_tcg_ops = {
     .do_transaction_failed = mb_cpu_transaction_failed,
     .do_unaligned_access = mb_cpu_do_unaligned_access,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 
 static void mb_cpu_class_init(ObjectClass *oc, void *data)
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index d426918291a9..3216a33d0047 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -548,6 +548,7 @@ static const struct TCGCPUOps mips_tcg_ops = {
     .do_unaligned_access = mips_cpu_do_unaligned_access,
     .io_recompile_replay_branch = mips_io_recompile_replay_branch,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 #endif /* CONFIG_TCG */
 
diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index 5e37defef805..55e21e8cd414 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -226,6 +226,7 @@ static const struct TCGCPUOps nios2_tcg_ops = {
     .do_interrupt = nios2_cpu_do_interrupt,
     .do_unaligned_access = nios2_cpu_do_unaligned_access,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 
 static void nios2_cpu_class_init(ObjectClass *oc, void *data)
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index bd34e429ecbf..0d9e1aa58a8f 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -192,6 +192,7 @@ static const struct TCGCPUOps openrisc_tcg_ops = {
 #ifndef CONFIG_USER_ONLY
     .do_interrupt = openrisc_cpu_do_interrupt,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 
 static void openrisc_cpu_class_init(ObjectClass *oc, void *data)
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 505a0ed6ac09..3f8bf2e3fcbf 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -9021,6 +9021,7 @@ static const struct TCGCPUOps ppc_tcg_ops = {
   .cpu_exec_exit = ppc_cpu_exec_exit,
   .do_unaligned_access = ppc_cpu_do_unaligned_access,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 #endif /* CONFIG_TCG */
 
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 991a6bb7604f..6a129b4fe211 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -650,6 +650,7 @@ static const struct TCGCPUOps riscv_tcg_ops = {
     .do_transaction_failed = riscv_cpu_do_transaction_failed,
     .do_unaligned_access = riscv_cpu_do_unaligned_access,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 
 static void riscv_cpu_class_init(ObjectClass *c, void *data)
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index 96cc96e514fe..7a1ebc694f22 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -192,6 +192,7 @@ static const struct TCGCPUOps rx_tcg_ops = {
 #ifndef CONFIG_USER_ONLY
     .do_interrupt = rx_cpu_do_interrupt,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 
 static void rx_cpu_class_init(ObjectClass *klass, void *data)
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 7b7b05f1d3a0..8dfcde1590c0 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -274,6 +274,7 @@ static const struct TCGCPUOps s390_tcg_ops = {
     .debug_excp_handler = s390x_cpu_debug_excp_handler,
     .do_unaligned_access = s390x_cpu_do_unaligned_access,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 #endif /* CONFIG_TCG */
 
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index 832692294211..0b4e03913656 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -244,6 +244,7 @@ static const struct TCGCPUOps superh_tcg_ops = {
     .do_unaligned_access = superh_cpu_do_unaligned_access,
     .io_recompile_replay_branch = superh_io_recompile_replay_branch,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 
 static void superh_cpu_class_init(ObjectClass *oc, void *data)
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index da6b30ec747a..2b2d2946223b 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -871,6 +871,7 @@ static const struct TCGCPUOps sparc_tcg_ops = {
     .do_transaction_failed = sparc_cpu_do_transaction_failed,
     .do_unaligned_access = sparc_cpu_do_unaligned_access,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 #endif /* CONFIG_TCG */
 
diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index b95682b7f04d..daeaecca26e0 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -154,6 +154,7 @@ static const struct TCGCPUOps tricore_tcg_ops = {
     .initialize = tricore_tcg_init,
     .synchronize_from_tb = tricore_cpu_synchronize_from_tb,
     .tlb_fill = tricore_cpu_tlb_fill,
+    TCG_CPU_OPS_COMMON,
 };
 
 static void tricore_cpu_class_init(ObjectClass *c, void *data)
diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index 58ec3a086224..d7d1b967040b 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -201,6 +201,7 @@ static const struct TCGCPUOps xtensa_tcg_ops = {
     .do_transaction_failed = xtensa_cpu_do_transaction_failed,
     .do_unaligned_access = xtensa_cpu_do_unaligned_access,
 #endif /* !CONFIG_USER_ONLY */
+    TCG_CPU_OPS_COMMON,
 };
 
 static void xtensa_cpu_class_init(ObjectClass *oc, void *data)
-- 
2.31.1

Re: modular tcg

[Claudio Fontana]

On 7/29/21 11:14 AM, Gerd Hoffmann wrote:
>   Hi,
> 
>>> So we need more work to make this actually work right.
>>
>> Yes.  I want have all of tcg in the tcg accel module, not only parts of
>> it, but that needs some more refactoring.  I'll go start looking at this
>> once I managed to wade through my vacation backlog.
> 
> So, changed the meson.build files to build tcg modular
> (https://git.kraxel.org/cgit/qemu/log/?h=sirius/modules-tcg-meson),
> then checked what unresolved symbols we have:
> 
> # -*- symbols -*-
>      18 tlb_flush
>       8 tb_flush
>       6 update_fp_status
>       6 cpu_restore_state
>       4 x86_register_ferr_irq
>       4 update_mxcsr_status
>       4 update_mxcsr_from_sse_status
>       4 tlb_flush_page
>       4 curr_cflags
>       3 cpu_cc_compute_all
>       2 tlb_reset_dirty
>       2 tlb_plugin_lookup
>       2 tcg_flush_softmmu_tlb
>       2 tcg_exec_unrealizefn
>       2 tcg_exec_realizefn
>       2 tb_invalidate_phys_range
>       2 tb_invalidate_phys_page_range
>       2 tb_check_watchpoint
>       2 cpu_x86_update_dr7
>       2 cpu_set_ignne
> # -*- files -*-
>      22 softmmu/physmem.c        tlb_flush tlb_flush_page curr_cflags tlb_reset_dirty tb_invalidate_phys_range tb_check_watchpoint cpu_restore_state
>      10 target/i386/helper.c     tlb_flush cpu_restore_state
>      15 cpu.c                    cpu_cc_compute_all tlb_flush tcg_flush_softmmu_tlb tcg_exec_unrealizefn tcg_exec_realizefn tb_invalidate_phys_page_range tb_flush
>       9 target/i386/cpu.h        update_fp_status cpu_cc_compute_all update_mxcsr_status
>       8 target/i386/machine.c    update_mxcsr_status update_fp_status tlb_flush cpu_x86_update_dr7
>       2 target/i386/gdbstub.c    update_mxcsr_from_sse_status
>       2 target/i386/cpu-dump.c   update_mxcsr_from_sse_status
>       2 plugins/loader.c         tb_flush
>       2 plugins/core.c           tb_flush
>       2 plugins/api.c            tlb_plugin_lookup
>       2 i386/pc_q35.c            x86_register_ferr_irq
>       2 i386/pc_piix.c           x86_register_ferr_irq
>       2 i386/pc.c                cpu_set_ignne
>       4 gdbstub.c                update_mxcsr_from_sse_status tb_flush
>       2 core/cpu-common.c        tcg_flush_softmmu_tlb
>       2 accel/tcg/cpu-exec-common.c cpu_restore_state
> 
> It's basically two groups:
> 
>  * Arch-specific (functions taking CPUX86State as argument), most of the
>    unresolved symbols in target/i386/ and i386/ directories go into this
>    category.

Yes, and we need to think about all targets, not just i386.

>  * Common (functions taking CPUState as argument).  Everything else.
> 
> The common functions could be added TCGCPUOps to allow them being called via

TCGCCPUOps are target-specific in their implementation, so I guess it's the arch specific part that could be TCGCPUOps (maybe, would need deep thinking).

> CPUClass->tcg_ops->$name instead of a direct symbol reference.  Not sure this
> is a good idea though.  Experimental patch covering tcg_exec_realizefn +
> tcg_exec_unrealizefn below.
> 
> No idea yet how to handle arch-specific bits best.  Seems there is no existing
> infrastructure to untangle target-specific code and tcg, so this probably needs
> something new.

We need target-specific modules. They could at the beginning absorb also the non-target specific parts in my view.
So you have a big tcg-arm module, a tcg-i386 module etc.

I think I sketched already the idea in the Makefile I shared before?


> 
> Noticed softmmu/physmem.c has lots of CONFIG_TCG #ifdefs, splitting this into
> softmmu/physmem-{common,tcg}.c is probably a good idea.
> 
> Comments?
> 
> take care,
>   Gerd
> 
> From c0e3f4cbe6aa6889d344beb4cac300b160253f00 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Thu, 29 Jul 2021 09:14:56 +0200
> Subject: [PATCH] tcg: add tcg_exec_{realizefn,unrealizefn} to TCGCPUOps
> 
> ---
>  include/hw/core/tcg-cpu-ops.h |  7 +++++++
>  cpu.c                         | 23 +++++++++++------------
>  target/alpha/cpu.c            |  1 +
>  target/arm/cpu.c              |  1 +
>  target/arm/cpu_tcg.c          |  1 +
>  target/avr/cpu.c              |  1 +
>  target/cris/cpu.c             |  1 +
>  target/hexagon/cpu.c          |  1 +
>  target/hppa/cpu.c             |  1 +
>  target/i386/tcg/tcg-cpu.c     |  1 +
>  target/m68k/cpu.c             |  1 +
>  target/microblaze/cpu.c       |  1 +
>  target/mips/cpu.c             |  1 +
>  target/nios2/cpu.c            |  1 +
>  target/openrisc/cpu.c         |  1 +
>  target/ppc/cpu_init.c         |  1 +
>  target/riscv/cpu.c            |  1 +
>  target/rx/cpu.c               |  1 +
>  target/s390x/cpu.c            |  1 +
>  target/sh4/cpu.c              |  1 +
>  target/sparc/cpu.c            |  1 +
>  target/tricore/cpu.c          |  1 +
>  target/xtensa/cpu.c           |  1 +
>  23 files changed, 39 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
> index eab27d0c0309..1f33953a4efc 100644
> --- a/include/hw/core/tcg-cpu-ops.h
> +++ b/include/hw/core/tcg-cpu-ops.h
> @@ -107,6 +107,13 @@ struct TCGCPUOps {
>  #endif /* CONFIG_SOFTMMU */
>  #endif /* NEED_CPU_H */
>  
> +    void (*exec_realizefn)(CPUState *cpu, Error **errp);
> +    void (*exec_unrealizefn)(CPUState *cpu);
>  };
>  
> +#define TCG_CPU_OPS_COMMON \
> +    .exec_realizefn = tcg_exec_realizefn, \
> +    .exec_unrealizefn = tcg_exec_unrealizefn
> +
> +
>  #endif /* TCG_CPU_OPS_H */
> diff --git a/cpu.c b/cpu.c
> index e1799a15bcf5..982463ce7418 100644
> --- a/cpu.c
> +++ b/cpu.c
> @@ -40,6 +40,10 @@
>  #include "hw/core/accel-cpu.h"
>  #include "trace/trace-root.h"
>  
> +#ifdef CONFIG_TCG
> +#include "hw/core/tcg-cpu-ops.h"
> +#endif /* CONFIG_TCG */
> +
>  uintptr_t qemu_host_page_size;
>  intptr_t qemu_host_page_mask;
>  
> @@ -129,20 +133,17 @@ const VMStateDescription vmstate_cpu_common = {
>  
>  void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>  {
> -#ifndef CONFIG_USER_ONLY
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> -#endif
>  
>      cpu_list_add(cpu);
>      if (!accel_cpu_realizefn(cpu, errp)) {
>          return;
>      }
> -#ifdef CONFIG_TCG
> +
>      /* NB: errp parameter is unused currently */
> -    if (tcg_enabled()) {
> -        tcg_exec_realizefn(cpu, errp);
> +    if (cc->tcg_ops) {
> +        cc->tcg_ops->exec_realizefn(cpu, errp);
>      }
> -#endif /* CONFIG_TCG */
>  
>  #ifdef CONFIG_USER_ONLY
>      assert(qdev_get_vmsd(DEVICE(cpu)) == NULL ||
> @@ -159,9 +160,9 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>  
>  void cpu_exec_unrealizefn(CPUState *cpu)
>  {
> -#ifndef CONFIG_USER_ONLY
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>  
> +#ifndef CONFIG_USER_ONLY
>      if (cc->sysemu_ops->legacy_vmsd != NULL) {
>          vmstate_unregister(NULL, cc->sysemu_ops->legacy_vmsd, cpu);
>      }
> @@ -169,12 +170,10 @@ void cpu_exec_unrealizefn(CPUState *cpu)
>          vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
>      }
>  #endif
> -#ifdef CONFIG_TCG
> -    /* NB: errp parameter is unused currently */
> -    if (tcg_enabled()) {
> -        tcg_exec_unrealizefn(cpu);
> +
> +    if (cc->tcg_ops) {
> +        cc->tcg_ops->exec_unrealizefn(cpu);
>      }
> -#endif /* CONFIG_TCG */
>  
>      cpu_list_remove(cpu);
>  }
> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
> index 4871ad0c0a6a..715fb97d6707 100644
> --- a/target/alpha/cpu.c
> +++ b/target/alpha/cpu.c
> @@ -226,6 +226,7 @@ static const struct TCGCPUOps alpha_tcg_ops = {
>      .do_transaction_failed = alpha_cpu_do_transaction_failed,
>      .do_unaligned_access = alpha_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void alpha_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 2866dd765882..7f051bd24e0b 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1996,6 +1996,7 @@ static const struct TCGCPUOps arm_tcg_ops = {
>      .debug_check_watchpoint = arm_debug_check_watchpoint,
>      .debug_check_breakpoint = arm_debug_check_breakpoint,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
> index ed444bf436a4..0f8657ede860 100644
> --- a/target/arm/cpu_tcg.c
> +++ b/target/arm/cpu_tcg.c
> @@ -913,6 +913,7 @@ static const struct TCGCPUOps arm_v7m_tcg_ops = {
>      .debug_check_watchpoint = arm_debug_check_watchpoint,
>      .debug_check_breakpoint = arm_debug_check_breakpoint,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index ea14175ca557..8b3261ef832c 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -201,6 +201,7 @@ static const struct TCGCPUOps avr_tcg_ops = {
>  #ifndef CONFIG_USER_ONLY
>      .do_interrupt = avr_cpu_do_interrupt,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void avr_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/cris/cpu.c b/target/cris/cpu.c
> index 70932b1f8c7d..32ca1051f050 100644
> --- a/target/cris/cpu.c
> +++ b/target/cris/cpu.c
> @@ -221,6 +221,7 @@ static const struct TCGCPUOps crisv32_tcg_ops = {
>  #ifndef CONFIG_USER_ONLY
>      .do_interrupt = cris_cpu_do_interrupt,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void crisv8_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
> index 3338365c16ec..86c001b67484 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -273,6 +273,7 @@ static const struct TCGCPUOps hexagon_tcg_ops = {
>      .initialize = hexagon_translate_init,
>      .synchronize_from_tb = hexagon_cpu_synchronize_from_tb,
>      .tlb_fill = hexagon_tlb_fill,
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void hexagon_cpu_class_init(ObjectClass *c, void *data)
> diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
> index 2eace4ee1240..5163118e986d 100644
> --- a/target/hppa/cpu.c
> +++ b/target/hppa/cpu.c
> @@ -151,6 +151,7 @@ static const struct TCGCPUOps hppa_tcg_ops = {
>      .do_interrupt = hppa_cpu_do_interrupt,
>      .do_unaligned_access = hppa_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void hppa_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
> index 93a79a574154..bf7d91fed6da 100644
> --- a/target/i386/tcg/tcg-cpu.c
> +++ b/target/i386/tcg/tcg-cpu.c
> @@ -79,6 +79,7 @@ static const struct TCGCPUOps x86_tcg_ops = {
>      .debug_excp_handler = breakpoint_handler,
>      .debug_check_breakpoint = x86_debug_check_breakpoint,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 72de6e972627..df06c07907d6 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -522,6 +522,7 @@ static const struct TCGCPUOps m68k_tcg_ops = {
>      .do_interrupt = m68k_cpu_do_interrupt,
>      .do_transaction_failed = m68k_cpu_transaction_failed,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void m68k_cpu_class_init(ObjectClass *c, void *data)
> diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
> index 72d8f2a0daa6..0e781264ef7b 100644
> --- a/target/microblaze/cpu.c
> +++ b/target/microblaze/cpu.c
> @@ -373,6 +373,7 @@ static const struct TCGCPUOps mb_tcg_ops = {
>      .do_transaction_failed = mb_cpu_transaction_failed,
>      .do_unaligned_access = mb_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void mb_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index d426918291a9..3216a33d0047 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -548,6 +548,7 @@ static const struct TCGCPUOps mips_tcg_ops = {
>      .do_unaligned_access = mips_cpu_do_unaligned_access,
>      .io_recompile_replay_branch = mips_io_recompile_replay_branch,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
> index 5e37defef805..55e21e8cd414 100644
> --- a/target/nios2/cpu.c
> +++ b/target/nios2/cpu.c
> @@ -226,6 +226,7 @@ static const struct TCGCPUOps nios2_tcg_ops = {
>      .do_interrupt = nios2_cpu_do_interrupt,
>      .do_unaligned_access = nios2_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void nios2_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
> index bd34e429ecbf..0d9e1aa58a8f 100644
> --- a/target/openrisc/cpu.c
> +++ b/target/openrisc/cpu.c
> @@ -192,6 +192,7 @@ static const struct TCGCPUOps openrisc_tcg_ops = {
>  #ifndef CONFIG_USER_ONLY
>      .do_interrupt = openrisc_cpu_do_interrupt,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void openrisc_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 505a0ed6ac09..3f8bf2e3fcbf 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -9021,6 +9021,7 @@ static const struct TCGCPUOps ppc_tcg_ops = {
>    .cpu_exec_exit = ppc_cpu_exec_exit,
>    .do_unaligned_access = ppc_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 991a6bb7604f..6a129b4fe211 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -650,6 +650,7 @@ static const struct TCGCPUOps riscv_tcg_ops = {
>      .do_transaction_failed = riscv_cpu_do_transaction_failed,
>      .do_unaligned_access = riscv_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void riscv_cpu_class_init(ObjectClass *c, void *data)
> diff --git a/target/rx/cpu.c b/target/rx/cpu.c
> index 96cc96e514fe..7a1ebc694f22 100644
> --- a/target/rx/cpu.c
> +++ b/target/rx/cpu.c
> @@ -192,6 +192,7 @@ static const struct TCGCPUOps rx_tcg_ops = {
>  #ifndef CONFIG_USER_ONLY
>      .do_interrupt = rx_cpu_do_interrupt,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void rx_cpu_class_init(ObjectClass *klass, void *data)
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 7b7b05f1d3a0..8dfcde1590c0 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -274,6 +274,7 @@ static const struct TCGCPUOps s390_tcg_ops = {
>      .debug_excp_handler = s390x_cpu_debug_excp_handler,
>      .do_unaligned_access = s390x_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
> index 832692294211..0b4e03913656 100644
> --- a/target/sh4/cpu.c
> +++ b/target/sh4/cpu.c
> @@ -244,6 +244,7 @@ static const struct TCGCPUOps superh_tcg_ops = {
>      .do_unaligned_access = superh_cpu_do_unaligned_access,
>      .io_recompile_replay_branch = superh_io_recompile_replay_branch,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void superh_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
> index da6b30ec747a..2b2d2946223b 100644
> --- a/target/sparc/cpu.c
> +++ b/target/sparc/cpu.c
> @@ -871,6 +871,7 @@ static const struct TCGCPUOps sparc_tcg_ops = {
>      .do_transaction_failed = sparc_cpu_do_transaction_failed,
>      .do_unaligned_access = sparc_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
> index b95682b7f04d..daeaecca26e0 100644
> --- a/target/tricore/cpu.c
> +++ b/target/tricore/cpu.c
> @@ -154,6 +154,7 @@ static const struct TCGCPUOps tricore_tcg_ops = {
>      .initialize = tricore_tcg_init,
>      .synchronize_from_tb = tricore_cpu_synchronize_from_tb,
>      .tlb_fill = tricore_cpu_tlb_fill,
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void tricore_cpu_class_init(ObjectClass *c, void *data)
> diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
> index 58ec3a086224..d7d1b967040b 100644
> --- a/target/xtensa/cpu.c
> +++ b/target/xtensa/cpu.c
> @@ -201,6 +201,7 @@ static const struct TCGCPUOps xtensa_tcg_ops = {
>      .do_transaction_failed = xtensa_cpu_do_transaction_failed,
>      .do_unaligned_access = xtensa_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void xtensa_cpu_class_init(ObjectClass *oc, void *data)
> 

Re: modular tcg

[Claudio Fontana]

On 7/29/21 11:14 AM, Gerd Hoffmann wrote:
>   Hi,
> 
>>> So we need more work to make this actually work right.
>>
>> Yes.  I want have all of tcg in the tcg accel module, not only parts of
>> it, but that needs some more refactoring.  I'll go start looking at this
>> once I managed to wade through my vacation backlog.
> 
> So, changed the meson.build files to build tcg modular
> (https://git.kraxel.org/cgit/qemu/log/?h=sirius/modules-tcg-meson),
> then checked what unresolved symbols we have:
> 
> # -*- symbols -*-
>      18 tlb_flush
>       8 tb_flush
>       6 update_fp_status
>       6 cpu_restore_state
>       4 x86_register_ferr_irq
>       4 update_mxcsr_status
>       4 update_mxcsr_from_sse_status
>       4 tlb_flush_page
>       4 curr_cflags
>       3 cpu_cc_compute_all
>       2 tlb_reset_dirty
>       2 tlb_plugin_lookup
>       2 tcg_flush_softmmu_tlb
>       2 tcg_exec_unrealizefn
>       2 tcg_exec_realizefn
>       2 tb_invalidate_phys_range
>       2 tb_invalidate_phys_page_range
>       2 tb_check_watchpoint
>       2 cpu_x86_update_dr7
>       2 cpu_set_ignne
> # -*- files -*-
>      22 softmmu/physmem.c        tlb_flush tlb_flush_page curr_cflags tlb_reset_dirty tb_invalidate_phys_range tb_check_watchpoint cpu_restore_state
>      10 target/i386/helper.c     tlb_flush cpu_restore_state
>      15 cpu.c                    cpu_cc_compute_all tlb_flush tcg_flush_softmmu_tlb tcg_exec_unrealizefn tcg_exec_realizefn tb_invalidate_phys_page_range tb_flush
>       9 target/i386/cpu.h        update_fp_status cpu_cc_compute_all update_mxcsr_status
>       8 target/i386/machine.c    update_mxcsr_status update_fp_status tlb_flush cpu_x86_update_dr7
>       2 target/i386/gdbstub.c    update_mxcsr_from_sse_status
>       2 target/i386/cpu-dump.c   update_mxcsr_from_sse_status
>       2 plugins/loader.c         tb_flush
>       2 plugins/core.c           tb_flush
>       2 plugins/api.c            tlb_plugin_lookup
>       2 i386/pc_q35.c            x86_register_ferr_irq
>       2 i386/pc_piix.c           x86_register_ferr_irq
>       2 i386/pc.c                cpu_set_ignne
>       4 gdbstub.c                update_mxcsr_from_sse_status tb_flush
>       2 core/cpu-common.c        tcg_flush_softmmu_tlb
>       2 accel/tcg/cpu-exec-common.c cpu_restore_state
> 
> It's basically two groups:
> 
>  * Arch-specific (functions taking CPUX86State as argument), most of the
>    unresolved symbols in target/i386/ and i386/ directories go into this
>    category.
>  * Common (functions taking CPUState as argument).  Everything else.
> 
> The common functions could be added TCGCPUOps to allow them being called via
> CPUClass->tcg_ops->$name instead of a direct symbol reference.  Not sure this
> is a good idea though.  Experimental patch covering tcg_exec_realizefn +
> tcg_exec_unrealizefn below.
> 
> No idea yet how to handle arch-specific bits best.  Seems there is no existing
> infrastructure to untangle target-specific code and tcg, so this probably needs
> something new.
> 
> Noticed softmmu/physmem.c has lots of CONFIG_TCG #ifdefs, splitting this into
> softmmu/physmem-{common,tcg}.c is probably a good idea.
> 
> Comments?
> 
> take care,
>   Gerd
> 
> From c0e3f4cbe6aa6889d344beb4cac300b160253f00 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Thu, 29 Jul 2021 09:14:56 +0200
> Subject: [PATCH] tcg: add tcg_exec_{realizefn,unrealizefn} to TCGCPUOps
> 
> ---
>  include/hw/core/tcg-cpu-ops.h |  7 +++++++
>  cpu.c                         | 23 +++++++++++------------
>  target/alpha/cpu.c            |  1 +
>  target/arm/cpu.c              |  1 +
>  target/arm/cpu_tcg.c          |  1 +
>  target/avr/cpu.c              |  1 +
>  target/cris/cpu.c             |  1 +
>  target/hexagon/cpu.c          |  1 +
>  target/hppa/cpu.c             |  1 +
>  target/i386/tcg/tcg-cpu.c     |  1 +
>  target/m68k/cpu.c             |  1 +
>  target/microblaze/cpu.c       |  1 +
>  target/mips/cpu.c             |  1 +
>  target/nios2/cpu.c            |  1 +
>  target/openrisc/cpu.c         |  1 +
>  target/ppc/cpu_init.c         |  1 +
>  target/riscv/cpu.c            |  1 +
>  target/rx/cpu.c               |  1 +
>  target/s390x/cpu.c            |  1 +
>  target/sh4/cpu.c              |  1 +
>  target/sparc/cpu.c            |  1 +
>  target/tricore/cpu.c          |  1 +
>  target/xtensa/cpu.c           |  1 +
>  23 files changed, 39 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
> index eab27d0c0309..1f33953a4efc 100644
> --- a/include/hw/core/tcg-cpu-ops.h
> +++ b/include/hw/core/tcg-cpu-ops.h
> @@ -107,6 +107,13 @@ struct TCGCPUOps {
>  #endif /* CONFIG_SOFTMMU */
>  #endif /* NEED_CPU_H */
>  
> +    void (*exec_realizefn)(CPUState *cpu, Error **errp);
> +    void (*exec_unrealizefn)(CPUState *cpu);
>  };
>  
> +#define TCG_CPU_OPS_COMMON \
> +    .exec_realizefn = tcg_exec_realizefn, \
> +    .exec_unrealizefn = tcg_exec_unrealizefn
> +
> +
>  #endif /* TCG_CPU_OPS_H */
> diff --git a/cpu.c b/cpu.c
> index e1799a15bcf5..982463ce7418 100644
> --- a/cpu.c
> +++ b/cpu.c
> @@ -40,6 +40,10 @@
>  #include "hw/core/accel-cpu.h"
>  #include "trace/trace-root.h"
>  
> +#ifdef CONFIG_TCG
> +#include "hw/core/tcg-cpu-ops.h"
> +#endif /* CONFIG_TCG */
> +
>  uintptr_t qemu_host_page_size;
>  intptr_t qemu_host_page_mask;
>  
> @@ -129,20 +133,17 @@ const VMStateDescription vmstate_cpu_common = {
>  
>  void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>  {
> -#ifndef CONFIG_USER_ONLY
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> -#endif
>  
>      cpu_list_add(cpu);
>      if (!accel_cpu_realizefn(cpu, errp)) {
>          return;
>      }
> -#ifdef CONFIG_TCG
> +
>      /* NB: errp parameter is unused currently */
> -    if (tcg_enabled()) {
> -        tcg_exec_realizefn(cpu, errp);
> +    if (cc->tcg_ops) {
> +        cc->tcg_ops->exec_realizefn(cpu, errp);
>      }
> -#endif /* CONFIG_TCG */
>  
>  #ifdef CONFIG_USER_ONLY
>      assert(qdev_get_vmsd(DEVICE(cpu)) == NULL ||
> @@ -159,9 +160,9 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>  
>  void cpu_exec_unrealizefn(CPUState *cpu)
>  {
> -#ifndef CONFIG_USER_ONLY
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>  
> +#ifndef CONFIG_USER_ONLY
>      if (cc->sysemu_ops->legacy_vmsd != NULL) {
>          vmstate_unregister(NULL, cc->sysemu_ops->legacy_vmsd, cpu);
>      }
> @@ -169,12 +170,10 @@ void cpu_exec_unrealizefn(CPUState *cpu)
>          vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
>      }
>  #endif
> -#ifdef CONFIG_TCG
> -    /* NB: errp parameter is unused currently */
> -    if (tcg_enabled()) {
> -        tcg_exec_unrealizefn(cpu);
> +
> +    if (cc->tcg_ops) {
> +        cc->tcg_ops->exec_unrealizefn(cpu);
>      }
> -#endif /* CONFIG_TCG */
>  
>      cpu_list_remove(cpu);
>  }
> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
> index 4871ad0c0a6a..715fb97d6707 100644
> --- a/target/alpha/cpu.c
> +++ b/target/alpha/cpu.c
> @@ -226,6 +226,7 @@ static const struct TCGCPUOps alpha_tcg_ops = {
>      .do_transaction_failed = alpha_cpu_do_transaction_failed,
>      .do_unaligned_access = alpha_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void alpha_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 2866dd765882..7f051bd24e0b 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1996,6 +1996,7 @@ static const struct TCGCPUOps arm_tcg_ops = {
>      .debug_check_watchpoint = arm_debug_check_watchpoint,
>      .debug_check_breakpoint = arm_debug_check_breakpoint,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
> index ed444bf436a4..0f8657ede860 100644
> --- a/target/arm/cpu_tcg.c
> +++ b/target/arm/cpu_tcg.c
> @@ -913,6 +913,7 @@ static const struct TCGCPUOps arm_v7m_tcg_ops = {
>      .debug_check_watchpoint = arm_debug_check_watchpoint,
>      .debug_check_breakpoint = arm_debug_check_breakpoint,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index ea14175ca557..8b3261ef832c 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -201,6 +201,7 @@ static const struct TCGCPUOps avr_tcg_ops = {
>  #ifndef CONFIG_USER_ONLY
>      .do_interrupt = avr_cpu_do_interrupt,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void avr_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/cris/cpu.c b/target/cris/cpu.c
> index 70932b1f8c7d..32ca1051f050 100644
> --- a/target/cris/cpu.c
> +++ b/target/cris/cpu.c
> @@ -221,6 +221,7 @@ static const struct TCGCPUOps crisv32_tcg_ops = {
>  #ifndef CONFIG_USER_ONLY
>      .do_interrupt = cris_cpu_do_interrupt,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void crisv8_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
> index 3338365c16ec..86c001b67484 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -273,6 +273,7 @@ static const struct TCGCPUOps hexagon_tcg_ops = {
>      .initialize = hexagon_translate_init,
>      .synchronize_from_tb = hexagon_cpu_synchronize_from_tb,
>      .tlb_fill = hexagon_tlb_fill,
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void hexagon_cpu_class_init(ObjectClass *c, void *data)
> diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
> index 2eace4ee1240..5163118e986d 100644
> --- a/target/hppa/cpu.c
> +++ b/target/hppa/cpu.c
> @@ -151,6 +151,7 @@ static const struct TCGCPUOps hppa_tcg_ops = {
>      .do_interrupt = hppa_cpu_do_interrupt,
>      .do_unaligned_access = hppa_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void hppa_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
> index 93a79a574154..bf7d91fed6da 100644
> --- a/target/i386/tcg/tcg-cpu.c
> +++ b/target/i386/tcg/tcg-cpu.c
> @@ -79,6 +79,7 @@ static const struct TCGCPUOps x86_tcg_ops = {
>      .debug_excp_handler = breakpoint_handler,
>      .debug_check_breakpoint = x86_debug_check_breakpoint,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 72de6e972627..df06c07907d6 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -522,6 +522,7 @@ static const struct TCGCPUOps m68k_tcg_ops = {
>      .do_interrupt = m68k_cpu_do_interrupt,
>      .do_transaction_failed = m68k_cpu_transaction_failed,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void m68k_cpu_class_init(ObjectClass *c, void *data)
> diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
> index 72d8f2a0daa6..0e781264ef7b 100644
> --- a/target/microblaze/cpu.c
> +++ b/target/microblaze/cpu.c
> @@ -373,6 +373,7 @@ static const struct TCGCPUOps mb_tcg_ops = {
>      .do_transaction_failed = mb_cpu_transaction_failed,
>      .do_unaligned_access = mb_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void mb_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index d426918291a9..3216a33d0047 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -548,6 +548,7 @@ static const struct TCGCPUOps mips_tcg_ops = {
>      .do_unaligned_access = mips_cpu_do_unaligned_access,
>      .io_recompile_replay_branch = mips_io_recompile_replay_branch,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
> index 5e37defef805..55e21e8cd414 100644
> --- a/target/nios2/cpu.c
> +++ b/target/nios2/cpu.c
> @@ -226,6 +226,7 @@ static const struct TCGCPUOps nios2_tcg_ops = {
>      .do_interrupt = nios2_cpu_do_interrupt,
>      .do_unaligned_access = nios2_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void nios2_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
> index bd34e429ecbf..0d9e1aa58a8f 100644
> --- a/target/openrisc/cpu.c
> +++ b/target/openrisc/cpu.c
> @@ -192,6 +192,7 @@ static const struct TCGCPUOps openrisc_tcg_ops = {
>  #ifndef CONFIG_USER_ONLY
>      .do_interrupt = openrisc_cpu_do_interrupt,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void openrisc_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 505a0ed6ac09..3f8bf2e3fcbf 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -9021,6 +9021,7 @@ static const struct TCGCPUOps ppc_tcg_ops = {
>    .cpu_exec_exit = ppc_cpu_exec_exit,
>    .do_unaligned_access = ppc_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 991a6bb7604f..6a129b4fe211 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -650,6 +650,7 @@ static const struct TCGCPUOps riscv_tcg_ops = {
>      .do_transaction_failed = riscv_cpu_do_transaction_failed,
>      .do_unaligned_access = riscv_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void riscv_cpu_class_init(ObjectClass *c, void *data)
> diff --git a/target/rx/cpu.c b/target/rx/cpu.c
> index 96cc96e514fe..7a1ebc694f22 100644
> --- a/target/rx/cpu.c
> +++ b/target/rx/cpu.c
> @@ -192,6 +192,7 @@ static const struct TCGCPUOps rx_tcg_ops = {
>  #ifndef CONFIG_USER_ONLY
>      .do_interrupt = rx_cpu_do_interrupt,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void rx_cpu_class_init(ObjectClass *klass, void *data)
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 7b7b05f1d3a0..8dfcde1590c0 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -274,6 +274,7 @@ static const struct TCGCPUOps s390_tcg_ops = {
>      .debug_excp_handler = s390x_cpu_debug_excp_handler,
>      .do_unaligned_access = s390x_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
> index 832692294211..0b4e03913656 100644
> --- a/target/sh4/cpu.c
> +++ b/target/sh4/cpu.c
> @@ -244,6 +244,7 @@ static const struct TCGCPUOps superh_tcg_ops = {
>      .do_unaligned_access = superh_cpu_do_unaligned_access,
>      .io_recompile_replay_branch = superh_io_recompile_replay_branch,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void superh_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
> index da6b30ec747a..2b2d2946223b 100644
> --- a/target/sparc/cpu.c
> +++ b/target/sparc/cpu.c
> @@ -871,6 +871,7 @@ static const struct TCGCPUOps sparc_tcg_ops = {
>      .do_transaction_failed = sparc_cpu_do_transaction_failed,
>      .do_unaligned_access = sparc_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
> index b95682b7f04d..daeaecca26e0 100644
> --- a/target/tricore/cpu.c
> +++ b/target/tricore/cpu.c
> @@ -154,6 +154,7 @@ static const struct TCGCPUOps tricore_tcg_ops = {
>      .initialize = tricore_tcg_init,
>      .synchronize_from_tb = tricore_cpu_synchronize_from_tb,
>      .tlb_fill = tricore_cpu_tlb_fill,
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void tricore_cpu_class_init(ObjectClass *c, void *data)
> diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
> index 58ec3a086224..d7d1b967040b 100644
> --- a/target/xtensa/cpu.c
> +++ b/target/xtensa/cpu.c
> @@ -201,6 +201,7 @@ static const struct TCGCPUOps xtensa_tcg_ops = {
>      .do_transaction_failed = xtensa_cpu_do_transaction_failed,
>      .do_unaligned_access = xtensa_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void xtensa_cpu_class_init(ObjectClass *oc, void *data)
> 

And another comment: I think we should have some progress on ARM with the kvm/tcg split and with the KConfig of boards,
before we continue here.

Ciao,

C

Re: modular tcg

[Gerd Hoffmann]

  Hi,

> > It's basically two groups:
> > 
> >  * Arch-specific (functions taking CPUX86State as argument), most of the
> >    unresolved symbols in target/i386/ and i386/ directories go into this
> >    category.
> 
> Yes, and we need to think about all targets, not just i386.

Sure.  I just want go forward in small steps, so my plan is to tackle
them one by one (starting with i386).

> >  * Common (functions taking CPUState as argument).  Everything else.
> > 
> > The common functions could be added TCGCPUOps to allow them being called via
> 
> TCGCCPUOps are target-specific in their implementation, so I guess
> it's the arch specific part that could be TCGCPUOps (maybe, would need
> deep thinking).

Ok, lets make it three groups then.

  (1) generic interface, arch implementation (this is what we have
      TCGCPUOps hooks right now).
  (2) generic interface, generic implementation (functions taking a
      CPUState as argument, simliar to group (1).
  (3) arch-specific interface and implementation (functions taking a
      CPUX86State argument).

We could add group (2) to TCGCPUOps for this ...

> > CPUClass->tcg_ops->$name instead of a direct symbol reference.  Not sure this
> > is a good idea though.  Experimental patch covering tcg_exec_realizefn +
> > tcg_exec_unrealizefn below.

... but as I sayed, not sure this is the best plan.

Adding group (3) to TCGCPUOps is a non-starter IMHO given that the
function prototypes are arch-specific (using CPUX86State) and also
the interfaces actually needed are arch-specific.  something like
x86_register_ferr_irq or cpu_x86_update_dr7 simply doesn't exist on
!x86.  I guess we'll need TCG${arch}Ops for those.

> > No idea yet how to handle arch-specific bits best.  Seems there is no existing
> > infrastructure to untangle target-specific code and tcg, so this probably needs
> > something new.
> 
> We need target-specific modules. They could at the beginning absorb
> also the non-target specific parts in my view.  So you have a big
> tcg-arm module, a tcg-i386 module etc.
> 
> I think I sketched already the idea in the Makefile I shared before?

We have target-specific modules in master branch.
Used for qtest (all archs) and tcg (i386/x86_64 only, accel ops only).

The build system changes to build more tcg bits modular are here:
https://git.kraxel.org/cgit/qemu/log/?h=sirius/modules-tcg-meson
Doesn't build due unresolved symbols, but shows which code
changes/cleanups/reorganizations are needed for (more) modular tcg.

take care,
  Gerd

Re: modular tcg

[Gerd Hoffmann]

  Hi,

> And another comment: I think we should have some progress on ARM with
> the kvm/tcg split and with the KConfig of boards, before we continue
> here.

Why?  This can easily be tacked in parallel.  We can flip the switch
for modular tcg per target in meson.build.

take care,
  Gerd

Re: modular tcg

[Claudio Fontana]

On 7/29/21 12:26 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>>> It's basically two groups:
>>>
>>>  * Arch-specific (functions taking CPUX86State as argument), most of the
>>>    unresolved symbols in target/i386/ and i386/ directories go into this
>>>    category.
>>
>> Yes, and we need to think about all targets, not just i386.
> 
> Sure.  I just want go forward in small steps, so my plan is to tackle
> them one by one (starting with i386).
> 
>>>  * Common (functions taking CPUState as argument).  Everything else.
>>>
>>> The common functions could be added TCGCPUOps to allow them being called via
>>
>> TCGCCPUOps are target-specific in their implementation, so I guess
>> it's the arch specific part that could be TCGCPUOps (maybe, would need
>> deep thinking).
> 
> Ok, lets make it three groups then.
> 
>   (1) generic interface, arch implementation (this is what we have
>       TCGCPUOps hooks right now).
>   (2) generic interface, generic implementation (functions taking a
>       CPUState as argument, simliar to group (1).
>   (3) arch-specific interface and implementation (functions taking a
>       CPUX86State argument).
> 
> We could add group (2) to TCGCPUOps for this ...
> 
>>> CPUClass->tcg_ops->$name instead of a direct symbol reference.  Not sure this
>>> is a good idea though.  Experimental patch covering tcg_exec_realizefn +
>>> tcg_exec_unrealizefn below.
> 
> ... but as I sayed, not sure this is the best plan.
> 
> Adding group (3) to TCGCPUOps is a non-starter IMHO given that the
> function prototypes are arch-specific (using CPUX86State) and also
> the interfaces actually needed are arch-specific.  something like
> x86_register_ferr_irq or cpu_x86_update_dr7 simply doesn't exist on
> !x86.  I guess we'll need TCG${arch}Ops for those.
> 
>>> No idea yet how to handle arch-specific bits best.  Seems there is no existing
>>> infrastructure to untangle target-specific code and tcg, so this probably needs
>>> something new.
>>
>> We need target-specific modules. They could at the beginning absorb
>> also the non-target specific parts in my view.  So you have a big
>> tcg-arm module, a tcg-i386 module etc.
>>
>> I think I sketched already the idea in the Makefile I shared before?
> 
> We have target-specific modules in master branch.
> Used for qtest (all archs) and tcg (i386/x86_64 only, accel ops only).
> 
> The build system changes to build more tcg bits modular are here:
> https://git.kraxel.org/cgit/qemu/log/?h=sirius/modules-tcg-meson
> Doesn't build due unresolved symbols, but shows which code
> changes/cleanups/reorganizations are needed for (more) modular tcg.
> 
> take care,
>   Gerd
> 

What I mean is, for starters, lets make all tcg code land in the target-specific module.

Sorry I am multitasking quite a bit so I may be missing something obvious.

Ciao,

Claudio

Re: modular tcg

[Claudio Fontana]

On 7/29/21 12:29 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>> And another comment: I think we should have some progress on ARM with
>> the kvm/tcg split and with the KConfig of boards, before we continue
>> here.
> 
> Why?  This can easily be tacked in parallel.  We can flip the switch
> for modular tcg per target in meson.build.
> 
> take care,
>   Gerd
> 

Because in the end we need to do this for ARM too and for the other archs too (s390 is already ok),

and in order to be sure not to end up in a dead-end, I think it would be good to have at least a sketch for the other archs as well..

Just my 2c ofc, I think really here still ARM is behind, and we should help it catch up.

If I had more time I would have pushed more on the ARM series, but.. yeah.

Ciao,

Claudio

Re: modular tcg

[Philippe Mathieu-Daudé]

On 7/29/21 12:44 PM, Claudio Fontana wrote:
> On 7/29/21 12:29 PM, Gerd Hoffmann wrote:
>>   Hi,
>>
>>> And another comment: I think we should have some progress on ARM with
>>> the kvm/tcg split and with the KConfig of boards, before we continue
>>> here.
>>
>> Why?  This can easily be tacked in parallel.  We can flip the switch
>> for modular tcg per target in meson.build.
>>
>> take care,
>>   Gerd
>>
> 
> Because in the end we need to do this for ARM too and for the other archs too (s390 is already ok),
> 
> and in order to be sure not to end up in a dead-end, I think it would be good to have at least a sketch for the other archs as well..
> 
> Just my 2c ofc, I think really here still ARM is behind, and we should help it catch up.
> 
> If I had more time I would have pushed more on the ARM series, but.. yeah.

IIUC Alex is waiting 6.2 release to respin.

Re: modular tcg

[Claudio Fontana]

On 7/29/21 1:34 PM, Philippe Mathieu-Daudé wrote:
> On 7/29/21 12:44 PM, Claudio Fontana wrote:
>> On 7/29/21 12:29 PM, Gerd Hoffmann wrote:
>>>   Hi,
>>>
>>>> And another comment: I think we should have some progress on ARM with
>>>> the kvm/tcg split and with the KConfig of boards, before we continue
>>>> here.
>>>
>>> Why?  This can easily be tacked in parallel.  We can flip the switch
>>> for modular tcg per target in meson.build.
>>>
>>> take care,
>>>   Gerd
>>>
>>
>> Because in the end we need to do this for ARM too and for the other archs too (s390 is already ok),
>>
>> and in order to be sure not to end up in a dead-end, I think it would be good to have at least a sketch for the other archs as well..
>>
>> Just my 2c ofc, I think really here still ARM is behind, and we should help it catch up.
>>
>> If I had more time I would have pushed more on the ARM series, but.. yeah.
> 
> IIUC Alex is waiting 6.2 release to respin.
> 

Ah that's great to know (and thanks again for picking it up).

CLaudio

Re: modular tcg

[Claudio Fontana]

On 7/29/21 1:34 PM, Philippe Mathieu-Daudé wrote:
> On 7/29/21 12:44 PM, Claudio Fontana wrote:
>> On 7/29/21 12:29 PM, Gerd Hoffmann wrote:
>>>   Hi,
>>>
>>>> And another comment: I think we should have some progress on ARM with
>>>> the kvm/tcg split and with the KConfig of boards, before we continue
>>>> here.
>>>
>>> Why?  This can easily be tacked in parallel.  We can flip the switch
>>> for modular tcg per target in meson.build.
>>>
>>> take care,
>>>   Gerd
>>>
>>
>> Because in the end we need to do this for ARM too and for the other archs too (s390 is already ok),
>>
>> and in order to be sure not to end up in a dead-end, I think it would be good to have at least a sketch for the other archs as well..
>>
>> Just my 2c ofc, I think really here still ARM is behind, and we should help it catch up.
>>
>> If I had more time I would have pushed more on the ARM series, but.. yeah.
> 
> IIUC Alex is waiting 6.2 release to respin.
> 

How does the Kconfig for ARM improvements go? I mean I think those improvements (enabling only compatible boards with the chosen accelerators) are important for both tcg-kvm split and possibly for modularization of ARM accelerators too right?

Ciao,

Claudio

Re: modular tcg

[Philippe Mathieu-Daudé]

On 7/29/21 4:22 PM, Claudio Fontana wrote:
> On 7/29/21 1:34 PM, Philippe Mathieu-Daudé wrote:
>> On 7/29/21 12:44 PM, Claudio Fontana wrote:
>>> On 7/29/21 12:29 PM, Gerd Hoffmann wrote:
>>>>   Hi,
>>>>
>>>>> And another comment: I think we should have some progress on ARM with
>>>>> the kvm/tcg split and with the KConfig of boards, before we continue
>>>>> here.
>>>>
>>>> Why?  This can easily be tacked in parallel.  We can flip the switch
>>>> for modular tcg per target in meson.build.
>>>>
>>>> take care,
>>>>   Gerd
>>>>
>>>
>>> Because in the end we need to do this for ARM too and for the other archs too (s390 is already ok),
>>>
>>> and in order to be sure not to end up in a dead-end, I think it would be good to have at least a sketch for the other archs as well..
>>>
>>> Just my 2c ofc, I think really here still ARM is behind, and we should help it catch up.
>>>
>>> If I had more time I would have pushed more on the ARM series, but.. yeah.
>>
>> IIUC Alex is waiting 6.2 release to respin.
>>
> 
> How does the Kconfig for ARM improvements go? I mean I think those improvements (enabling only compatible boards with the chosen accelerators) are important for both tcg-kvm split and possibly for modularization of ARM accelerators too right?

I think we all (Alex/you/me) reached the same point where builds work
but current the testing framework isn't ready for non-TCG or
modularized-TCG so the CI ends failing.

I don't want to push for 'CI build-only' because most of the annoying
problems were from runtime (interfaces not resolved, ... which are
important when using modules or board with unavailable devices).

I tried to address that with a QMP command to query accelerators but
there is a disagreement whether we should query for available/built-in/
loaded/modularized-but-not-installed/...). At this point I think I
fairly understand the technical problems but misunderstand the big
picture here, in particular w.r.t. management apps. I spent too many
time on this to appear enthusiastic, sorry.

Re: modular tcg

[Claudio Fontana]

On 7/29/21 4:59 PM, Philippe Mathieu-Daudé wrote:
> On 7/29/21 4:22 PM, Claudio Fontana wrote:
>> On 7/29/21 1:34 PM, Philippe Mathieu-Daudé wrote:
>>> On 7/29/21 12:44 PM, Claudio Fontana wrote:
>>>> On 7/29/21 12:29 PM, Gerd Hoffmann wrote:
>>>>>   Hi,
>>>>>
>>>>>> And another comment: I think we should have some progress on ARM with
>>>>>> the kvm/tcg split and with the KConfig of boards, before we continue
>>>>>> here.
>>>>>
>>>>> Why?  This can easily be tacked in parallel.  We can flip the switch
>>>>> for modular tcg per target in meson.build.
>>>>>
>>>>> take care,
>>>>>   Gerd
>>>>>
>>>>
>>>> Because in the end we need to do this for ARM too and for the other archs too (s390 is already ok),
>>>>
>>>> and in order to be sure not to end up in a dead-end, I think it would be good to have at least a sketch for the other archs as well..
>>>>
>>>> Just my 2c ofc, I think really here still ARM is behind, and we should help it catch up.
>>>>
>>>> If I had more time I would have pushed more on the ARM series, but.. yeah.
>>>
>>> IIUC Alex is waiting 6.2 release to respin.
>>>
>>
>> How does the Kconfig for ARM improvements go? I mean I think those improvements (enabling only compatible boards with the chosen accelerators) are important for both tcg-kvm split and possibly for modularization of ARM accelerators too right?
> 
> I think we all (Alex/you/me) reached the same point where builds work
> but current the testing framework isn't ready for non-TCG or
> modularized-TCG so the CI ends failing.
> 
> I don't want to push for 'CI build-only' because most of the annoying
> problems were from runtime (interfaces not resolved, ... which are
> important when using modules or board with unavailable devices).
> 
> I tried to address that with a QMP command to query accelerators but
> there is a disagreement whether we should query for available/built-in/
> loaded/modularized-but-not-installed/...). At this point I think I
> fairly understand the technical problems but misunderstand the big
> picture here, in particular w.r.t. management apps. I spent too many
> time on this to appear enthusiastic, sorry.
> 

The Kconfig thing also depended on the querying the accelerator part, or not?

I mean, was there the idea was "I want to build only the ARM boards that are compatible with the accel options I chose",
which is totally sensible, and removes all the need for workarounds in the tcg/kvm split series Alex is now maintaining.

I think you actually had a solution for quering the accelerators btw, and the problem was just caching the result to improve the performance?

Thanks,

Claudio

Re: modular tcg (was: Re: [PATCH v2 1/1] modules: Improve error message when module is not) found

[Paolo Bonzini]

On 29/07/21 11:14, Gerd Hoffmann wrote:
> The common functions could be added TCGCPUOps to allow them being called via
> CPUClass->tcg_ops->$name instead of a direct symbol reference.  Not sure this
> is a good idea though.  Experimental patch covering tcg_exec_realizefn +
> tcg_exec_unrealizefn below.

A lot of these (though probably not all) are already stubbed out as 
"static inline" in include/exec/exec-all.h.  I think they can be changed 
to function pointers in the style of ui/spice-module.c 
(accel/tcg/tcg-module.c?).

> No idea yet how to handle arch-specific bits best.  Seems there is no existing
> infrastructure to untangle target-specific code and tcg, so this probably needs
> something new.

If they are few enough, I would just move them out of target/i386/tcg 
and into target/i386/cpu-sysemu.c.

> Noticed softmmu/physmem.c has lots of CONFIG_TCG #ifdefs, splitting this into
> softmmu/physmem-{common,tcg}.c is probably a good idea.

I only count one, and those function should be easily moved  to 
accel/tcg/cputlb.c (after all both physmem.c and cputlb.c used to be a 
single file, exec.c, so this is just an oversight).


Paolo

Re: modular tcg

[Gerd Hoffmann]

  Hi,

On Thu, Jul 29, 2021 at 06:40:17PM +0200, Paolo Bonzini wrote:
> On 29/07/21 11:14, Gerd Hoffmann wrote:
> > The common functions could be added TCGCPUOps to allow them being called via
> > CPUClass->tcg_ops->$name instead of a direct symbol reference.  Not sure this
> > is a good idea though.  Experimental patch covering tcg_exec_realizefn +
> > tcg_exec_unrealizefn below.
> 
> A lot of these (though probably not all) are already stubbed out as "static
> inline" in include/exec/exec-all.h.  I think they can be changed to function
> pointers in the style of ui/spice-module.c (accel/tcg/tcg-module.c?).

Yep, was thinking about that too.  But then I noticed we already have
TCGCPUOps and wondered whenever extending that would be the better
option.

> > No idea yet how to handle arch-specific bits best.  Seems there is no existing
> > infrastructure to untangle target-specific code and tcg, so this probably needs
> > something new.
> 
> If they are few enough, I would just move them out of target/i386/tcg and
> into target/i386/cpu-sysemu.c.

I'll have a look.

> > Noticed softmmu/physmem.c has lots of CONFIG_TCG #ifdefs, splitting this into
> > softmmu/physmem-{common,tcg}.c is probably a good idea.
> 
> I only count one, and those function should be easily moved  to
> accel/tcg/cputlb.c (after all both physmem.c and cputlb.c used to be a
> single file, exec.c, so this is just an oversight).

Well, I noticed one larger block covering multiple functions, didn't
check the whole file ...

thanks & take care,
  Gerd

Re: modular tcg

[Claudio Fontana]

On 7/30/21 11:05 AM, Gerd Hoffmann wrote:
>   Hi,
> 
> On Thu, Jul 29, 2021 at 06:40:17PM +0200, Paolo Bonzini wrote:
>> On 29/07/21 11:14, Gerd Hoffmann wrote:
>>> The common functions could be added TCGCPUOps to allow them being called via
>>> CPUClass->tcg_ops->$name instead of a direct symbol reference.  Not sure this
>>> is a good idea though.  Experimental patch covering tcg_exec_realizefn +
>>> tcg_exec_unrealizefn below.
>>
>> A lot of these (though probably not all) are already stubbed out as "static
>> inline" in include/exec/exec-all.h.  I think they can be changed to function
>> pointers in the style of ui/spice-module.c (accel/tcg/tcg-module.c?).
> 
> Yep, was thinking about that too.  But then I noticed we already have
> TCGCPUOps and wondered whenever extending that would be the better
> option.

I'd read TCGCPUOps as TCG CPU-specific Operations in this case,
IIRC they are for the TCG Operations that have cpu target-specific behavior.

Maybe they should be called TCGCPUTargetOps to avoid confusion?

So, a TCGCPUOps will have an arm implementation, x86 implementation and so on.

In my view tcg_exec_realizefn does not fit, as this is just a set of additional
generic non-target-specific operations for the generic cpu_exec_realizefn() that needs to be called specifically and only for TCG.


> 
>>> No idea yet how to handle arch-specific bits best.  Seems there is no existing
>>> infrastructure to untangle target-specific code and tcg, so this probably needs
>>> something new.
>>
>> If they are few enough, I would just move them out of target/i386/tcg and
>> into target/i386/cpu-sysemu.c.
> 
> I'll have a look.
> 
>>> Noticed softmmu/physmem.c has lots of CONFIG_TCG #ifdefs, splitting this into
>>> softmmu/physmem-{common,tcg}.c is probably a good idea.
>>
>> I only count one, and those function should be easily moved  to
>> accel/tcg/cputlb.c (after all both physmem.c and cputlb.c used to be a
>> single file, exec.c, so this is just an oversight).
> 
> Well, I noticed one larger block covering multiple functions, didn't
> check the whole file ...
> 
> thanks & take care,
>   Gerd
> 

Re: modular tcg (was: Re: [PATCH v2 1/1] modules: Improve error message when module is not) found

[Claudio Fontana]

Hi all,

modularization of TCG is still incomplete in my view: we need to handle the error case better.

If we configure --enable-modules --enable-tcg --enable-kvm , but for some reason the .so is not present or is not loaded successfully,
and kvm fails to be enabled (for example, /dev/kvm not available etc),

we try to revert to tcg as fallback, and then fail with an unhelpful message:

ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)

What we should I think say:

ERROR: tcg has been configured and requested, but plugin could not be loaded.

I'll dig into it a bit, but another pair of eyes would be helful as well..

Thanks,

Claudio


On 7/29/21 11:14, Gerd Hoffmann wrote:
>   Hi,
> 
>>> So we need more work to make this actually work right.
>>
>> Yes.  I want have all of tcg in the tcg accel module, not only parts of
>> it, but that needs some more refactoring.  I'll go start looking at this
>> once I managed to wade through my vacation backlog.
> 
> So, changed the meson.build files to build tcg modular
> (https://git.kraxel.org/cgit/qemu/log/?h=sirius/modules-tcg-meson),
> then checked what unresolved symbols we have:
> 
> # -*- symbols -*-
>      18 tlb_flush
>       8 tb_flush
>       6 update_fp_status
>       6 cpu_restore_state
>       4 x86_register_ferr_irq
>       4 update_mxcsr_status
>       4 update_mxcsr_from_sse_status
>       4 tlb_flush_page
>       4 curr_cflags
>       3 cpu_cc_compute_all
>       2 tlb_reset_dirty
>       2 tlb_plugin_lookup
>       2 tcg_flush_softmmu_tlb
>       2 tcg_exec_unrealizefn
>       2 tcg_exec_realizefn
>       2 tb_invalidate_phys_range
>       2 tb_invalidate_phys_page_range
>       2 tb_check_watchpoint
>       2 cpu_x86_update_dr7
>       2 cpu_set_ignne
> # -*- files -*-
>      22 softmmu/physmem.c        tlb_flush tlb_flush_page curr_cflags tlb_reset_dirty tb_invalidate_phys_range tb_check_watchpoint cpu_restore_state
>      10 target/i386/helper.c     tlb_flush cpu_restore_state
>      15 cpu.c                    cpu_cc_compute_all tlb_flush tcg_flush_softmmu_tlb tcg_exec_unrealizefn tcg_exec_realizefn tb_invalidate_phys_page_range tb_flush
>       9 target/i386/cpu.h        update_fp_status cpu_cc_compute_all update_mxcsr_status
>       8 target/i386/machine.c    update_mxcsr_status update_fp_status tlb_flush cpu_x86_update_dr7
>       2 target/i386/gdbstub.c    update_mxcsr_from_sse_status
>       2 target/i386/cpu-dump.c   update_mxcsr_from_sse_status
>       2 plugins/loader.c         tb_flush
>       2 plugins/core.c           tb_flush
>       2 plugins/api.c            tlb_plugin_lookup
>       2 i386/pc_q35.c            x86_register_ferr_irq
>       2 i386/pc_piix.c           x86_register_ferr_irq
>       2 i386/pc.c                cpu_set_ignne
>       4 gdbstub.c                update_mxcsr_from_sse_status tb_flush
>       2 core/cpu-common.c        tcg_flush_softmmu_tlb
>       2 accel/tcg/cpu-exec-common.c cpu_restore_state
> 
> It's basically two groups:
> 
>  * Arch-specific (functions taking CPUX86State as argument), most of the
>    unresolved symbols in target/i386/ and i386/ directories go into this
>    category.
>  * Common (functions taking CPUState as argument).  Everything else.
> 
> The common functions could be added TCGCPUOps to allow them being called via
> CPUClass->tcg_ops->$name instead of a direct symbol reference.  Not sure this
> is a good idea though.  Experimental patch covering tcg_exec_realizefn +
> tcg_exec_unrealizefn below.
> 
> No idea yet how to handle arch-specific bits best.  Seems there is no existing
> infrastructure to untangle target-specific code and tcg, so this probably needs
> something new.
> 
> Noticed softmmu/physmem.c has lots of CONFIG_TCG #ifdefs, splitting this into
> softmmu/physmem-{common,tcg}.c is probably a good idea.
> 
> Comments?
> 
> take care,
>   Gerd
> 
> From c0e3f4cbe6aa6889d344beb4cac300b160253f00 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Thu, 29 Jul 2021 09:14:56 +0200
> Subject: [PATCH] tcg: add tcg_exec_{realizefn,unrealizefn} to TCGCPUOps
> 
> ---
>  include/hw/core/tcg-cpu-ops.h |  7 +++++++
>  cpu.c                         | 23 +++++++++++------------
>  target/alpha/cpu.c            |  1 +
>  target/arm/cpu.c              |  1 +
>  target/arm/cpu_tcg.c          |  1 +
>  target/avr/cpu.c              |  1 +
>  target/cris/cpu.c             |  1 +
>  target/hexagon/cpu.c          |  1 +
>  target/hppa/cpu.c             |  1 +
>  target/i386/tcg/tcg-cpu.c     |  1 +
>  target/m68k/cpu.c             |  1 +
>  target/microblaze/cpu.c       |  1 +
>  target/mips/cpu.c             |  1 +
>  target/nios2/cpu.c            |  1 +
>  target/openrisc/cpu.c         |  1 +
>  target/ppc/cpu_init.c         |  1 +
>  target/riscv/cpu.c            |  1 +
>  target/rx/cpu.c               |  1 +
>  target/s390x/cpu.c            |  1 +
>  target/sh4/cpu.c              |  1 +
>  target/sparc/cpu.c            |  1 +
>  target/tricore/cpu.c          |  1 +
>  target/xtensa/cpu.c           |  1 +
>  23 files changed, 39 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
> index eab27d0c0309..1f33953a4efc 100644
> --- a/include/hw/core/tcg-cpu-ops.h
> +++ b/include/hw/core/tcg-cpu-ops.h
> @@ -107,6 +107,13 @@ struct TCGCPUOps {
>  #endif /* CONFIG_SOFTMMU */
>  #endif /* NEED_CPU_H */
>  
> +    void (*exec_realizefn)(CPUState *cpu, Error **errp);
> +    void (*exec_unrealizefn)(CPUState *cpu);
>  };
>  
> +#define TCG_CPU_OPS_COMMON \
> +    .exec_realizefn = tcg_exec_realizefn, \
> +    .exec_unrealizefn = tcg_exec_unrealizefn
> +
> +
>  #endif /* TCG_CPU_OPS_H */
> diff --git a/cpu.c b/cpu.c
> index e1799a15bcf5..982463ce7418 100644
> --- a/cpu.c
> +++ b/cpu.c
> @@ -40,6 +40,10 @@
>  #include "hw/core/accel-cpu.h"
>  #include "trace/trace-root.h"
>  
> +#ifdef CONFIG_TCG
> +#include "hw/core/tcg-cpu-ops.h"
> +#endif /* CONFIG_TCG */
> +
>  uintptr_t qemu_host_page_size;
>  intptr_t qemu_host_page_mask;
>  
> @@ -129,20 +133,17 @@ const VMStateDescription vmstate_cpu_common = {
>  
>  void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>  {
> -#ifndef CONFIG_USER_ONLY
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> -#endif
>  
>      cpu_list_add(cpu);
>      if (!accel_cpu_realizefn(cpu, errp)) {
>          return;
>      }
> -#ifdef CONFIG_TCG
> +
>      /* NB: errp parameter is unused currently */
> -    if (tcg_enabled()) {
> -        tcg_exec_realizefn(cpu, errp);
> +    if (cc->tcg_ops) {
> +        cc->tcg_ops->exec_realizefn(cpu, errp);
>      }
> -#endif /* CONFIG_TCG */
>  
>  #ifdef CONFIG_USER_ONLY
>      assert(qdev_get_vmsd(DEVICE(cpu)) == NULL ||
> @@ -159,9 +160,9 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>  
>  void cpu_exec_unrealizefn(CPUState *cpu)
>  {
> -#ifndef CONFIG_USER_ONLY
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>  
> +#ifndef CONFIG_USER_ONLY
>      if (cc->sysemu_ops->legacy_vmsd != NULL) {
>          vmstate_unregister(NULL, cc->sysemu_ops->legacy_vmsd, cpu);
>      }
> @@ -169,12 +170,10 @@ void cpu_exec_unrealizefn(CPUState *cpu)
>          vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
>      }
>  #endif
> -#ifdef CONFIG_TCG
> -    /* NB: errp parameter is unused currently */
> -    if (tcg_enabled()) {
> -        tcg_exec_unrealizefn(cpu);
> +
> +    if (cc->tcg_ops) {
> +        cc->tcg_ops->exec_unrealizefn(cpu);
>      }
> -#endif /* CONFIG_TCG */
>  
>      cpu_list_remove(cpu);
>  }
> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
> index 4871ad0c0a6a..715fb97d6707 100644
> --- a/target/alpha/cpu.c
> +++ b/target/alpha/cpu.c
> @@ -226,6 +226,7 @@ static const struct TCGCPUOps alpha_tcg_ops = {
>      .do_transaction_failed = alpha_cpu_do_transaction_failed,
>      .do_unaligned_access = alpha_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void alpha_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 2866dd765882..7f051bd24e0b 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1996,6 +1996,7 @@ static const struct TCGCPUOps arm_tcg_ops = {
>      .debug_check_watchpoint = arm_debug_check_watchpoint,
>      .debug_check_breakpoint = arm_debug_check_breakpoint,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
> index ed444bf436a4..0f8657ede860 100644
> --- a/target/arm/cpu_tcg.c
> +++ b/target/arm/cpu_tcg.c
> @@ -913,6 +913,7 @@ static const struct TCGCPUOps arm_v7m_tcg_ops = {
>      .debug_check_watchpoint = arm_debug_check_watchpoint,
>      .debug_check_breakpoint = arm_debug_check_breakpoint,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index ea14175ca557..8b3261ef832c 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -201,6 +201,7 @@ static const struct TCGCPUOps avr_tcg_ops = {
>  #ifndef CONFIG_USER_ONLY
>      .do_interrupt = avr_cpu_do_interrupt,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void avr_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/cris/cpu.c b/target/cris/cpu.c
> index 70932b1f8c7d..32ca1051f050 100644
> --- a/target/cris/cpu.c
> +++ b/target/cris/cpu.c
> @@ -221,6 +221,7 @@ static const struct TCGCPUOps crisv32_tcg_ops = {
>  #ifndef CONFIG_USER_ONLY
>      .do_interrupt = cris_cpu_do_interrupt,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void crisv8_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
> index 3338365c16ec..86c001b67484 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -273,6 +273,7 @@ static const struct TCGCPUOps hexagon_tcg_ops = {
>      .initialize = hexagon_translate_init,
>      .synchronize_from_tb = hexagon_cpu_synchronize_from_tb,
>      .tlb_fill = hexagon_tlb_fill,
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void hexagon_cpu_class_init(ObjectClass *c, void *data)
> diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
> index 2eace4ee1240..5163118e986d 100644
> --- a/target/hppa/cpu.c
> +++ b/target/hppa/cpu.c
> @@ -151,6 +151,7 @@ static const struct TCGCPUOps hppa_tcg_ops = {
>      .do_interrupt = hppa_cpu_do_interrupt,
>      .do_unaligned_access = hppa_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void hppa_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
> index 93a79a574154..bf7d91fed6da 100644
> --- a/target/i386/tcg/tcg-cpu.c
> +++ b/target/i386/tcg/tcg-cpu.c
> @@ -79,6 +79,7 @@ static const struct TCGCPUOps x86_tcg_ops = {
>      .debug_excp_handler = breakpoint_handler,
>      .debug_check_breakpoint = x86_debug_check_breakpoint,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 72de6e972627..df06c07907d6 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -522,6 +522,7 @@ static const struct TCGCPUOps m68k_tcg_ops = {
>      .do_interrupt = m68k_cpu_do_interrupt,
>      .do_transaction_failed = m68k_cpu_transaction_failed,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void m68k_cpu_class_init(ObjectClass *c, void *data)
> diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
> index 72d8f2a0daa6..0e781264ef7b 100644
> --- a/target/microblaze/cpu.c
> +++ b/target/microblaze/cpu.c
> @@ -373,6 +373,7 @@ static const struct TCGCPUOps mb_tcg_ops = {
>      .do_transaction_failed = mb_cpu_transaction_failed,
>      .do_unaligned_access = mb_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void mb_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index d426918291a9..3216a33d0047 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -548,6 +548,7 @@ static const struct TCGCPUOps mips_tcg_ops = {
>      .do_unaligned_access = mips_cpu_do_unaligned_access,
>      .io_recompile_replay_branch = mips_io_recompile_replay_branch,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
> index 5e37defef805..55e21e8cd414 100644
> --- a/target/nios2/cpu.c
> +++ b/target/nios2/cpu.c
> @@ -226,6 +226,7 @@ static const struct TCGCPUOps nios2_tcg_ops = {
>      .do_interrupt = nios2_cpu_do_interrupt,
>      .do_unaligned_access = nios2_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void nios2_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
> index bd34e429ecbf..0d9e1aa58a8f 100644
> --- a/target/openrisc/cpu.c
> +++ b/target/openrisc/cpu.c
> @@ -192,6 +192,7 @@ static const struct TCGCPUOps openrisc_tcg_ops = {
>  #ifndef CONFIG_USER_ONLY
>      .do_interrupt = openrisc_cpu_do_interrupt,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void openrisc_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 505a0ed6ac09..3f8bf2e3fcbf 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -9021,6 +9021,7 @@ static const struct TCGCPUOps ppc_tcg_ops = {
>    .cpu_exec_exit = ppc_cpu_exec_exit,
>    .do_unaligned_access = ppc_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 991a6bb7604f..6a129b4fe211 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -650,6 +650,7 @@ static const struct TCGCPUOps riscv_tcg_ops = {
>      .do_transaction_failed = riscv_cpu_do_transaction_failed,
>      .do_unaligned_access = riscv_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void riscv_cpu_class_init(ObjectClass *c, void *data)
> diff --git a/target/rx/cpu.c b/target/rx/cpu.c
> index 96cc96e514fe..7a1ebc694f22 100644
> --- a/target/rx/cpu.c
> +++ b/target/rx/cpu.c
> @@ -192,6 +192,7 @@ static const struct TCGCPUOps rx_tcg_ops = {
>  #ifndef CONFIG_USER_ONLY
>      .do_interrupt = rx_cpu_do_interrupt,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void rx_cpu_class_init(ObjectClass *klass, void *data)
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 7b7b05f1d3a0..8dfcde1590c0 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -274,6 +274,7 @@ static const struct TCGCPUOps s390_tcg_ops = {
>      .debug_excp_handler = s390x_cpu_debug_excp_handler,
>      .do_unaligned_access = s390x_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
> index 832692294211..0b4e03913656 100644
> --- a/target/sh4/cpu.c
> +++ b/target/sh4/cpu.c
> @@ -244,6 +244,7 @@ static const struct TCGCPUOps superh_tcg_ops = {
>      .do_unaligned_access = superh_cpu_do_unaligned_access,
>      .io_recompile_replay_branch = superh_io_recompile_replay_branch,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void superh_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
> index da6b30ec747a..2b2d2946223b 100644
> --- a/target/sparc/cpu.c
> +++ b/target/sparc/cpu.c
> @@ -871,6 +871,7 @@ static const struct TCGCPUOps sparc_tcg_ops = {
>      .do_transaction_failed = sparc_cpu_do_transaction_failed,
>      .do_unaligned_access = sparc_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  #endif /* CONFIG_TCG */
>  
> diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
> index b95682b7f04d..daeaecca26e0 100644
> --- a/target/tricore/cpu.c
> +++ b/target/tricore/cpu.c
> @@ -154,6 +154,7 @@ static const struct TCGCPUOps tricore_tcg_ops = {
>      .initialize = tricore_tcg_init,
>      .synchronize_from_tb = tricore_cpu_synchronize_from_tb,
>      .tlb_fill = tricore_cpu_tlb_fill,
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void tricore_cpu_class_init(ObjectClass *c, void *data)
> diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
> index 58ec3a086224..d7d1b967040b 100644
> --- a/target/xtensa/cpu.c
> +++ b/target/xtensa/cpu.c
> @@ -201,6 +201,7 @@ static const struct TCGCPUOps xtensa_tcg_ops = {
>      .do_transaction_failed = xtensa_cpu_do_transaction_failed,
>      .do_unaligned_access = xtensa_cpu_do_unaligned_access,
>  #endif /* !CONFIG_USER_ONLY */
> +    TCG_CPU_OPS_COMMON,
>  };
>  
>  static void xtensa_cpu_class_init(ObjectClass *oc, void *data)