[PATCH 0/6] accel: Restrict TCG-specific code

[PATCH 0/6] accel: Restrict TCG-specific code

[Philippe Mathieu-Daudé]

Hi,

I've prepared some patches to have KVM-only builds.
Some patches are generic - well kind of, instead they are
TCG specific =) - so I'm sending them as a separate series.

Please review,

Phil.

Philippe Mathieu-Daudé (6):
  accel/tcg: Make cpu_gen_init() static
  accel/tcg: Restrict tb_flush_jmp_cache() from other accelerators
  accel/tcg: Restrict tb_gen_code() from other accelerators
  accel/tcg: Declare missing cpu_loop_exit*() stubs
  accel/tcg: Restrict cpu_io_recompile() from other accelerators
  softmmu: Restrict watchpoint handlers to TCG accelerator

 accel/tcg/internal.h      | 23 +++++++++++++++++++++++
 include/exec/exec-all.h   | 11 -----------
 include/hw/core/cpu.h     |  4 ++--
 accel/stubs/tcg-stub.c    | 10 ++++++++++
 accel/tcg/cpu-exec.c      |  1 +
 accel/tcg/cputlb.c        |  1 +
 accel/tcg/translate-all.c |  3 ++-
 accel/tcg/user-exec.c     |  1 +
 softmmu/physmem.c         |  4 ++++
 9 files changed, 44 insertions(+), 14 deletions(-)
 create mode 100644 accel/tcg/internal.h

-- 
2.26.2

[PATCH 1/6] accel/tcg: Make cpu_gen_init() static

[Philippe Mathieu-Daudé]

cpu_gen_init() is TCG specific, only used in tcg/translate-all.c.
No need to export it to other accelerators, declare it statically.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
We could also inline the 1-line call..
---
 include/exec/exec-all.h   | 2 --
 accel/tcg/translate-all.c | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 2e5b4bba48f..516013e735a 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -47,8 +47,6 @@ void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb, int max_insns);
 void restore_state_to_opc(CPUArchState *env, TranslationBlock *tb,
                           target_ulong *data);
 
-void cpu_gen_init(void);
-
 /**
  * cpu_restore_state:
  * @cpu: the vCPU state is to be restore to
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index e9de6ff9dd7..ca7ef6aa177 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -243,7 +243,7 @@ static void page_table_config_init(void)
     assert(v_l2_levels >= 0);
 }
 
-void cpu_gen_init(void)
+static void cpu_gen_init(void)
 {
     tcg_context_init(&tcg_init_ctx);
 }
-- 
2.26.2

Re: [PATCH 1/6] accel/tcg: Make cpu_gen_init() static

[Claudio Fontana]

On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
> cpu_gen_init() is TCG specific, only used in tcg/translate-all.c.
> No need to export it to other accelerators, declare it statically.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> We could also inline the 1-line call..
> ---
>  include/exec/exec-all.h   | 2 --
>  accel/tcg/translate-all.c | 2 +-
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 2e5b4bba48f..516013e735a 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -47,8 +47,6 @@ void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb, int max_insns);
>  void restore_state_to_opc(CPUArchState *env, TranslationBlock *tb,
>                            target_ulong *data);
>  
> -void cpu_gen_init(void);
> -
>  /**
>   * cpu_restore_state:
>   * @cpu: the vCPU state is to be restore to
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index e9de6ff9dd7..ca7ef6aa177 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -243,7 +243,7 @@ static void page_table_config_init(void)
>      assert(v_l2_levels >= 0);
>  }
>  
> -void cpu_gen_init(void)
> +static void cpu_gen_init(void)
>  {
>      tcg_context_init(&tcg_init_ctx);
>  }
> 


Reviewed-by: Claudio Fontana <cfontana@suse.de>

Re: [PATCH 1/6] accel/tcg: Make cpu_gen_init() static

[Richard Henderson]

On 1/17/21 6:48 AM, Philippe Mathieu-Daudé wrote:
> cpu_gen_init() is TCG specific, only used in tcg/translate-all.c.
> No need to export it to other accelerators, declare it statically.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> We could also inline the 1-line call..
> ---
>  include/exec/exec-all.h   | 2 --
>  accel/tcg/translate-all.c | 2 +-
>  2 files changed, 1 insertion(+), 3 deletions(-)

Applied to tcg-next.

r~

[PATCH 2/6] accel/tcg: Restrict tb_flush_jmp_cache() from other accelerators

[Philippe Mathieu-Daudé]

tb_flush_jmp_cache() is only called within TCG accelerator,
declare it locally.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
We could also inline it in cputlb.c, the single user.
---
 accel/tcg/internal.h      | 16 ++++++++++++++++
 include/exec/exec-all.h   |  3 ---
 accel/tcg/cputlb.c        |  1 +
 accel/tcg/translate-all.c |  1 +
 4 files changed, 18 insertions(+), 3 deletions(-)
 create mode 100644 accel/tcg/internal.h

diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
new file mode 100644
index 00000000000..4981d98dbfd
--- /dev/null
+++ b/accel/tcg/internal.h
@@ -0,0 +1,16 @@
+/*
+ * internal execution defines for qemu
+ *
+ *  Copyright (c) 2003 Fabrice Bellard
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+
+#ifndef ACCEL_TCG_INTERNAL_H
+#define ACCEL_TCG_INTERNAL_H
+
+#include "exec/exec-all.h"
+
+void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr);
+
+#endif
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 516013e735a..1e3e7cf8e78 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -663,9 +663,6 @@ tb_page_addr_t get_page_addr_code_hostp(CPUArchState *env, target_ulong addr,
 void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length);
 void tlb_set_dirty(CPUState *cpu, target_ulong vaddr);
 
-/* exec.c */
-void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr);
-
 MemoryRegionSection *
 address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
                                   hwaddr *xlat, hwaddr *plen,
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index ced3dc077ec..b1f0f404aa5 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -36,6 +36,7 @@
 #include "exec/translate-all.h"
 #include "trace/trace-root.h"
 #include "trace/mem.h"
+#include "internal.h"
 #ifdef CONFIG_PLUGIN
 #include "qemu/plugin-memory.h"
 #endif
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index ca7ef6aa177..6427bf87ae0 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -60,6 +60,7 @@
 #include "sysemu/cpu-timers.h"
 #include "sysemu/tcg.h"
 #include "qapi/error.h"
+#include "internal.h"
 
 /* #define DEBUG_TB_INVALIDATE */
 /* #define DEBUG_TB_FLUSH */
-- 
2.26.2

Re: [PATCH 2/6] accel/tcg: Restrict tb_flush_jmp_cache() from other accelerators

[Claudio Fontana]

On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
> tb_flush_jmp_cache() is only called within TCG accelerator,
> declare it locally.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> We could also inline it in cputlb.c, the single user.

That is what I was thinking, I would be more inclined to put it as a static function there.

Thanks,

Claudio

