[PATCH] accel/tcg: test CPUJumpCache in tb_jmp_cache_clear_page()

[PATCH] accel/tcg: test CPUJumpCache in tb_jmp_cache_clear_page()

[Eric Auger]

After commit 4e4fa6c12d ("accel/tcg: Complete cpu initialization
before registration"), it looks the CPUJumpCache handle can be NULL.
This causes a SIGSEV when running debug-wp-migration kvm unit test.

At the first place it should be clarified why this TCG code is called
with KVM acceleration. This may hide another bug.

Fixes: 4e4fa6c12d ("accel/tcg: Complete cpu initialization before registration")
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 accel/tcg/cputlb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 4e040a1cb9..ac0245ee6c 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -103,6 +103,10 @@ static void tb_jmp_cache_clear_page(CPUState *cpu, target_ulong page_addr)
     int i, i0 = tb_jmp_cache_hash_page(page_addr);
     CPUJumpCache *jc = cpu->tb_jmp_cache;
 
+    if (!jc) {
+        return;
+    }
+
     for (i = 0; i < TB_JMP_PAGE_SIZE; i++) {
         qatomic_set(&jc->array[i0 + i].tb, NULL);
     }
-- 
2.37.3

Re: [PATCH] accel/tcg: test CPUJumpCache in tb_jmp_cache_clear_page()

[Philippe Mathieu-Daudé]

On 3/2/23 18:15, Eric Auger wrote:
> After commit 4e4fa6c12d ("accel/tcg: Complete cpu initialization
> before registration"), it looks the CPUJumpCache handle can be NULL.
> This causes a SIGSEV when running debug-wp-migration kvm unit test.

Do you mean commit a976a99a29 ("include/hw/core: Create struct
CPUJumpCache") instead?

> At the first place it should be clarified why this TCG code is called
> with KVM acceleration. This may hide another bug.
> 
> Fixes: 4e4fa6c12d ("accel/tcg: Complete cpu initialization before registration")
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>   accel/tcg/cputlb.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 4e040a1cb9..ac0245ee6c 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -103,6 +103,10 @@ static void tb_jmp_cache_clear_page(CPUState *cpu, target_ulong page_addr)
>       int i, i0 = tb_jmp_cache_hash_page(page_addr);
>       CPUJumpCache *jc = cpu->tb_jmp_cache;
>   
> +    if (!jc) {

unlikely()?

> +        return;
> +    }
> +
>       for (i = 0; i < TB_JMP_PAGE_SIZE; i++) {
>           qatomic_set(&jc->array[i0 + i].tb, NULL);
>       }

Re: [PATCH] accel/tcg: test CPUJumpCache in tb_jmp_cache_clear_page()

[Eric Auger]

Hi Philippe,
On 2/3/23 18:21, Philippe Mathieu-Daudé wrote:
> On 3/2/23 18:15, Eric Auger wrote:
>> After commit 4e4fa6c12d ("accel/tcg: Complete cpu initialization
>> before registration"), it looks the CPUJumpCache handle can be NULL.
>> This causes a SIGSEV when running debug-wp-migration kvm unit test.
>
> Do you mean commit a976a99a29 ("include/hw/core: Create struct
> CPUJumpCache") instead?
No as reported in
https://lore.kernel.org/all/d91ccc02-a963-946d-169a-fd4da2610861@redhat.com/
my bisection pointed to 4e4fa6c12d ("accel/tcg: Complete cpu initialization
before registration")
>
>> At the first place it should be clarified why this TCG code is called
>> with KVM acceleration. This may hide another bug.
>>
>> Fixes: 4e4fa6c12d ("accel/tcg: Complete cpu initialization before
>> registration")
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>   accel/tcg/cputlb.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
>> index 4e040a1cb9..ac0245ee6c 100644
>> --- a/accel/tcg/cputlb.c
>> +++ b/accel/tcg/cputlb.c
>> @@ -103,6 +103,10 @@ static void tb_jmp_cache_clear_page(CPUState
>> *cpu, target_ulong page_addr)
>>       int i, i0 = tb_jmp_cache_hash_page(page_addr);
>>       CPUJumpCache *jc = cpu->tb_jmp_cache;
>>   +    if (!jc) {
>
> unlikely()?
yes maybe

Thanks

Eric
>
>> +        return;
>> +    }
>> +
>>       for (i = 0; i < TB_JMP_PAGE_SIZE; i++) {
>>           qatomic_set(&jc->array[i0 + i].tb, NULL);
>>       }
>

Re: [PATCH] accel/tcg: test CPUJumpCache in tb_jmp_cache_clear_page()

[Richard Henderson]

On 2/3/23 07:15, Eric Auger wrote:
> After commit 4e4fa6c12d ("accel/tcg: Complete cpu initialization
> before registration"), it looks the CPUJumpCache handle can be NULL.
> This causes a SIGSEV when running debug-wp-migration kvm unit test.
> 
> At the first place it should be clarified why this TCG code is called
> with KVM acceleration. This may hide another bug.
> 
> Fixes: 4e4fa6c12d ("accel/tcg: Complete cpu initialization before registration")
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>   accel/tcg/cputlb.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 4e040a1cb9..ac0245ee6c 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -103,6 +103,10 @@ static void tb_jmp_cache_clear_page(CPUState *cpu, target_ulong page_addr)
>       int i, i0 = tb_jmp_cache_hash_page(page_addr);
>       CPUJumpCache *jc = cpu->tb_jmp_cache;
>   
> +    if (!jc) {
> +        return;
> +    }

While I think we shouldn't arrive here for KVM, it was previously not an error to do so. 
I'm going to go ahead and queue this while the correct cpregs change gets worked out.


r~