Re: [PATCH] target/loongarch: Fix SWI interrupt delivery via CSR_ESTAT

On 2026/8/5 下午3:19, Philippe Mathieu-Daudé wrote:
> On 2026-08-05 8:56, Bibo Mao wrote:
>> applied to loongarch-next with small modification.
>>
>> diff --git a/target/loongarch/tcg/csr_helper.c b/target/loongarch/tcg/ 
>> csr_helper.c
>> index 7dc33bc180..d8d54d81e0 100644
>> --- a/target/loongarch/tcg/csr_helper.c
>> +++ b/target/loongarch/tcg/csr_helper.c
>> @@ -103,9 +103,22 @@ target_ulong helper_csrwr_estat(CPULoongArchState 
>> *env, target_ulong val)
>>   {
>>       CPUSysState *sys = env_sys(env);
>>       int64_t old_v = sys->CSR_ESTAT;
>> +    CPUState *cs = env_cpu(env);
>>
>>       /* Only IS[1:0] can be written */
>>       sys->CSR_ESTAT = deposit64(sys->CSR_ESTAT, 0, 2, val);
>> +    /*
>> +     * Software interrupts (SWI0/SWI1) are latched in CSR.ESTAT.IS[1:0].
>> +     * Make sure the CPU interrupt request state tracks the pending 
>> bits,
>> +     * matching the behavior of loongarch_cpu_set_irq().
>> +     */
>> +    bql_lock();
>> +    if (FIELD_EX64(sys->CSR_ESTAT, CSR_ESTAT, IS)) {
>> +        cpu_interrupt(cs, CPU_INTERRUPT_HARD);
>> +    } else {
>> +        cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
>> +    }
>> +    bql_unlock();
>>
>>       return old_v;
>>   }
>>
>> On 2026/4/15 下午7:17, Andrew S. Rightenburg via qemu development wrote:
>>> In TCG mode, helper_csrwr_estat() updates CSR.ESTAT.IS[1:0] (SWI0/SWI1)
>>> when the guest writes CSR_ESTAT, but it did not update the CPU interrupt
>>> request state. As a result, software interrupts could be observed as 
>>> pending
>>> in CSR.ESTAT while no interrupt exception was taken.
>>>
>>> Update CPU_INTERRUPT_HARD after modifying CSR_ESTAT, matching the 
>>> behavior of
>>> loongarch_cpu_set_irq(). The helper runs without the Big QEMU Lock 
>>> (BQL), so
>>> take the BQL while calling cpu_interrupt().
> 
> Eh I don't understand this justification about taking the BQL...
I guess there is g_assert(bql_locked()) in cpu_interrupt() -:)
> 
> If we can not call loongarch_cpu_set_irq() directly, we should extract
> the common code in a common method and re-use, not duplicate the same
> code.
yes, I think it is better to call loongarch_cpu_set_irq() directly to 
avoid duplicated code.

Regards
Bibo Mao
> 
>>>
>>> Fixes: 5b1dedfe848b ("target/loongarch: Add LoongArch CSR instruction")
>>> Reported-by: Andrew S. Rightenburg <[email protected]>
>>> Signed-off-by: Andrew S. Rightenburg <[email protected]>
>>> ---
>>>   target/loongarch/tcg/csr_helper.c | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/target/loongarch/tcg/csr_helper.c 
>>> b/target/loongarch/tcg/ csr_helper.c
>>> index cd35ca93c7..0a5ed52eb6 100644
>>> --- a/target/loongarch/tcg/csr_helper.c
>>> +++ b/target/loongarch/tcg/csr_helper.c
>>> @@ -98,10 +98,22 @@ target_ulong helper_csrrd_msgir(CPULoongArchState 
>>> *env)
>>>   target_ulong helper_csrwr_estat(CPULoongArchState *env, 
>>> target_ulong val)
>>>   {
>>>       int64_t old_v = env->CSR_ESTAT;
>>> +    CPUState *cs = env_cpu(env);
>>>       /* Only IS[1:0] can be written */
>>>       env->CSR_ESTAT = deposit64(env->CSR_ESTAT, 0, 2, val);
>>> +    /*
>>> +     * Software interrupts (SWI0/SWI1) are latched in 
>>> CSR.ESTAT.IS[1:0].
>>> +     * Make sure the CPU interrupt request state tracks the pending 
>>> bits,
>>> +     * matching the behavior of loongarch_cpu_set_irq().
>>> +     */
>>> +    if (FIELD_EX64(env->CSR_ESTAT, CSR_ESTAT, IS)) {
>>> +        bql_lock();
>>> +        cpu_interrupt(cs, CPU_INTERRUPT_HARD);
>>> +        bql_unlock();
>>> +    }
>>> +
>>>       return old_v;
>>>   }
>>>
>>
>>