> ---
>  accel/tcg/internal.h      | 16 ++++++++++++++++
>  include/exec/exec-all.h   |  3 ---
>  accel/tcg/cputlb.c        |  1 +
>  accel/tcg/translate-all.c |  1 +
>  4 files changed, 18 insertions(+), 3 deletions(-)
>  create mode 100644 accel/tcg/internal.h
> 
> diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
> new file mode 100644
> index 00000000000..4981d98dbfd
> --- /dev/null
> +++ b/accel/tcg/internal.h
> @@ -0,0 +1,16 @@
> +/*
> + * internal execution defines for qemu
> + *
> + *  Copyright (c) 2003 Fabrice Bellard
> + *
> + * SPDX-License-Identifier: LGPL-2.1-or-later
> + */
> +
> +#ifndef ACCEL_TCG_INTERNAL_H
> +#define ACCEL_TCG_INTERNAL_H
> +
> +#include "exec/exec-all.h"
> +
> +void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr);
> +
> +#endif
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 516013e735a..1e3e7cf8e78 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -663,9 +663,6 @@ tb_page_addr_t get_page_addr_code_hostp(CPUArchState *env, target_ulong addr,
>  void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length);
>  void tlb_set_dirty(CPUState *cpu, target_ulong vaddr);
>  
> -/* exec.c */
> -void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr);
> -
>  MemoryRegionSection *
>  address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
>                                    hwaddr *xlat, hwaddr *plen,
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index ced3dc077ec..b1f0f404aa5 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -36,6 +36,7 @@
>  #include "exec/translate-all.h"
>  #include "trace/trace-root.h"
>  #include "trace/mem.h"
> +#include "internal.h"
>  #ifdef CONFIG_PLUGIN
>  #include "qemu/plugin-memory.h"
>  #endif
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index ca7ef6aa177..6427bf87ae0 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -60,6 +60,7 @@
>  #include "sysemu/cpu-timers.h"
>  #include "sysemu/tcg.h"
>  #include "qapi/error.h"
> +#include "internal.h"
>  
>  /* #define DEBUG_TB_INVALIDATE */
>  /* #define DEBUG_TB_FLUSH */
> 

Re: [PATCH 2/6] accel/tcg: Restrict tb_flush_jmp_cache() from other accelerators

[Richard Henderson]

On 1/17/21 6:48 AM, Philippe Mathieu-Daudé wrote:
> tb_flush_jmp_cache() is only called within TCG accelerator,
> declare it locally.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> We could also inline it in cputlb.c, the single user.
> ---

Yes, I think we should move it to cputlb.c.
I have queued a patch to do so.


r~

[PATCH 3/6] accel/tcg: Restrict tb_gen_code() from other accelerators

[Philippe Mathieu-Daudé]

tb_gen_code() is only called within TCG accelerator,
declare it locally.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 accel/tcg/internal.h    | 5 +++++
 include/exec/exec-all.h | 5 -----
 accel/tcg/cpu-exec.c    | 1 +
 accel/tcg/user-exec.c   | 1 +
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
index 4981d98dbfd..f7e18c3498b 100644
--- a/accel/tcg/internal.h
+++ b/accel/tcg/internal.h
@@ -11,6 +11,11 @@
 
 #include "exec/exec-all.h"
 
+TranslationBlock *tb_gen_code(CPUState *cpu,
+                              target_ulong pc, target_ulong cs_base,
+                              uint32_t flags,
+                              int cflags);
+
 void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr);
 
 #endif
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 1e3e7cf8e78..3acc7c2943a 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -64,11 +64,6 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool will_exit);
 
 void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu);
 void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr);
-TranslationBlock *tb_gen_code(CPUState *cpu,
-                              target_ulong pc, target_ulong cs_base,
-                              uint32_t flags,
-                              int cflags);
-
 void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
 void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
 void QEMU_NORETURN cpu_loop_exit_atomic(CPUState *cpu, uintptr_t pc);
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index e0df9b6a1dd..43676ae8d13 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -41,6 +41,7 @@
 #include "exec/cpu-all.h"
 #include "sysemu/cpu-timers.h"
 #include "sysemu/replay.h"
+#include "internal.h"
 
 /* -icount align implementation. */
 
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 1215b55ca08..05f3c09cbf9 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -28,6 +28,7 @@
 #include "qemu/atomic128.h"
 #include "trace/trace-root.h"
 #include "trace/mem.h"
+#include "internal.h"
 
 #undef EAX
 #undef ECX
-- 
2.26.2

Re: [PATCH 3/6] accel/tcg: Restrict tb_gen_code() from other accelerators

[Claudio Fontana]

On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
> tb_gen_code() is only called within TCG accelerator,
> declare it locally.

Is this used only in accel/tcg/cpu-exec.c ? Should it be a static function there?

Ciao,

Claudio

> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  accel/tcg/internal.h    | 5 +++++
>  include/exec/exec-all.h | 5 -----
>  accel/tcg/cpu-exec.c    | 1 +
>  accel/tcg/user-exec.c   | 1 +
>  4 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
> index 4981d98dbfd..f7e18c3498b 100644
> --- a/accel/tcg/internal.h
> +++ b/accel/tcg/internal.h
> @@ -11,6 +11,11 @@
>  
>  #include "exec/exec-all.h"
>  
> +TranslationBlock *tb_gen_code(CPUState *cpu,
> +                              target_ulong pc, target_ulong cs_base,
> +                              uint32_t flags,
> +                              int cflags);
> +
>  void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr);
>  
>  #endif
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 1e3e7cf8e78..3acc7c2943a 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -64,11 +64,6 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool will_exit);
>  
>  void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu);
>  void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr);
> -TranslationBlock *tb_gen_code(CPUState *cpu,
> -                              target_ulong pc, target_ulong cs_base,
> -                              uint32_t flags,
> -                              int cflags);
> -
>  void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
>  void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
>  void QEMU_NORETURN cpu_loop_exit_atomic(CPUState *cpu, uintptr_t pc);
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index e0df9b6a1dd..43676ae8d13 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -41,6 +41,7 @@
>  #include "exec/cpu-all.h"
>  #include "sysemu/cpu-timers.h"
>  #include "sysemu/replay.h"
> +#include "internal.h"
>  
>  /* -icount align implementation. */
>  
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index 1215b55ca08..05f3c09cbf9 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -28,6 +28,7 @@
>  #include "qemu/atomic128.h"
>  #include "trace/trace-root.h"
>  #include "trace/mem.h"
> +#include "internal.h"
>  
>  #undef EAX
>  #undef ECX
> 

Re: [PATCH 3/6] accel/tcg: Restrict tb_gen_code() from other accelerators

[Richard Henderson]

On 1/17/21 11:12 PM, Claudio Fontana wrote:
> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
>> tb_gen_code() is only called within TCG accelerator,
>> declare it locally.
> 
> Is this used only in accel/tcg/cpu-exec.c ? Should it be a static function there?

Possibly, but there's a *lot* of code that would have to be moved.  For now,
queuing a slightly modified version of the patch.

>> --- a/accel/tcg/user-exec.c
>> +++ b/accel/tcg/user-exec.c
>> @@ -28,6 +28,7 @@
>>  #include "qemu/atomic128.h"
>>  #include "trace/trace-root.h"
>>  #include "trace/mem.h"
>> +#include "internal.h"

Not needed by this patch.


