[PATCH 0/4] accel/tcg: Stubs cleanups

[PATCH 0/4] accel/tcg: Stubs cleanups

[Philippe Mathieu-Daudé]

- Remove unused stubs,
- Guard with tcg_enabled() and remove more,
- Reduce tlb_set_dirty() scope?

Philippe Mathieu-Daudé (4):
  accel/tcg: Remove tlb_set_dirty() stub
  accel/tcg: Remove unused tcg_flush_jmp_cache() stub
  accel/tcg: Guard tb_flush() with tcg_enabled() and remove the stub
  accel/tcg: Keep tlb_set_dirty() internal

 include/exec/exec-all.h |  1 -
 accel/stubs/tcg-stub.c  | 12 ------------
 accel/tcg/cputlb.c      |  2 +-
 cpu.c                   | 15 +++++++++------
 gdbstub/softmmu.c       |  5 ++++-
 hw/ppc/spapr_hcall.c    |  2 +-
 6 files changed, 15 insertions(+), 22 deletions(-)

-- 
2.41.0

[PATCH 1/4] accel/tcg: Remove tlb_set_dirty() stub

[Philippe Mathieu-Daudé]

Since commit 34d49937e4 ("accel/tcg: Handle atomic accesses
to notdirty memory correctly") there is only a single call
to tlb_set_dirty(), within accel/tcg/cputlb.c itself where
the function is defined.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/stubs/tcg-stub.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c
index a9e7a2d5b4..f088054f34 100644
--- a/accel/stubs/tcg-stub.c
+++ b/accel/stubs/tcg-stub.c
@@ -18,10 +18,6 @@ void tb_flush(CPUState *cpu)
 {
 }
 
-void tlb_set_dirty(CPUState *cpu, vaddr vaddr)
-{
-}
-
 void tcg_flush_jmp_cache(CPUState *cpu)
 {
 }
-- 
2.41.0

[PATCH 2/4] accel/tcg: Remove unused tcg_flush_jmp_cache() stub

[Philippe Mathieu-Daudé]

Since commit ba7d3d1858 ("cpu_common_reset: wrap TCG
specific code in tcg_enabled()") we protect the single call
to tcg_flush_jmp_cache() with a check on tcg_enabled(). The
stub isn't needed anymore.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/stubs/tcg-stub.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c
index f088054f34..dd890d6cf6 100644
--- a/accel/stubs/tcg-stub.c
+++ b/accel/stubs/tcg-stub.c
@@ -18,10 +18,6 @@ void tb_flush(CPUState *cpu)
 {
 }
 
-void tcg_flush_jmp_cache(CPUState *cpu)
-{
-}
-
 int probe_access_flags(CPUArchState *env, vaddr addr, int size,
                        MMUAccessType access_type, int mmu_idx,
                        bool nonfault, void **phost, uintptr_t retaddr)
-- 
2.41.0

[RFC PATCH 3/4] accel/tcg: Guard tb_flush() with tcg_enabled() and remove the stub

[Philippe Mathieu-Daudé]

The check on tcg_enabled() make it clearer we want
this call under TCG.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/stubs/tcg-stub.c |  4 ----
 cpu.c                  | 15 +++++++++------
 gdbstub/softmmu.c      |  5 ++++-
 hw/ppc/spapr_hcall.c   |  2 +-
 4 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c
index dd890d6cf6..7d9846f7f2 100644
--- a/accel/stubs/tcg-stub.c
+++ b/accel/stubs/tcg-stub.c
@@ -14,10 +14,6 @@
 #include "exec/tb-flush.h"
 #include "exec/exec-all.h"
 
-void tb_flush(CPUState *cpu)
-{
-}
-
 int probe_access_flags(CPUArchState *env, vaddr addr, int size,
                        MMUAccessType access_type, int mmu_idx,
                        bool nonfault, void **phost, uintptr_t retaddr)
diff --git a/cpu.c b/cpu.c
index 0769b0b153..ce3b04f21f 100644
--- a/cpu.c
+++ b/cpu.c
@@ -57,12 +57,15 @@ static int cpu_common_post_load(void *opaque, int version_id)
     cpu->interrupt_request &= ~0x01;
     tlb_flush(cpu);
 
-    /* loadvm has just updated the content of RAM, bypassing the
-     * usual mechanisms that ensure we flush TBs for writes to
-     * memory we've translated code from. So we must flush all TBs,
-     * which will now be stale.
-     */
-    tb_flush(cpu);
+    if (tcg_enabled()) {
+        /*
+         * loadvm has just updated the content of RAM, bypassing the
+         * usual mechanisms that ensure we flush TBs for writes to
+         * memory we've translated code from. So we must flush all TBs,
+         * which will now be stale.
+         */
+        tb_flush(cpu);
+    }
 
     return 0;
 }
diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
index 9f0b8b5497..edd13f047d 100644
--- a/gdbstub/softmmu.c
+++ b/gdbstub/softmmu.c
@@ -21,6 +21,7 @@
 #include "sysemu/cpus.h"
 #include "sysemu/runstate.h"
 #include "sysemu/replay.h"
+#include "sysemu/tcg.h"
 #include "hw/core/cpu.h"
 #include "hw/cpu/cluster.h"
 #include "hw/boards.h"
@@ -170,7 +171,9 @@ static void gdb_vm_state_change(void *opaque, bool running, RunState state)
         } else {
             trace_gdbstub_hit_break();
         }
