[PATCH v2 0/3] accel/tcg: Alternate fix for #1866

[PATCH v2 0/3] accel/tcg: Alternate fix for #1866

[Richard Henderson]

Supercedes: 20230914174436.1597356-1-richard.henderson@linaro.org
("[PATCH 0/6] accel/tcg: Always require can_do_io (#1866)")

An alternate fix for #1866 without touching can_do_io, and is
perhaps a bit cleaner and clearer.

Instead, the current cpu checks whether an address space update
is required on each entry to the slow path.  This certainly
catches all i/o.  It easily works for the sequential case of
two i/o, the first of which changes the address space and the
second of which requires the new address space.

I've done a tiny bit of performance testing between the two
solutions and it seems to be a wash.  So now it's simply a
matter of cleanliness.


r~


Richard Henderson (3):
  softmmu: Use cpu->created in tcg_commit
  softmmu: Introduce AddressSpaceDispatch generation numbers
  softmmu: Introduce cpu_address_space_sync

 include/exec/memory.h |  6 ++++++
 accel/tcg/cputlb.c    |  2 ++
 softmmu/physmem.c     | 46 ++++++++++++++++++++++++++++++++++++-------
 3 files changed, 47 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] softmmu: Use cpu->created in tcg_commit

[Richard Henderson]

Use created not halt_cond to test for cpu created.

Fixes: 0d58c660689f ("softmmu: Use async_run_on_cpu in tcg_commit")
Reported-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 softmmu/physmem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 18277ddd67..88fafec1da 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2515,9 +2515,9 @@ static void tcg_commit(MemoryListener *listener)
      * the memory data structures.
      *
      * That said, the listener is also called during realize, before
-     * all of the tcg machinery for run-on is initialized: thus halt_cond.
+     * all of the tcg machinery for run-on is initialized: thus created.
      */
-    if (cpu->halt_cond) {
+    if (cpu->created) {
         async_run_on_cpu(cpu, tcg_commit_cpu, RUN_ON_CPU_HOST_PTR(cpuas));
     } else {
         tcg_commit_cpu(cpu, RUN_ON_CPU_HOST_PTR(cpuas));
-- 
2.34.1

Re: [PATCH v2 1/3] softmmu: Use cpu->created in tcg_commit

[Philippe Mathieu-Daudé]

On 15/9/23 18:32, Richard Henderson wrote:
> Use created not halt_cond to test for cpu created.
> 
> Fixes: 0d58c660689f ("softmmu: Use async_run_on_cpu in tcg_commit")
> Reported-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   softmmu/physmem.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v2 2/3] softmmu: Introduce AddressSpaceDispatch generation numbers

[Richard Henderson]

Allow the cpu to easily check if an address space update
is required.  The acquire/release semantics will be used
by a subsequent patch.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 softmmu/physmem.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 88fafec1da..e1c535380a 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -163,6 +163,8 @@ struct CPUAddressSpace {
     AddressSpace *as;
     struct AddressSpaceDispatch *memory_dispatch;
     MemoryListener tcg_as_listener;
+    uint32_t commit_gen;
+    uint32_t layout_gen;
 };
 
 struct DirtyBitmapSnapshot {
@@ -2486,14 +2488,23 @@ static void tcg_log_global_after_sync(MemoryListener *listener)
     }
 }
 
-static void tcg_commit_cpu(CPUState *cpu, run_on_cpu_data data)
+static void tcg_commit_cpu_0(CPUState *cpu, CPUAddressSpace *cpuas)
 {
-    CPUAddressSpace *cpuas = data.host_ptr;
-
     cpuas->memory_dispatch = address_space_to_dispatch(cpuas->as);
     tlb_flush(cpu);
 }
 
+static void tcg_commit_cpu_1(CPUState *cpu, run_on_cpu_data data)
+{
+    CPUAddressSpace *cpuas = data.host_ptr;
+    uint32_t gen = qatomic_load_acquire(&cpuas->layout_gen);
+
+    if (cpuas->commit_gen != gen) {
+        cpuas->commit_gen = gen;
+        tcg_commit_cpu_0(cpu, cpuas);
+    }
+}
+
 static void tcg_commit(MemoryListener *listener)
 {
     CPUAddressSpace *cpuas;
@@ -2518,9 +2529,10 @@ static void tcg_commit(MemoryListener *listener)
      * all of the tcg machinery for run-on is initialized: thus created.
      */
     if (cpu->created) {
-        async_run_on_cpu(cpu, tcg_commit_cpu, RUN_ON_CPU_HOST_PTR(cpuas));
+        qatomic_store_release(&cpuas->layout_gen, cpuas->layout_gen + 1);
+        async_run_on_cpu(cpu, tcg_commit_cpu_1, RUN_ON_CPU_HOST_PTR(cpuas));
     } else {
-        tcg_commit_cpu(cpu, RUN_ON_CPU_HOST_PTR(cpuas));
+        tcg_commit_cpu_0(cpu, cpuas);
     }
 }
 
