Re: [PATCH v2 1/3] runstate: set crash_occurred on guest crashloaded

On 2026/08/05 21:10, Zhengrong Li wrote:
> Hi Philippe,
> 
> Thanks for the review.
> 
> I followed the existing pattern in qemu_system_guest_panicked(),
> which already sets crash_occurred on current_cpu when a guest
> panic is reported. The intent was to keep the panicked and
> crash-loaded paths consistent.
> 
> Putting it in the generic function also means any future crash
> notification mechanism (beyond pvpanic) would automatically
> benefit from identifying the faulting CPU in guest dumps.
> 
> I agree the original commit bac05aa9a77 ("cpu: Add crash_occurred
> flag into CPUState") doesn't explain the design choice well.
> 
> If you prefer moving it to handle_event() in the caller, I'm
> happy to adjust. What do you think?

I find your reasoning makes sense. It is a common requirement of 
qemu_system_guest_panicked() callers to update current_cpu, and having 
the logic to update the CPU in the same function will de-duplicate that 
logic. While there is only one caller of 
qemu_system_guest_crashloaded(), consistency and future prospect 
justifies to do the same.

On the other hand, it is also true that a function that is prefixed 
qemu_system_* implicitly updating current_cpu is obscure. I think the 
intent would be clearer if the functions explicitly take the CPU via a 
parameter. In fact, most callers of qemu_system_guest_panicked() 
explicitly take the CPU via a parameter. e.g., kvm_cpu_exec(). 
qemu_system_guest_panicked() breaks the propagation of the CPU value and 
potentially attributes the panic to a wrong CPU.

We are adding a similar function and its call, so it is a good timing to 
perform such refactoring. I still don't require you to do that since 
updating the signature of qemu_system_guest_panicked() requires a simple 
yet codebase-wide edit that is not strictly necessary for the feature we 
are adding.

Regards,
Akihiko Odaki