-        tb_flush(cpu);
+        if (tcg_enabled()) {
+            tb_flush(cpu);
+        }
         ret = GDB_SIGNAL_TRAP;
         break;
     case RUN_STATE_PAUSED:
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index b7dc388f2f..306f8fdf55 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -296,7 +296,7 @@ static target_ulong h_page_init(PowerPCCPU *cpu, SpaprMachineState *spapr,
     if (flags & (H_ICACHE_SYNCHRONIZE | H_ICACHE_INVALIDATE)) {
         if (kvm_enabled()) {
             kvmppc_icbi_range(cpu, pdst, len);
-        } else {
+        } else if (tcg_enabled()) {
             tb_flush(CPU(cpu));
         }
     }
-- 
2.41.0

[RFC PATCH 4/4] accel/tcg: Keep tlb_set_dirty() internal

[Philippe Mathieu-Daudé]

Since commit 34d49937e4 ("accel/tcg: Handle atomic accesses
to notdirty memory correctly") tlb_set_dirty() is only used
(once) in the very same file it is defined... Make it static.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/exec-all.h | 1 -
 accel/tcg/cputlb.c      | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index b2f5cd4c2a..59efa7bc28 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -695,7 +695,6 @@ static inline void mmap_unlock(void) {}
 #define WITH_MMAP_LOCK_GUARD()
 
 void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length);
-void tlb_set_dirty(CPUState *cpu, vaddr addr);
 
 MemoryRegionSection *
 address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index c643d66190..fe9d702f3e 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1062,7 +1062,7 @@ static inline void tlb_set_dirty1_locked(CPUTLBEntry *tlb_entry,
 
 /* update the TLB corresponding to virtual page vaddr
    so that it is no longer dirty */
-void tlb_set_dirty(CPUState *cpu, vaddr addr)
+static void tlb_set_dirty(CPUState *cpu, vaddr addr)
 {
     CPUArchState *env = cpu->env_ptr;
     int mmu_idx;
-- 
2.41.0

Re: [PATCH 1/4] accel/tcg: Remove tlb_set_dirty() stub

[Harsh Prateek Bora]

On 9/15/23 01:22, Philippe Mathieu-Daudé wrote:
> Since commit 34d49937e4 ("accel/tcg: Handle atomic accesses
> to notdirty memory correctly") there is only a single call
> to tlb_set_dirty(), within accel/tcg/cputlb.c itself where
> the function is defined.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> ---
>   accel/stubs/tcg-stub.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c
> index a9e7a2d5b4..f088054f34 100644
> --- a/accel/stubs/tcg-stub.c
> +++ b/accel/stubs/tcg-stub.c
> @@ -18,10 +18,6 @@ void tb_flush(CPUState *cpu)
>   {
>   }
>   
> -void tlb_set_dirty(CPUState *cpu, vaddr vaddr)
> -{
> -}
> -
>   void tcg_flush_jmp_cache(CPUState *cpu)
>   {
>   }

Re: [PATCH 2/4] accel/tcg: Remove unused tcg_flush_jmp_cache() stub

[Harsh Prateek Bora]

On 9/15/23 01:22, Philippe Mathieu-Daudé wrote:
> Since commit ba7d3d1858 ("cpu_common_reset: wrap TCG
> specific code in tcg_enabled()") we protect the single call
> to tcg_flush_jmp_cache() with a check on tcg_enabled(). The
> stub isn't needed anymore.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Commit log might need rephrase as I see multiple instance of call,
with one without check for tcg_enabled() in plugin_cpu_update__async but 
seems to be building only with tcg enabled so no build breaks.

Thanks
Harsh
> ---
>   accel/stubs/tcg-stub.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c
> index f088054f34..dd890d6cf6 100644
> --- a/accel/stubs/tcg-stub.c
> +++ b/accel/stubs/tcg-stub.c
> @@ -18,10 +18,6 @@ void tb_flush(CPUState *cpu)
>   {
>   }
>   
> -void tcg_flush_jmp_cache(CPUState *cpu)
> -{
> -}
> -
>   int probe_access_flags(CPUArchState *env, vaddr addr, int size,
>                          MMUAccessType access_type, int mmu_idx,
>                          bool nonfault, void **phost, uintptr_t retaddr)

Re: [RFC PATCH 3/4] accel/tcg: Guard tb_flush() with tcg_enabled() and remove the stub

[Harsh Prateek Bora]

On 9/15/23 01:22, Philippe Mathieu-Daudé wrote:
> The check on tcg_enabled() make it clearer we want
> this call under TCG.
> 

tb_flush already has a check for tcg_enabled() in its definition.
Do we really need to check for same before calling it?

Thanks
Harsh

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   accel/stubs/tcg-stub.c |  4 ----
>   cpu.c                  | 15 +++++++++------
>   gdbstub/softmmu.c      |  5 ++++-
>   hw/ppc/spapr_hcall.c   |  2 +-
>   4 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c
> index dd890d6cf6..7d9846f7f2 100644
> --- a/accel/stubs/tcg-stub.c
> +++ b/accel/stubs/tcg-stub.c
> @@ -14,10 +14,6 @@
>   #include "exec/tb-flush.h"
>   #include "exec/exec-all.h"
>   
> -void tb_flush(CPUState *cpu)
> -{
> -}
> -
>   int probe_access_flags(CPUArchState *env, vaddr addr, int size,
>                          MMUAccessType access_type, int mmu_idx,
>                          bool nonfault, void **phost, uintptr_t retaddr)
> diff --git a/cpu.c b/cpu.c
> index 0769b0b153..ce3b04f21f 100644
> --- a/cpu.c
> +++ b/cpu.c
> @@ -57,12 +57,15 @@ static int cpu_common_post_load(void *opaque, int version_id)
>       cpu->interrupt_request &= ~0x01;
>       tlb_flush(cpu);
>   
> -    /* loadvm has just updated the content of RAM, bypassing the
> -     * usual mechanisms that ensure we flush TBs for writes to
> -     * memory we've translated code from. So we must flush all TBs,
> -     * which will now be stale.
> -     */
> -    tb_flush(cpu);
> +    if (tcg_enabled()) {
> +        /*
> +         * loadvm has just updated the content of RAM, bypassing the
> +         * usual mechanisms that ensure we flush TBs for writes to
> +         * memory we've translated code from. So we must flush all TBs,
> +         * which will now be stale.
> +         */
> +        tb_flush(cpu);
> +    }
>   
>       return 0;
>   }
> diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
> index 9f0b8b5497..edd13f047d 100644
> --- a/gdbstub/softmmu.c
> +++ b/gdbstub/softmmu.c
> @@ -21,6 +21,7 @@
>   #include "sysemu/cpus.h"
>   #include "sysemu/runstate.h"
>   #include "sysemu/replay.h"
> +#include "sysemu/tcg.h"
>   #include "hw/core/cpu.h"
>   #include "hw/cpu/cluster.h"
>   #include "hw/boards.h"
> @@ -170,7 +171,9 @@ static void gdb_vm_state_change(void *opaque, bool running, RunState state)
>           } else {
>               trace_gdbstub_hit_break();
>           }
> -        tb_flush(cpu);
> +        if (tcg_enabled()) {
> +            tb_flush(cpu);
> +        }
>           ret = GDB_SIGNAL_TRAP;
>           break;
>       case RUN_STATE_PAUSED:
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index b7dc388f2f..306f8fdf55 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -296,7 +296,7 @@ static target_ulong h_page_init(PowerPCCPU *cpu, SpaprMachineState *spapr,
>       if (flags & (H_ICACHE_SYNCHRONIZE | H_ICACHE_INVALIDATE)) {
>           if (kvm_enabled()) {
>               kvmppc_icbi_range(cpu, pdst, len);
> -        } else {
> +        } else if (tcg_enabled()) {
>               tb_flush(CPU(cpu));
>           }
>       }

Re: [RFC PATCH 4/4] accel/tcg: Keep tlb_set_dirty() internal

[Harsh Prateek Bora]

On 9/15/23 01:22, Philippe Mathieu-Daudé wrote:
> Since commit 34d49937e4 ("accel/tcg: Handle atomic accesses
> to notdirty memory correctly") tlb_set_dirty() is only used
> (once) in the very same file it is defined... Make it static.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> ---
>   include/exec/exec-all.h | 1 -
>   accel/tcg/cputlb.c      | 2 +-
>   2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index b2f5cd4c2a..59efa7bc28 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -695,7 +695,6 @@ static inline void mmap_unlock(void) {}
>   #define WITH_MMAP_LOCK_GUARD()
>   
>   void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length);
> -void tlb_set_dirty(CPUState *cpu, vaddr addr);
>   
>   MemoryRegionSection *
>   address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index c643d66190..fe9d702f3e 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1062,7 +1062,7 @@ static inline void tlb_set_dirty1_locked(CPUTLBEntry *tlb_entry,
>   
>   /* update the TLB corresponding to virtual page vaddr
>      so that it is no longer dirty */
> -void tlb_set_dirty(CPUState *cpu, vaddr addr)
> +static void tlb_set_dirty(CPUState *cpu, vaddr addr)
>   {
>       CPUArchState *env = cpu->env_ptr;
>       int mmu_idx;

Re: [PATCH 2/4] accel/tcg: Remove unused tcg_flush_jmp_cache() stub

[Philippe Mathieu-Daudé]

On 15/9/23 17:12, Harsh Prateek Bora wrote:
> 
> 
> On 9/15/23 01:22, Philippe Mathieu-Daudé wrote:
>> Since commit ba7d3d1858 ("cpu_common_reset: wrap TCG
>> specific code in tcg_enabled()") we protect the single call
>> to tcg_flush_jmp_cache() with a check on tcg_enabled(). The
>> stub isn't needed anymore.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Commit log might need rephrase as I see multiple instance of call,
> with one without check for tcg_enabled() in plugin_cpu_update__async but 
> seems to be building only with tcg enabled so no build breaks.

TCG Plugins are restricted to TCG, so we don't need to guard
there :) I'll update the description mentioning it.

Thanks for your review!

> Thanks
> Harsh
>> ---
>>   accel/stubs/tcg-stub.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c
>> index f088054f34..dd890d6cf6 100644
>> --- a/accel/stubs/tcg-stub.c
>> +++ b/accel/stubs/tcg-stub.c
>> @@ -18,10 +18,6 @@ void tb_flush(CPUState *cpu)
>>   {
>>   }
>> -void tcg_flush_jmp_cache(CPUState *cpu)
>> -{
>> -}
>> -
>>   int probe_access_flags(CPUArchState *env, vaddr addr, int size,
>>                          MMUAccessType access_type, int mmu_idx,
>>                          bool nonfault, void **phost, uintptr_t retaddr)

Re: [RFC PATCH 3/4] accel/tcg: Guard tb_flush() with tcg_enabled() and remove the stub

[Philippe Mathieu-Daudé]

On 15/9/23 17:25, Harsh Prateek Bora wrote:
> 
> 
> On 9/15/23 01:22, Philippe Mathieu-Daudé wrote:
>> The check on tcg_enabled() make it clearer we want
>> this call under TCG.
>>
> 
> tb_flush already has a check for tcg_enabled() in its definition.
> Do we really need to check for same before calling it?

Good point, I didn't notice. I'll replace the call in
tb_flush() by an assertion.

> 
> Thanks
> Harsh
> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   accel/stubs/tcg-stub.c |  4 ----
>>   cpu.c                  | 15 +++++++++------
>>   gdbstub/softmmu.c      |  5 ++++-
>>   hw/ppc/spapr_hcall.c   |  2 +-
>>   4 files changed, 14 insertions(+), 12 deletions(-)

Re: [RFC PATCH 3/4] accel/tcg: Guard tb_flush() with tcg_enabled() and remove the stub

[Harsh Prateek Bora]

On 9/15/23 21:12, Philippe Mathieu-Daudé wrote:
> On 15/9/23 17:25, Harsh Prateek Bora wrote:
>>
>>
>> On 9/15/23 01:22, Philippe Mathieu-Daudé wrote:
>>> The check on tcg_enabled() make it clearer we want
>>> this call under TCG.
>>>
>>
>> tb_flush already has a check for tcg_enabled() in its definition.
>> Do we really need to check for same before calling it?
> 
> Good point, I didn't notice. I'll replace the call in
> tb_flush() by an assertion.

I guess you meant asserting in else case of the check inside ?
We may want to have the check internally only than having every caller 
to do that.

> 
>>
>> Thanks
>> Harsh
>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   accel/stubs/tcg-stub.c |  4 ----
>>>   cpu.c                  | 15 +++++++++------
>>>   gdbstub/softmmu.c      |  5 ++++-
>>>   hw/ppc/spapr_hcall.c   |  2 +-
>>>   4 files changed, 14 insertions(+), 12 deletions(-)
> 
>