-- 
2.34.1

[PATCH v2 3/3] softmmu: Introduce cpu_address_space_sync

[Richard Henderson]

Synchronously check and update the address space for the
current cpu for any slow path access.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1866
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/memory.h |  6 ++++++
 accel/tcg/cputlb.c    |  2 ++
 softmmu/physmem.c     | 20 ++++++++++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 68284428f8..7ec842076f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2780,6 +2780,12 @@ void address_space_cache_destroy(MemoryRegionCache *cache);
 IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
                                             bool is_write, MemTxAttrs attrs);
 
+/*
+ * Ensure all cpu address spaces are up-to-date.
+ * Return true if changes made and tlb flushed.
+ */
+void cpu_address_space_sync(CPUState *cpu);
+
 /* address_space_translate: translate an address range into an address space
  * into a MemoryRegion and an address range into that section.  Should be
  * called from an RCU critical section, to avoid that the last reference
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 3270f65c20..91be3f3064 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1827,6 +1827,8 @@ static bool mmu_lookup(CPUArchState *env, vaddr addr, MemOpIdx oi,
     l->page[1].size = 0;
     crosspage = (addr ^ l->page[1].addr) & TARGET_PAGE_MASK;
 
+    cpu_address_space_sync(env_cpu(env));
+
     if (likely(!crosspage)) {
         mmu_lookup1(env, &l->page[0], l->mmu_idx, type, ra);
 
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index e1c535380a..5a89caa257 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2536,6 +2536,26 @@ static void tcg_commit(MemoryListener *listener)
     }
 }
 
+void cpu_address_space_sync(CPUState *cpu)
+{
+    int i, n = cpu->num_ases;
+    bool need_flush = false;
+
+    for (i = 0; i < n; ++i) {
+        CPUAddressSpace *cpuas = &cpu->cpu_ases[i];
+        uint32_t gen = qatomic_load_acquire(&cpuas->layout_gen);
+
+        if (cpuas->commit_gen != gen) {
+            cpuas->commit_gen = gen;
+            cpuas->memory_dispatch = address_space_to_dispatch(cpuas->as);
+            need_flush = true;
+        }
+    }
+    if (need_flush) {
+        tlb_flush(cpu);
+    }
+}
+
 static void memory_map_init(void)
 {
     system_memory = g_malloc(sizeof(*system_memory));
-- 
2.34.1

Re: [PATCH v2 3/3] softmmu: Introduce cpu_address_space_sync

[Philippe Mathieu-Daudé]

On 15/9/23 18:32, Richard Henderson wrote:
> Synchronously check and update the address space for the
> current cpu for any slow path access.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1866
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/exec/memory.h |  6 ++++++
>   accel/tcg/cputlb.c    |  2 ++
>   softmmu/physmem.c     | 20 ++++++++++++++++++++
>   3 files changed, 28 insertions(+)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 68284428f8..7ec842076f 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2780,6 +2780,12 @@ void address_space_cache_destroy(MemoryRegionCache *cache);
>   IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
>                                               bool is_write, MemTxAttrs attrs);
>   
> +/*
> + * Ensure all cpu address spaces are up-to-date.
> + * Return true if changes made and tlb flushed.

No return value, stale comment?

> + */
> +void cpu_address_space_sync(CPUState *cpu);
> +
>   /* address_space_translate: translate an address range into an address space
>    * into a MemoryRegion and an address range into that section.  Should be
>    * called from an RCU critical section, to avoid that the last reference
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 3270f65c20..91be3f3064 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1827,6 +1827,8 @@ static bool mmu_lookup(CPUArchState *env, vaddr addr, MemOpIdx oi,
>       l->page[1].size = 0;
>       crosspage = (addr ^ l->page[1].addr) & TARGET_PAGE_MASK;
>   
> +    cpu_address_space_sync(env_cpu(env));
> +
>       if (likely(!crosspage)) {
>           mmu_lookup1(env, &l->page[0], l->mmu_idx, type, ra);
>   
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index e1c535380a..5a89caa257 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -2536,6 +2536,26 @@ static void tcg_commit(MemoryListener *listener)
>       }
>   }
>   
> +void cpu_address_space_sync(CPUState *cpu)
> +{
> +    int i, n = cpu->num_ases;
> +    bool need_flush = false;
> +
> +    for (i = 0; i < n; ++i) {
> +        CPUAddressSpace *cpuas = &cpu->cpu_ases[i];
> +        uint32_t gen = qatomic_load_acquire(&cpuas->layout_gen);
> +
> +        if (cpuas->commit_gen != gen) {
> +            cpuas->commit_gen = gen;
> +            cpuas->memory_dispatch = address_space_to_dispatch(cpuas->as);
> +            need_flush = true;
> +        }
> +    }
> +    if (need_flush) {
> +        tlb_flush(cpu);
> +    }
> +}
> +
>   static void memory_map_init(void)
>   {
>       system_memory = g_malloc(sizeof(*system_memory));

Re: [PATCH v2 3/3] softmmu: Introduce cpu_address_space_sync

[Richard Henderson]

On 9/15/23 09:57, Philippe Mathieu-Daudé wrote:
> On 15/9/23 18:32, Richard Henderson wrote:
>> Synchronously check and update the address space for the
>> current cpu for any slow path access.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1866
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   include/exec/memory.h |  6 ++++++
>>   accel/tcg/cputlb.c    |  2 ++
>>   softmmu/physmem.c     | 20 ++++++++++++++++++++
>>   3 files changed, 28 insertions(+)
>>
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index 68284428f8..7ec842076f 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -2780,6 +2780,12 @@ void address_space_cache_destroy(MemoryRegionCache *cache);
>>   IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
>>                                               bool is_write, MemTxAttrs attrs);
>> +/*
>> + * Ensure all cpu address spaces are up-to-date.
>> + * Return true if changes made and tlb flushed.
> 
> No return value, stale comment?

Whoops, yes.

r~

Re: [PATCH v2 0/3] accel/tcg: Alternate fix for #1866

[Philippe Mathieu-Daudé]

On 15/9/23 18:32, Richard Henderson wrote:
> Supercedes: 20230914174436.1597356-1-richard.henderson@linaro.org
> ("[PATCH 0/6] accel/tcg: Always require can_do_io (#1866)")

Patches 1-4 (5?) are cleanups although, isn't it?

> An alternate fix for #1866 without touching can_do_io, and is
> perhaps a bit cleaner and clearer.
> 
> Instead, the current cpu checks whether an address space update
> is required on each entry to the slow path.  This certainly
> catches all i/o.  It easily works for the sequential case of
> two i/o, the first of which changes the address space and the
> second of which requires the new address space.
> 
> I've done a tiny bit of performance testing between the two
> solutions and it seems to be a wash.  So now it's simply a
> matter of cleanliness.
> 
> 
> r~
> 
> 
> Richard Henderson (3):
>    softmmu: Use cpu->created in tcg_commit
>    softmmu: Introduce AddressSpaceDispatch generation numbers
>    softmmu: Introduce cpu_address_space_sync
> 
>   include/exec/memory.h |  6 ++++++
>   accel/tcg/cputlb.c    |  2 ++
>   softmmu/physmem.c     | 46 ++++++++++++++++++++++++++++++++++++-------
>   3 files changed, 47 insertions(+), 7 deletions(-)
> 

Re: [PATCH v2 0/3] accel/tcg: Alternate fix for #1866

[Richard Henderson]

On 9/15/23 09:55, Philippe Mathieu-Daudé wrote:
> On 15/9/23 18:32, Richard Henderson wrote:
>> Supercedes: 20230914174436.1597356-1-richard.henderson@linaro.org
>> ("[PATCH 0/6] accel/tcg: Always require can_do_io (#1866)")
> 
> Patches 1-4 (5?) are cleanups although, isn't it?

Yes, 1-4 are good cleanups, and 5 is probably a bug fix
(though I don't see the hang without patch 6), despite
the fact that the hang is *with* icount (replay tests).


r~

Re: [PATCH v2 0/3] accel/tcg: Alternate fix for #1866

[Richard Henderson]

On 9/15/23 09:32, Richard Henderson wrote:
> Supercedes: 20230914174436.1597356-1-richard.henderson@linaro.org
> ("[PATCH 0/6] accel/tcg: Always require can_do_io (#1866)")
> 
> An alternate fix for #1866 without touching can_do_io, and is
> perhaps a bit cleaner and clearer.
> 
> Instead, the current cpu checks whether an address space update
> is required on each entry to the slow path.  This certainly
> catches all i/o.  It easily works for the sequential case of
> two i/o, the first of which changes the address space and the
> second of which requires the new address space.
> 
> I've done a tiny bit of performance testing between the two
> solutions and it seems to be a wash.  So now it's simply a
> matter of cleanliness.

Ho hum, this doesn't fix the x86_64 ovmf issue also quoted in #1866.


r~