Re: Resetting non-qdev children in a 3-phase reset device

[Philippe Mathieu-Daudé]

On 4/26/21 11:33 AM, Peter Maydell wrote:
> On Mon, 26 Apr 2021 at 10:23, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 4/25/21 8:33 PM, Peter Maydell wrote:
>>> On Sat, 24 Apr 2021 at 14:04, Philippe Mathieu-Daudé <f4bug@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.