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

Eric Auger posted 1 patch 1 year, 2 months ago
accel/tcg/cputlb.c | 4 ++++
1 file changed, 4 insertions(+)
[PATCH] accel/tcg: test CPUJumpCache in tb_jmp_cache_clear_page()
Posted by Eric Auger 1 year, 2 months ago
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()
Posted by Richard Henderson 1 year, 2 months ago
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~
Re: [PATCH] accel/tcg: test CPUJumpCache in tb_jmp_cache_clear_page()
Posted by Philippe Mathieu-Daudé 1 year, 2 months ago
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()
Posted by Eric Auger 1 year, 2 months ago
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);
>>       }
>