r~

Re: [PATCH 3/6] accel/tcg: Restrict tb_gen_code() from other accelerators

[Claudio Fontana]

On 1/21/21 7:06 AM, Richard Henderson wrote:
> On 1/17/21 11:12 PM, Claudio Fontana wrote:
>> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
>>> tb_gen_code() is only called within TCG accelerator,
>>> declare it locally.
>>
>> Is this used only in accel/tcg/cpu-exec.c ? Should it be a static function there?
> 
> Possibly, but there's a *lot* of code that would have to be moved.  For now,
> queuing a slightly modified version of the patch.
> 
>>> --- a/accel/tcg/user-exec.c
>>> +++ b/accel/tcg/user-exec.c
>>> @@ -28,6 +28,7 @@
>>>  #include "qemu/atomic128.h"
>>>  #include "trace/trace-root.h"
>>>  #include "trace/mem.h"
>>> +#include "internal.h"
> 
> Not needed by this patch.
> 
> 
> r~
> 

Hello,

resurrecting this, and in reference to its commit: "c03f041f128301c6a6c32242846be08719cd4fc3",

the name "internal.h" ends up polluting the include paths,
so that when working for example on s390x, including "internal.h" ends up including this instead of the file in target/s390x/.

I am not sure what exactly the right solution is, for this specific problem,
and if we should look at the include paths settings in detail,

but in my view calling files just "internal.h" or "internals.h" in general is not a good idea.

I can see two issues with this naming:

1) it describes nothing about the actual intended contents, other that they should be "internal".
Rather it would be better to know what the file is intended to contain, or we end up (as we end up) with very large files containing completely unrelated content.

2) we end up with clashes in our include paths if we are not super careful.

Probably in this case, the target/s390x/internal.h could be given another name (s390x-internal.h) and then split up in the future (there is a whole bunch of unrelated suff).

For accel/tcg/internal.h, maybe renaming it, or removing it altogether could both be good options?

Thanks,

Claudio

Re: [PATCH 3/6] accel/tcg: Restrict tb_gen_code() from other accelerators

[Philippe Mathieu-Daudé]

On 3/15/21 2:52 PM, Claudio Fontana wrote:
> On 1/21/21 7:06 AM, Richard Henderson wrote:
>> On 1/17/21 11:12 PM, Claudio Fontana wrote:
>>> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
>>>> tb_gen_code() is only called within TCG accelerator,
>>>> declare it locally.
>>>
>>> Is this used only in accel/tcg/cpu-exec.c ? Should it be a static function there?
>>
>> Possibly, but there's a *lot* of code that would have to be moved.  For now,
>> queuing a slightly modified version of the patch.
>>
>>>> --- a/accel/tcg/user-exec.c
>>>> +++ b/accel/tcg/user-exec.c
>>>> @@ -28,6 +28,7 @@
>>>>  #include "qemu/atomic128.h"
>>>>  #include "trace/trace-root.h"
>>>>  #include "trace/mem.h"
>>>> +#include "internal.h"
>>
>> Not needed by this patch.
>>
>>
>> r~
>>
> 
> Hello,
> 
> resurrecting this, and in reference to its commit: "c03f041f128301c6a6c32242846be08719cd4fc3",
> 
> the name "internal.h" ends up polluting the include paths,
> so that when working for example on s390x, including "internal.h" ends up including this instead of the file in target/s390x/.
> 
> I am not sure what exactly the right solution is, for this specific problem,
> and if we should look at the include paths settings in detail,
> 
> but in my view calling files just "internal.h" or "internals.h" in general is not a good idea.
> 
> I can see two issues with this naming:
> 
> 1) it describes nothing about the actual intended contents, other that they should be "internal".
> Rather it would be better to know what the file is intended to contain, or we end up (as we end up) with very large files containing completely unrelated content.
> 
> 2) we end up with clashes in our include paths if we are not super careful.
> 
> Probably in this case, the target/s390x/internal.h could be given another name (s390x-internal.h) and then split up in the future (there is a whole bunch of unrelated suff).
> 
> For accel/tcg/internal.h, maybe renaming it, or removing it altogether could both be good options?

I think something like commit ed5bad39e57 is required, restricting
the scope of:

  add_project_arguments('-iquote',
                        meson.current_source_dir() / 'tcg' / tcg_arch,

... to tcg, and ...
                        '-iquote',
                        meson.current_source_dir() / 'accel/tcg',

to accel.

                        language: ['c', 'cpp', 'objc'])

[PATCH 4/6] accel/tcg: Declare missing cpu_loop_exit*() stubs

[Philippe Mathieu-Daudé]

cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c,
and are not available when TCG accelerator is not built. Add stubs so
linking without TCG succeed.

Problematic files:

- hw/semihosting/console.c in qemu_semihosting_console_inc()
- hw/ppc/spapr_hcall.c in h_confer()
- hw/s390x/ipl.c in s390_ipl_reset_request()
- hw/misc/mips_itu.c

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
I suppose the s390x kvm-only build didn't catch this because
of compiler optimization:

in s390_ipl_reset_request():

640     if (tcg_enabled()) {
641         cpu_loop_exit(cs);
642     }

and "sysemu/tcg.h" is:

 13 #ifdef CONFIG_TCG
 14 extern bool tcg_allowed;
 15 #define tcg_enabled() (tcg_allowed)
 16 #else
 17 #define tcg_enabled() 0
 18 #endif
---
 accel/stubs/tcg-stub.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c
index 8c18d3eabdd..2304606f8e0 100644
--- a/accel/stubs/tcg-stub.c
+++ b/accel/stubs/tcg-stub.c
@@ -28,3 +28,13 @@ void *probe_access(CPUArchState *env, target_ulong addr, int size,
      /* Handled by hardware accelerator. */
      g_assert_not_reached();
 }
+
+void QEMU_NORETURN cpu_loop_exit(CPUState *cpu)
+{
+    g_assert_not_reached();
+}
+
+void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc)
+{
+    g_assert_not_reached();
+}
-- 
2.26.2

Re: [PATCH 4/6] accel/tcg: Declare missing cpu_loop_exit*() stubs

[Claudio Fontana]

On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
> cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c,
> and are not available when TCG accelerator is not built. Add stubs so
> linking without TCG succeed.
> 
> Problematic files:
> 
> - hw/semihosting/console.c in qemu_semihosting_console_inc()
> - hw/ppc/spapr_hcall.c in h_confer()
> - hw/s390x/ipl.c in s390_ipl_reset_request()
> - hw/misc/mips_itu.c
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> I suppose the s390x kvm-only build didn't catch this because
> of compiler optimization:
> 
> in s390_ipl_reset_request():
> 
> 640     if (tcg_enabled()) {
> 641         cpu_loop_exit(cs);
> 642     }

Ciao Philippe,

yes I have seen this a lot on x86 also, and seems to depend on the compiler used.

On OpenSUSE 15.2, with gcc based on 7.5.0, I am getting this optimization also, and so no error happens in these cases.

It is a bit inconvenient because I have to rely completely on the CI to catch these situations.

Ciao,

Claudio



