On 4/26/21 11:33 AM, Peter Maydell wrote:
> On Mon, 26 Apr 2021 at 10:23, Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
>> On 4/25/21 8:33 PM, Peter Maydell wrote:
>>> On Sat, 24 Apr 2021 at 14:04, Philippe Mathieu-Daudé <f4...@amsat.org>
>>> wrote:
>>>> I now understand better the diag288 case, but I still don't understand
>>>> the TYPE_APIC one. It has no DeviceClass::reset(), its abstract parent
>>>> TYPE_APIC_COMMON register apic_reset_common() but being TYPE_DEVICE it
>>>> is not on a qbus. It is somehow connected to the X86CPU object, but the
>>>> single call to apic_init_reset() is from do_cpu_init() - not a reset
>>>> method -.
>>>
>>> pc_machine_reset() calls device_legacy_reset(cpu->apic_state)
>>> which is to say it invokes the DeviceState::reset method,
>>> which is either kvm_apic_reset or apic_reset_common.
>>
>> Oh, thanks! I guess "convoluted" is the proper adjective to describe
>> this reset logic. I suppose APIC is a very old device, part of the
>> Frankenstein PC, so hard to rework (because we are scared of the
>> implications of changing old & heavily used devices).
>
> This is mostly just another instance of "our reset logic doesn't
> deal well with devices which aren't on buses". The APIC isn't on a bus,
> so the machine that uses it has a local workaround to manually arrange
> for it to reset, just as it does for the CPU.
But the APIC is created within the CPU realize():
static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
{
APICCommonState *apic;
ObjectClass *apic_class = OBJECT_CLASS(apic_get_class());
cpu->apic_state = DEVICE(object_new_with_class(apic_class));
...
... so I'd expect the CPU (TYPE_X86_CPU) to reset the APIC in its
reset(), not the machine (regardless how the CPU itself is reset).
*But* there is an undocumented MachineClass::wakeup() handler which
complicates the picture:
static void pc_machine_reset(MachineState *machine)
{
CPUState *cs;
X86CPU *cpu;
qemu_devices_reset();
/* Reset APIC after devices have been reset to cancel
* any changes that qemu_devices_reset() might have done.
*/
CPU_FOREACH(cs) {
cpu = X86_CPU(cs);
if (cpu->apic_state) {
device_legacy_reset(cpu->apic_state);
}
}
}
static void pc_machine_wakeup(MachineState *machine)
{
cpu_synchronize_all_states();
pc_machine_reset(machine);
cpu_synchronize_all_post_reset();
}
It is called once:
/*
* Wake the VM after suspend.
*/
static void qemu_system_wakeup(void)
{
MachineClass *mc;
mc = current_machine ? MACHINE_GET_CLASS(current_machine) : NULL;
if (mc && mc->wakeup) {
mc->wakeup(current_machine);
}
}
and TYPE_PC_MACHINE is the single object implementing it.
This wakeup is handled in main_loop_should_exit() after the
qemu_reset_requested() check, and pc_machine_wakeup calls
cpu_synchronize_all_post_reset().
Could a 3-phase CPU reset simplify all this?
I agree it is 'mostly just another instance of "our reset logic
doesn't deal well with devices which aren't on buses"', but a
very convoluted one IMHO.
Regards,
Phil.
