bibo mao <maob...@loongson.cn> writes:
On 2025/3/19 下午2:09, Markus Armbruster wrote:
>> Bibo Mao <maob...@loongson.cn> writes:
>>
>>> In function virt_cpu_irq_init(), there is notification with ipi and extioi
>>> interrupt controller for cpu creation. Local variable with error type is
>>> used, however there is no check with its return value.
>>
>> Good catch.
>>
>> When the first call fails, we pass non-null @err to the second call,
>> which is wrong. If that one also fails, it'll likely trip
>> error_setv()'s assertion.
>>
>>> Here set dest error object with error_abort, rather than local variable, so
>>> application will abort to run if there is error.
>>
>> Why is failure impossible there?
> In plug hanlder of extioi/ipi, there is only warn_report() if object is
> not TYPE_LOONGARCH_CPU, parameter errp is not changed.
>
> With caller funciton virt_cpu_irq_init(), DEVICE(cs) is object with type
> TYPE_LOONGARCH_CPU always, so failure is impossible here.
>
>>
>> If failure is impossible, the code before the patch is harmlessly wrong.
> yes, it is harmlessly wrong.
Could use something like
target/loongarch: Clean up virt_cpu_irq_init() error handling
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.
virt_cpu_irq_init() is wrong that way: it passes &err to
hotplug_handler_plug() twice. If both calls failed, this could trip
error_setv()'s assertion. Moreover, if just one fails, the Error
object leaks. Fortunately, these calls can't actually fail.
Messed up in commit 50ebc3fc47f7 (hw/intc/loongarch_ipi: Notify ipi
object when cpu is plugged) and commit 087a23a87c57
(hw/intc/loongarch_extioi: Use cpu plug notification).
Clean this up by clearing passing &error_abort instead.
Signed-off-by: Bibo Mao <maob...@loongson.cn>
Note: I replaced the Fixes: tags by a "Messed up ..." paragraph, because
there is no bug to fix according to your explanation.
With something like that:
Acked-by: Markus Armbruster <arm...@redhat.com>
> Regards
> Bibo Mao
>>
>> If failure is possible, the code before the patch has a crash bug, and
>> the patch makes it crash harder, i.e. when either call fails instead of
>> when both fail.
>>
>>> Fixes: 50ebc3fc47fe (hw/intc/loongarch_ipi: Notify ipi object when cpu is
>>> plugged)
>>> Fixes: 087a23a87c57 (hw/intc/loongarch_extioi: Use cpu plug notification)
>>> Signed-off-by: Bibo Mao <maob...@loongson.cn>