> 
> and "sysemu/tcg.h" is:
> 
>  13 #ifdef CONFIG_TCG
>  14 extern bool tcg_allowed;
>  15 #define tcg_enabled() (tcg_allowed)
>  16 #else
>  17 #define tcg_enabled() 0
>  18 #endif
> ---
>  accel/stubs/tcg-stub.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c
> index 8c18d3eabdd..2304606f8e0 100644
> --- a/accel/stubs/tcg-stub.c
> +++ b/accel/stubs/tcg-stub.c
> @@ -28,3 +28,13 @@ void *probe_access(CPUArchState *env, target_ulong addr, int size,
>       /* Handled by hardware accelerator. */
>       g_assert_not_reached();
>  }
> +
> +void QEMU_NORETURN cpu_loop_exit(CPUState *cpu)
> +{
> +    g_assert_not_reached();
> +}
> +
> +void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc)
> +{
> +    g_assert_not_reached();
> +}
> 

Re: [PATCH 4/6] accel/tcg: Declare missing cpu_loop_exit*() stubs

[Claudio Fontana]

On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
> cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c,
> and are not available when TCG accelerator is not built. Add stubs so
> linking without TCG succeed.

The reason why stubs are needed here at all seems to be that that the code
calling cpu_loop_exit is not refactored properly yet;

if we look at the example of i386, after the refactoring moving tcg related code into target/i386/tcg/,
(and really even before that I think),
the code calling cpu_loop_exit is not built for non-TCG at all, and so we don't need stubs.

I am ok with this anyway, just wanted to convey that I think we should look at stubs as a necessary evil until all code stops mixing tcg, kvm and other accels...

Thanks,

Claudio

> 
> Problematic files:
> 
> - hw/semihosting/console.c in qemu_semihosting_console_inc()
> - hw/ppc/spapr_hcall.c in h_confer()
> - hw/s390x/ipl.c in s390_ipl_reset_request()
> - hw/misc/mips_itu.c
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> I suppose the s390x kvm-only build didn't catch this because
> of compiler optimization:
> 
> in s390_ipl_reset_request():
> 
> 640     if (tcg_enabled()) {
> 641         cpu_loop_exit(cs);
> 642     }
> 
> and "sysemu/tcg.h" is:
> 
>  13 #ifdef CONFIG_TCG
>  14 extern bool tcg_allowed;
>  15 #define tcg_enabled() (tcg_allowed)
>  16 #else
>  17 #define tcg_enabled() 0
>  18 #endif
> ---
>  accel/stubs/tcg-stub.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c
> index 8c18d3eabdd..2304606f8e0 100644
> --- a/accel/stubs/tcg-stub.c
> +++ b/accel/stubs/tcg-stub.c
> @@ -28,3 +28,13 @@ void *probe_access(CPUArchState *env, target_ulong addr, int size,
>       /* Handled by hardware accelerator. */
>       g_assert_not_reached();
>  }
> +
> +void QEMU_NORETURN cpu_loop_exit(CPUState *cpu)
> +{
> +    g_assert_not_reached();
> +}
> +
> +void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc)
> +{
> +    g_assert_not_reached();
> +}
> 

Re: [PATCH 4/6] accel/tcg: Declare missing cpu_loop_exit*() stubs

[Philippe Mathieu-Daudé]

On 1/18/21 10:29 AM, Claudio Fontana wrote:
> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
>> cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c,
>> and are not available when TCG accelerator is not built. Add stubs so
>> linking without TCG succeed.
> 
> The reason why stubs are needed here at all seems to be that that the code
> calling cpu_loop_exit is not refactored properly yet;

I agree ...

> if we look at the example of i386, after the refactoring moving tcg related code into target/i386/tcg/,
> (and really even before that I think),
> the code calling cpu_loop_exit is not built for non-TCG at all, and so we don't need stubs.
> 
> I am ok with this anyway, just wanted to convey that I think we should look at stubs as a necessary evil until all code stops mixing tcg, kvm and other accels...
> 
> Thanks,
> 
> Claudio
> 
>>
>> Problematic files:
>>
>> - hw/semihosting/console.c in qemu_semihosting_console_inc()
>> - hw/ppc/spapr_hcall.c in h_confer()
>> - hw/s390x/ipl.c in s390_ipl_reset_request()
>> - hw/misc/mips_itu.c

... but I have no clue how to refactore these, as they
are used in both KVM and TCG.

How would you do? I'm stuck with the semihosting code
dependency on ARM since 2 years...

Phil.

Re: [PATCH 4/6] accel/tcg: Declare missing cpu_loop_exit*() stubs

[Claudio Fontana]

On 1/18/21 10:39 AM, Philippe Mathieu-Daudé wrote:
> On 1/18/21 10:29 AM, Claudio Fontana wrote:
>> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
>>> cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c,
>>> and are not available when TCG accelerator is not built. Add stubs so
>>> linking without TCG succeed.
>>
>> The reason why stubs are needed here at all seems to be that that the code
>> calling cpu_loop_exit is not refactored properly yet;
> 
> I agree ...
> 
>> if we look at the example of i386, after the refactoring moving tcg related code into target/i386/tcg/,
>> (and really even before that I think),
>> the code calling cpu_loop_exit is not built for non-TCG at all, and so we don't need stubs.
>>
>> I am ok with this anyway, just wanted to convey that I think we should look at stubs as a necessary evil until all code stops mixing tcg, kvm and other accels...
>>
>> Thanks,
>>
>> Claudio
>>
>>>
>>> Problematic files:
>>>
>>> - hw/semihosting/console.c in qemu_semihosting_console_inc()
>>> - hw/ppc/spapr_hcall.c in h_confer()
>>> - hw/s390x/ipl.c in s390_ipl_reset_request()
>>> - hw/misc/mips_itu.c
> 
> ... but I have no clue how to refactore these, as they
> are used in both KVM and TCG.
> 
> How would you do? I'm stuck with the semihosting code
> dependency on ARM since 2 years...
> 
> Phil.
> 

Just naively looking at this, qemu_semihosting_console_inc seems called only by
do_arm_semihosting in target/arm/arm-semi.c,

which in turn is called by linux-user (TCG),

target/arm/m_helper.c in arm_v7m_cpu_do_interrupt(),
which I would assume is TCG only too, just waiting for the TCG/KVM refactoring in ARM, which I would assume would make cpu_tcg.c TCG-only,

target/arm/helper.c in handle_semihosting, which is already wrapped in #ifdef CONFIG_TCG and is commented with:

"
* We only see semihosting exceptions in TCG only as they are not                                                                           
* trapped to the hypervisor in KVM.                                                                                                        
*/
"

So am I wrong in my assumption that as soon as we are able to separate TCG vs KVM in target/arm/ , the issue of hw/semihosting/console.c would be solved?

Did not look at the other cases.

Ciao!

Claudio

Re: [PATCH 4/6] accel/tcg: Declare missing cpu_loop_exit*() stubs

[Alex Bennée]

Claudio Fontana <cfontana@suse.de> writes:

> On 1/18/21 10:39 AM, Philippe Mathieu-Daudé wrote:
>> On 1/18/21 10:29 AM, Claudio Fontana wrote:
>>> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
>>>> cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c,
>>>> and are not available when TCG accelerator is not built. Add stubs so
>>>> linking without TCG succeed.
>>>
>>> The reason why stubs are needed here at all seems to be that that the code
>>> calling cpu_loop_exit is not refactored properly yet;
>> 
>> I agree ...
>> 
>>> if we look at the example of i386, after the refactoring moving tcg related code into target/i386/tcg/,
>>> (and really even before that I think),
>>> the code calling cpu_loop_exit is not built for non-TCG at all, and so we don't need stubs.
>>>
>>> I am ok with this anyway, just wanted to convey that I think we should look at stubs as a necessary evil until all code stops mixing tcg, kvm and other accels...
>>>
>>> Thanks,
>>>
>>> Claudio
>>>
>>>>
>>>> Problematic files:
>>>>
>>>> - hw/semihosting/console.c in qemu_semihosting_console_inc()
>>>> - hw/ppc/spapr_hcall.c in h_confer()
>>>> - hw/s390x/ipl.c in s390_ipl_reset_request()
>>>> - hw/misc/mips_itu.c
>> 
>> ... but I have no clue how to refactore these, as they
>> are used in both KVM and TCG.
>> 
>> How would you do? I'm stuck with the semihosting code
>> dependency on ARM since 2 years...
>> 
>> Phil.
>> 
>
> Just naively looking at this, qemu_semihosting_console_inc seems called only by
> do_arm_semihosting in target/arm/arm-semi.c,
>
> which in turn is called by linux-user (TCG),
>
> target/arm/m_helper.c in arm_v7m_cpu_do_interrupt(),
> which I would assume is TCG only too, just waiting for the TCG/KVM refactoring in ARM, which I would assume would make cpu_tcg.c TCG-only,
>
> target/arm/helper.c in handle_semihosting, which is already wrapped in #ifdef CONFIG_TCG and is commented with:
>
> "
> * We only see semihosting exceptions in TCG only as they are not                                                                           
> * trapped to the hypervisor in KVM.                                                                                                        
> */
> "
>
> So am I wrong in my assumption that as soon as we are able to separate
> TCG vs KVM in target/arm/ , the issue of hw/semihosting/console.c
> would be solved?

I think it is - certainly for ARM. I don't know if real RiscV HW can
trap semihosting calls to the kernel/hypervisor.

-- 
Alex Bennée

Re: [PATCH 4/6] accel/tcg: Declare missing cpu_loop_exit*() stubs

[Richard Henderson]

On 1/17/21 6:48 AM, Philippe Mathieu-Daudé wrote:
> cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c,
> and are not available when TCG accelerator is not built. Add stubs so
> linking without TCG succeed.
> 
> Problematic files:
> 
> - hw/semihosting/console.c in qemu_semihosting_console_inc()
> - hw/ppc/spapr_hcall.c in h_confer()
> - hw/s390x/ipl.c in s390_ipl_reset_request()
> - hw/misc/mips_itu.c
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

Queued to tcg-next.


r~

[RFC PATCH 5/6] accel/tcg: Restrict cpu_io_recompile() from other accelerators

[Philippe Mathieu-Daudé]

As cpu_io_recompile() is only called within TCG accelerator
in cputlb.c, declare it locally.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC because not sure if other accelerator could implement this.
---
 accel/tcg/internal.h    | 2 ++
 include/exec/exec-all.h | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
index f7e18c3498b..c72a69e4d63 100644
--- a/accel/tcg/internal.h
+++ b/accel/tcg/internal.h
@@ -18,4 +18,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
 
 void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr);
 
+void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr);
+
 #endif
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 3acc7c2943a..125000bcf70 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -63,7 +63,6 @@ void restore_state_to_opc(CPUArchState *env, TranslationBlock *tb,
 bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool will_exit);
 
 void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu);
-void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr);
 void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
 void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
 void QEMU_NORETURN cpu_loop_exit_atomic(CPUState *cpu, uintptr_t pc);
-- 
2.26.2

Re: [RFC PATCH 5/6] accel/tcg: Restrict cpu_io_recompile() from other accelerators

[Claudio Fontana]

On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
> As cpu_io_recompile() is only called within TCG accelerator
> in cputlb.c, declare it locally.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

It's only used in accel/tcg/cputlb.c, should it be a static function there?


> ---
> RFC because not sure if other accelerator could implement this.
> ---
>  accel/tcg/internal.h    | 2 ++
>  include/exec/exec-all.h | 1 -
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
> index f7e18c3498b..c72a69e4d63 100644
> --- a/accel/tcg/internal.h
> +++ b/accel/tcg/internal.h
> @@ -18,4 +18,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>  
>  void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr);
>  
> +void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr);
> +
>  #endif
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 3acc7c2943a..125000bcf70 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -63,7 +63,6 @@ void restore_state_to_opc(CPUArchState *env, TranslationBlock *tb,
>  bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool will_exit);
>  
>  void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu);
> -void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr);
>  void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
>  void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
>  void QEMU_NORETURN cpu_loop_exit_atomic(CPUState *cpu, uintptr_t pc);
> 

Re: [RFC PATCH 5/6] accel/tcg: Restrict cpu_io_recompile() from other accelerators

[Richard Henderson]

On 1/17/21 6:48 AM, Philippe Mathieu-Daudé wrote:
> As cpu_io_recompile() is only called within TCG accelerator
> in cputlb.c, declare it locally.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> RFC because not sure if other accelerator could implement this.
> ---
>  accel/tcg/internal.h    | 2 ++
>  include/exec/exec-all.h | 1 -
>  2 files changed, 2 insertions(+), 1 deletion(-)

Queued to tcg-next.

r~

[RFC PATCH 6/6] softmmu: Restrict watchpoint handlers to TCG accelerator

[Philippe Mathieu-Daudé]

Watchpoint funtions use cpu_restore_state() which is only
available when TCG accelerator is built. Restrict them
to TCG.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC because we could keep that code by adding an empty
    stub for cpu_restore_state(), but it is unclear as
    the function is named generically.
---
 include/hw/core/cpu.h | 4 ++--
 softmmu/physmem.c     | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 140fa32a5e3..1b4af30db04 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -1033,7 +1033,7 @@ static inline bool cpu_breakpoint_test(CPUState *cpu, vaddr pc, int mask)
     return false;
 }
 
-#ifdef CONFIG_USER_ONLY
+#if !defined(CONFIG_TCG) || defined(CONFIG_USER_ONLY)
 static inline int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
                                         int flags, CPUWatchpoint **watchpoint)
 {
@@ -1098,7 +1098,7 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
  * If no watchpoint is registered for the range, the result is 0.
  */
 int cpu_watchpoint_address_matches(CPUState *cpu, vaddr addr, vaddr len);
-#endif
+#endif /* !CONFIG_TCG || CONFIG_USER_ONLY */
 
 /**
  * cpu_get_address_space:
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 65602ed548e..5135a6371b5 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -765,6 +765,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
     return cpu->cpu_ases[asidx].as;
 }
 
+#ifdef CONFIG_TCG
 /* Add a watchpoint.  */
 int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
                           int flags, CPUWatchpoint **watchpoint)
@@ -873,6 +874,7 @@ int cpu_watchpoint_address_matches(CPUState *cpu, vaddr addr, vaddr len)
     }
     return ret;
 }
+#endif /* CONFIG_TCG */
 
 /* Called from RCU critical section */
 static RAMBlock *qemu_get_ram_block(ram_addr_t addr)
@@ -2356,6 +2358,7 @@ ram_addr_t qemu_ram_addr_from_host(void *ptr)
     return block->offset + offset;
 }
 
+#ifdef CONFIG_TCG
 /* Generate a debug exception if a watchpoint has been hit.  */
 void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
                           MemTxAttrs attrs, int flags, uintptr_t ra)
@@ -2424,6 +2427,7 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
         }
     }
 }
+#endif /* CONFIG_TCG */
 
 static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
                                  MemTxAttrs attrs, void *buf, hwaddr len);
-- 
2.26.2

Re: [RFC PATCH 6/6] softmmu: Restrict watchpoint handlers to TCG accelerator

[Claudio Fontana]

On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
> Watchpoint funtions use cpu_restore_state() which is only
> available when TCG accelerator is built. Restrict them
> to TCG.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

I am doing some of this in my series, and I did not notice that
cpu_watchpoint_insert was also TCG only.

Probably we should merge this somehow.

I thought it was used by gdbstub.c as well, passing flags BP_GDB .

I noticed that gdbstub does something else entirely for kvm_enabled(), ie, kvm_insert_breakpoint,
but what about the other accels, it seems that the code flows to the cpu_breakpoint_insert and watchpoint_insert..?

should cpu_breakpoint_insert have the same fate then?

And is this really all TCG specific?

From gdbstub.c:1020:

static int gdb_breakpoint_insert(int type, target_ulong addr, target_ulong len)
{
    CPUState *cpu;
    int err = 0;

    if (kvm_enabled()) {
        return kvm_insert_breakpoint(gdbserver_state.c_cpu, addr, len, type);
    }

    switch (type) {
    case GDB_BREAKPOINT_SW:
    case GDB_BREAKPOINT_HW:
        CPU_FOREACH(cpu) {
            err = cpu_breakpoint_insert(cpu, addr, BP_GDB, NULL);
            if (err) {
                break;
            }
        }
        return err;
#ifndef CONFIG_USER_ONLY
    case GDB_WATCHPOINT_WRITE:
    case GDB_WATCHPOINT_READ:
    case GDB_WATCHPOINT_ACCESS:
        CPU_FOREACH(cpu) {
            err = cpu_watchpoint_insert(cpu, addr, len,
                                        xlat_gdb_type(cpu, type), NULL);




> ---
> RFC because we could keep that code by adding an empty
>     stub for cpu_restore_state(), but it is unclear as
>     the function is named generically.
> ---
>  include/hw/core/cpu.h | 4 ++--
>  softmmu/physmem.c     | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 140fa32a5e3..1b4af30db04 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -1033,7 +1033,7 @@ static inline bool cpu_breakpoint_test(CPUState *cpu, vaddr pc, int mask)
>      return false;
>  }
>  
> -#ifdef CONFIG_USER_ONLY
> +#if !defined(CONFIG_TCG) || defined(CONFIG_USER_ONLY)
>  static inline int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
>                                          int flags, CPUWatchpoint **watchpoint)
>  {
> @@ -1098,7 +1098,7 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
>   * If no watchpoint is registered for the range, the result is 0.
>   */
>  int cpu_watchpoint_address_matches(CPUState *cpu, vaddr addr, vaddr len);
> -#endif
> +#endif /* !CONFIG_TCG || CONFIG_USER_ONLY */
>  
>  /**
>   * cpu_get_address_space:
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 65602ed548e..5135a6371b5 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -765,6 +765,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
>      return cpu->cpu_ases[asidx].as;
>  }
>  
> +#ifdef CONFIG_TCG
>  /* Add a watchpoint.  */
>  int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
>                            int flags, CPUWatchpoint **watchpoint)
> @@ -873,6 +874,7 @@ int cpu_watchpoint_address_matches(CPUState *cpu, vaddr addr, vaddr len)
>      }
>      return ret;
>  }
> +#endif /* CONFIG_TCG */
>  
>  /* Called from RCU critical section */
>  static RAMBlock *qemu_get_ram_block(ram_addr_t addr)
> @@ -2356,6 +2358,7 @@ ram_addr_t qemu_ram_addr_from_host(void *ptr)
>      return block->offset + offset;
>  }
>  
> +#ifdef CONFIG_TCG
>  /* Generate a debug exception if a watchpoint has been hit.  */
>  void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
>                            MemTxAttrs attrs, int flags, uintptr_t ra)
> @@ -2424,6 +2427,7 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
>          }
>      }
>  }
> +#endif /* CONFIG_TCG */
>  
>  static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
>                                   MemTxAttrs attrs, void *buf, hwaddr len);
> 

Re: [RFC PATCH 6/6] softmmu: Restrict watchpoint handlers to TCG accelerator

[Philippe Mathieu-Daudé]

On 1/18/21 10:10 AM, Claudio Fontana wrote:
> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
>> Watchpoint funtions use cpu_restore_state() which is only
>> available when TCG accelerator is built. Restrict them
>> to TCG.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> I am doing some of this in my series, and I did not notice that
> cpu_watchpoint_insert was also TCG only.
> 
> Probably we should merge this somehow.
> 
> I thought it was used by gdbstub.c as well, passing flags BP_GDB .

BP_MEM_ACCESS for watchpoint.

> I noticed that gdbstub does something else entirely for kvm_enabled(), ie, kvm_insert_breakpoint,
> but what about the other accels, it seems that the code flows to the cpu_breakpoint_insert and watchpoint_insert..?
> 
> should cpu_breakpoint_insert have the same fate then?
> 
> And is this really all TCG specific?
> 
> From gdbstub.c:1020:
> 
> static int gdb_breakpoint_insert(int type, target_ulong addr, target_ulong len)
> {
>     CPUState *cpu;
>     int err = 0;
> 
>     if (kvm_enabled()) {
>         return kvm_insert_breakpoint(gdbserver_state.c_cpu, addr, len, type);

Doh I missed that. I remember Peter and Richard explained it once
but I forgot and couldn't find on the list, maybe it was on IRC.

See i.e. in target/arm/kvm64.c:

 312 int kvm_arch_insert_hw_breakpoint(target_ulong addr,
 313                                   target_ulong len, int type)
 314 {
 315     switch (type) {
 316     case GDB_BREAKPOINT_HW:
 317         return insert_hw_breakpoint(addr);
 318         break;
 319     case GDB_WATCHPOINT_READ:
 320     case GDB_WATCHPOINT_WRITE:
 321     case GDB_WATCHPOINT_ACCESS:
 322         return insert_hw_watchpoint(addr, len, type);
 323     default:
 324         return -ENOSYS;
 325     }
 326 }

> 
>> ---
>> RFC because we could keep that code by adding an empty
>>     stub for cpu_restore_state(), but it is unclear as
>>     the function is named generically.
>> ---
>>  include/hw/core/cpu.h | 4 ++--
>>  softmmu/physmem.c     | 4 ++++
>>  2 files changed, 6 insertions(+), 2 deletions(-)

Re: [RFC PATCH 6/6] softmmu: Restrict watchpoint handlers to TCG accelerator

[Claudio Fontana]

On 1/18/21 10:36 AM, Philippe Mathieu-Daudé wrote:
> On 1/18/21 10:10 AM, Claudio Fontana wrote:
>> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
>>> Watchpoint funtions use cpu_restore_state() which is only
>>> available when TCG accelerator is built. Restrict them
>>> to TCG.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> I am doing some of this in my series, and I did not notice that
>> cpu_watchpoint_insert was also TCG only.
>>
>> Probably we should merge this somehow.
>>
>> I thought it was used by gdbstub.c as well, passing flags BP_GDB .
> 
> BP_MEM_ACCESS for watchpoint.
> 
>> I noticed that gdbstub does something else entirely for kvm_enabled(), ie, kvm_insert_breakpoint,
>> but what about the other accels, it seems that the code flows to the cpu_breakpoint_insert and watchpoint_insert..?
>>
>> should cpu_breakpoint_insert have the same fate then?
>>
>> And is this really all TCG specific?
>>
>> From gdbstub.c:1020:
>>
>> static int gdb_breakpoint_insert(int type, target_ulong addr, target_ulong len)
>> {
>>     CPUState *cpu;
>>     int err = 0;
>>
>>     if (kvm_enabled()) {
>>         return kvm_insert_breakpoint(gdbserver_state.c_cpu, addr, len, type);
> 
> Doh I missed that. I remember Peter and Richard explained it once
> but I forgot and couldn't find on the list, maybe it was on IRC.
> 
> See i.e. in target/arm/kvm64.c:
> 
>  312 int kvm_arch_insert_hw_breakpoint(target_ulong addr,
>  313                                   target_ulong len, int type)
>  314 {
>  315     switch (type) {
>  316     case GDB_BREAKPOINT_HW:
>  317         return insert_hw_breakpoint(addr);
>  318         break;
>  319     case GDB_WATCHPOINT_READ:
>  320     case GDB_WATCHPOINT_WRITE:
>  321     case GDB_WATCHPOINT_ACCESS:
>  322         return insert_hw_watchpoint(addr, len, type);
>  323     default:
>  324         return -ENOSYS;
>  325     }
>  326 }
> 
>>
>>> ---
>>> RFC because we could keep that code by adding an empty
>>>     stub for cpu_restore_state(), but it is unclear as
>>>     the function is named generically.
>>> ---
>>>  include/hw/core/cpu.h | 4 ++--
>>>  softmmu/physmem.c     | 4 ++++
>>>  2 files changed, 6 insertions(+), 2 deletions(-)
> 

Hello Philippe,

I have reached this issue again when working on the ARM part of the cleanup,
did we reach a conclusion on whether cpu_watchpoint_insert is TCG-specific or not,

and more in general about breakpoints and watchpoints?

The way I encountered this issue now is during the KVM/TCG split in target/arm.

there are calls in target/arm/cpu.c and machine.c of the kind:

hw_breakpoint_update_all()
hw_watchpoint_update_all()

is this all TCG-specific,

including also hw_watchpoint_update(), hw_breakpoint_update() ?

kvm64.c seems to have its own handling of breakpoints, including arrays:

GArray *hw_breakpoints, *hw_watchpoints;

while the TCG stuff relies on cpu->watchpoints in softmmu/physmem.c:

$ gid watchpoints
include/hw/core/cpu.h:139: * address before attempting to match it against watchpoints.
include/hw/core/cpu.h:388:    QTAILQ_HEAD(, CPUWatchpoint) watchpoints;
linux-user/main.c:210:    /* Clone all break/watchpoints.
linux-user/main.c:212:       BP_CPU break/watchpoints are handled correctly on clone. */
linux-user/main.c:214:    QTAILQ_INIT(&new_cpu->watchpoints);
linux-user/main.c:218:    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
softmmu/physmem.c:791:    /* keep all GDB-injected watchpoints in front */
softmmu/physmem.c:793:        QTAILQ_INSERT_HEAD(&cpu->watchpoints, wp, entry);
softmmu/physmem.c:795:        QTAILQ_INSERT_TAIL(&cpu->watchpoints, wp, entry);
softmmu/physmem.c:816:    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
softmmu/physmem.c:829:    QTAILQ_REMOVE(&cpu->watchpoints, watchpoint, entry);
softmmu/physmem.c:836:/* Remove all matching watchpoints.  */
softmmu/physmem.c:841:    QTAILQ_FOREACH_SAFE(wp, &cpu->watchpoints, entry, next) {
softmmu/physmem.c:868:/* Return flags for watchpoints that match addr + prot.  */
softmmu/physmem.c:874:    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
softmmu/physmem.c:906:    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
softmmu/physmem.c:911:                 * Don't process the watchpoints when we are
accel/tcg/cpu-exec.c:511:        QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
accel/tcg/cpu-exec.c:822:               after-access watchpoints.  Since this request should never
hw/core/cpu.c:361:    QTAILQ_INIT(&cpu->watchpoints);


Even in i386 there is a bit of confusion still, and I think sorting out this could improve the code on i386 as well..

Thanks for any comment,

Ciao,

CLaudio

Re: [RFC PATCH 6/6] softmmu: Restrict watchpoint handlers to TCG accelerator

[Alex Bennée]

Claudio Fontana <cfontana@suse.de> writes:

> On 1/18/21 10:36 AM, Philippe Mathieu-Daudé wrote:
>> On 1/18/21 10:10 AM, Claudio Fontana wrote:
>>> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
>>>> Watchpoint funtions use cpu_restore_state() which is only
>>>> available when TCG accelerator is built. Restrict them
>>>> to TCG.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>
>>> I am doing some of this in my series, and I did not notice that
>>> cpu_watchpoint_insert was also TCG only.
>>>
>>> Probably we should merge this somehow.
>>>
>>> I thought it was used by gdbstub.c as well, passing flags BP_GDB .
>> 
>> BP_MEM_ACCESS for watchpoint.
>> 
>>> I noticed that gdbstub does something else entirely for kvm_enabled(), ie, kvm_insert_breakpoint,
>>> but what about the other accels, it seems that the code flows to the
>>cpu_breakpoint_insert and watchpoint_insert..?

For KVM (and I guess other accelerators) the kvm_insert_breakpoint is
really the entry point for all debug. SW breakpoints are dealt with
separately from HW breakpoints and watchpoints which involve more than
just planting code in the guests RAM. 

>>>
>>> should cpu_breakpoint_insert have the same fate then?
>>>
>>> And is this really all TCG specific?
>>>
>>> From gdbstub.c:1020:
>>>
>>> static int gdb_breakpoint_insert(int type, target_ulong addr, target_ulong len)
>>> {
>>>     CPUState *cpu;
>>>     int err = 0;
>>>
>>>     if (kvm_enabled()) {
>>>         return kvm_insert_breakpoint(gdbserver_state.c_cpu, addr, len, type);
>> 
>> Doh I missed that. I remember Peter and Richard explained it once
>> but I forgot and couldn't find on the list, maybe it was on IRC.
>> 
>> See i.e. in target/arm/kvm64.c:
>> 
>>  312 int kvm_arch_insert_hw_breakpoint(target_ulong addr,
>>  313                                   target_ulong len, int type)
>>  314 {
>>  315     switch (type) {
>>  316     case GDB_BREAKPOINT_HW:
>>  317         return insert_hw_breakpoint(addr);
>>  318         break;
>>  319     case GDB_WATCHPOINT_READ:
>>  320     case GDB_WATCHPOINT_WRITE:
>>  321     case GDB_WATCHPOINT_ACCESS:
>>  322         return insert_hw_watchpoint(addr, len, type);
>>  323     default:
>>  324         return -ENOSYS;
>>  325     }
>>  326 }
>> 
>>>
>>>> ---
>>>> RFC because we could keep that code by adding an empty
>>>>     stub for cpu_restore_state(), but it is unclear as
>>>>     the function is named generically.
>>>> ---
>>>>  include/hw/core/cpu.h | 4 ++--
>>>>  softmmu/physmem.c     | 4 ++++
>>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>> 
>
> Hello Philippe,
>
> I have reached this issue again when working on the ARM part of the cleanup,
> did we reach a conclusion on whether cpu_watchpoint_insert is TCG-specific or not,
>
> and more in general about breakpoints and watchpoints?
>
> The way I encountered this issue now is during the KVM/TCG split in target/arm.
>
> there are calls in target/arm/cpu.c and machine.c of the kind:
>
> hw_breakpoint_update_all()
> hw_watchpoint_update_all()

This is the third case, using the TCG's internal breakpoint/watchpoint
structures to simulate the hardwares HW breakpoint/watchpoint support
under emulation.

> is this all TCG-specific,
>
> including also hw_watchpoint_update(), hw_breakpoint_update() ?
>
> kvm64.c seems to have its own handling of breakpoints, including arrays:
>
> GArray *hw_breakpoints, *hw_watchpoints;

Correct. KVM and other HW accelerators are really just ensuring that
accounting is done in some arch specific way to ensure registers are
setup and traps properly interpreted.

>
> while the TCG stuff relies on cpu->watchpoints in softmmu/physmem.c:

Because we need core TCG support to detect cases for both gdbstub and
emulating real HW.

>
> $ gid watchpoints
> include/hw/core/cpu.h:139: * address before attempting to match it against watchpoints.
> include/hw/core/cpu.h:388:    QTAILQ_HEAD(, CPUWatchpoint) watchpoints;
> linux-user/main.c:210:    /* Clone all break/watchpoints.
> linux-user/main.c:212:       BP_CPU break/watchpoints are handled correctly on clone. */
> linux-user/main.c:214:    QTAILQ_INIT(&new_cpu->watchpoints);
> linux-user/main.c:218:    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
> softmmu/physmem.c:791:    /* keep all GDB-injected watchpoints in front */
> softmmu/physmem.c:793:        QTAILQ_INSERT_HEAD(&cpu->watchpoints, wp, entry);
> softmmu/physmem.c:795:        QTAILQ_INSERT_TAIL(&cpu->watchpoints, wp, entry);
> softmmu/physmem.c:816:    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
> softmmu/physmem.c:829:    QTAILQ_REMOVE(&cpu->watchpoints, watchpoint, entry);
> softmmu/physmem.c:836:/* Remove all matching watchpoints.  */
> softmmu/physmem.c:841:    QTAILQ_FOREACH_SAFE(wp, &cpu->watchpoints, entry, next) {
> softmmu/physmem.c:868:/* Return flags for watchpoints that match addr + prot.  */
> softmmu/physmem.c:874:    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
> softmmu/physmem.c:906:    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
> softmmu/physmem.c:911:                 * Don't process the watchpoints when we are
> accel/tcg/cpu-exec.c:511:        QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
> accel/tcg/cpu-exec.c:822:               after-access watchpoints.  Since this request should never
> hw/core/cpu.c:361:    QTAILQ_INIT(&cpu->watchpoints);

Also we need softmmu for watchpoints - because otherwise there is no way
to mark memory to trigger on access. One day we might solve this with
the oft talked about softmmu for user-mode combination.

> Even in i386 there is a bit of confusion still, and I think sorting out this could improve the code on i386 as well..
>
> Thanks for any comment,
>
> Ciao,
>
> CLaudio


-- 
Alex Bennée

Re: [RFC PATCH 6/6] softmmu: Restrict watchpoint handlers to TCG accelerator

[Richard Henderson]

On 1/17/21 6:48 AM, Philippe Mathieu-Daudé wrote:
> Watchpoint funtions use cpu_restore_state() which is only
> available when TCG accelerator is built. Restrict them
> to TCG.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> RFC because we could keep that code by adding an empty
>     stub for cpu_restore_state(), but it is unclear as
>     the function is named generically.
> ---
>  include/hw/core/cpu.h | 4 ++--
>  softmmu/physmem.c     | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)

And all of the targets that use this are already conditionalized for this?  As
opposed to leaving the declarations and adding stubs?


r~

Re: [PATCH 0/6] accel: Restrict TCG-specific code

[Claudio Fontana]

On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> I've prepared some patches to have KVM-only builds.
> Some patches are generic - well kind of, instead they are
> TCG specific =) - so I'm sending them as a separate series.
> 
> Please review,
> 
> Phil.

I am wondering the best way to combine with:

[PATCH v12 00/22] i386 cleanup PART 2
https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg02427.html

In particular:

[PATCH v12 12/22] physmem: make watchpoint checking code TCG-only
[PATCH v12 13/22] cpu: move adjust_watchpoint_address to tcg_ops
[PATCH v12 14/22] cpu: move debug_check_watchpoint to tcg_ops

Thanks!

Claudio


> 
> Philippe Mathieu-Daudé (6):
>   accel/tcg: Make cpu_gen_init() static
>   accel/tcg: Restrict tb_flush_jmp_cache() from other accelerators
>   accel/tcg: Restrict tb_gen_code() from other accelerators
>   accel/tcg: Declare missing cpu_loop_exit*() stubs
>   accel/tcg: Restrict cpu_io_recompile() from other accelerators
>   softmmu: Restrict watchpoint handlers to TCG accelerator
> 
>  accel/tcg/internal.h      | 23 +++++++++++++++++++++++
>  include/exec/exec-all.h   | 11 -----------
>  include/hw/core/cpu.h     |  4 ++--
>  accel/stubs/tcg-stub.c    | 10 ++++++++++
>  accel/tcg/cpu-exec.c      |  1 +
>  accel/tcg/cputlb.c        |  1 +
>  accel/tcg/translate-all.c |  3 ++-
>  accel/tcg/user-exec.c     |  1 +
>  softmmu/physmem.c         |  4 ++++
>  9 files changed, 44 insertions(+), 14 deletions(-)
>  create mode 100644 accel/tcg/internal.h
>