Re: [RFC V1 04/14] accel: set accelerator and machine props earlier

[Steven Sistare <steven.sistare@oracle.com>]

On 10/22/2024 4:30 AM, David Hildenbrand wrote:
> On 18.10.24 21:15, Steven Sistare wrote:
>> On 10/18/2024 11:40 AM, Steven Sistare wrote:
>>> On 10/18/2024 11:32 AM, Steven Sistare wrote:
>>>> On 10/18/2024 11:08 AM, Fabiano Rosas wrote:
>>>>> Steve Sistare <steven.sistare@oracle.com> writes:
>>>>>
>>>>>> Make all global and compat properties available before the first objects
>>>>>> are created.  Set accelerator compatibility properties in
>>>>>> configure_accelerators, when the accelerator is chosen, and call
>>>>>> configure_accelerators earlier.  Set machine options earlier.
>>>>>>
>>>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>>>> ---
>>>>>>    accel/accel-system.c |  2 --
>>>>>>    system/vl.c          | 34 ++++++++++++++++++----------------
>>>>>>    2 files changed, 18 insertions(+), 18 deletions(-)
>>>>>>
>>>>>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>>>>>> index f6c947d..c8aeae4 100644
>>>>>> --- a/accel/accel-system.c
>>>>>> +++ b/accel/accel-system.c
>>>>>> @@ -41,8 +41,6 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
>>>>>>            ms->accelerator = NULL;
>>>>>>            *(acc->allowed) = false;
>>>>>>            object_unref(OBJECT(accel));
>>>>>> -    } else {
>>>>>> -        object_set_accelerator_compat_props(acc->compat_props);
>>>>>>        }
>>>>>>        return ret;
>>>>>>    }
>>>>>> diff --git a/system/vl.c b/system/vl.c
>>>>>> index b94a6b9..bca2292 100644
>>>>>> --- a/system/vl.c
>>>>>> +++ b/system/vl.c
>>>>>> @@ -2346,6 +2346,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>>>>>            goto bad;
>>>>>>        }
>>>>>> +    object_set_accelerator_compat_props(ac->compat_props);
>>>>>>        acs->accel = accel;
>>>>>>        return 1;
>>>>>> @@ -3728,29 +3729,14 @@ void qemu_init(int argc, char **argv)
>>>>>>        parse_memory_options();
>>>>>>        qemu_create_machine(machine_opts_dict);
>>>>>> -
>>>>>> -    suspend_mux_open();
>>>>>> -
>>>>>> -    qemu_disable_default_devices();
>>>>>> -    qemu_setup_display();
>>>>>> -    qemu_create_default_devices();
>>>>>> -    qemu_create_early_backends();
>>>>>> -
>>>>>>        qemu_apply_legacy_machine_options(machine_opts_dict);
>>>>>>        qemu_apply_machine_options(machine_opts_dict);
>>>>>>        qobject_unref(machine_opts_dict);
>>>>>> -    phase_advance(PHASE_MACHINE_CREATED);
>>>>>> -    /*
>>>>>> -     * Note: uses machine properties such as kernel-irqchip, must run
>>>>>> -     * after qemu_apply_machine_options.
>>>>>> -     */
>>>>>>        accel = configure_accelerators(argv[0]);
>>>>>> -    create_accelerator(accel);
>>>>>> -    phase_advance(PHASE_ACCEL_CREATED);
>>>>>>        /*
>>>>>> -     * Beware, QOM objects created before this point miss global and
>>>>>> +     * QOM objects created after this point see all global and
>>>>>>         * compat properties.
>>>>>>         *
>>>>>>         * Global properties get set up by qdev_prop_register_global(),
>>>>>> @@ -3765,6 +3751,22 @@ void qemu_init(int argc, char **argv)
>>>>>>         * called from do_configure_accelerator().
>>>>>>         */
>>>>>> +    suspend_mux_open();
>>>>>> +
>>>>>> +    qemu_disable_default_devices();
>>>>>> +    qemu_setup_display();
>>>>>> +    qemu_create_default_devices();
>>>>>> +    qemu_create_early_backends();
>>>>>> +
>>>>>> +    phase_advance(PHASE_MACHINE_CREATED);
>>>>>> +
>>>>>> +    /*
>>>>>> +     * Note: uses machine properties such as kernel-irqchip, must run
>>>>>> +     * after qemu_apply_machine_options.
>>>>>> +     */
>>>>>> +    create_accelerator(accel);
>>>>>> +    phase_advance(PHASE_ACCEL_CREATED);
>>>>>> +
>>>>>>        machine_class = MACHINE_GET_CLASS(current_machine);
>>>>>>        if (!qtest_enabled() && machine_class->deprecation_reason) {
>>>>>>            warn_report("Machine type '%s' is deprecated: %s",
>>>>>
>>>>> Hi Steve,
>>>>>
>>>>> after this commit:
>>>>>
>>>>> $ QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/xlnx-can-test
>>>>> # random seed: R02Saf9b44f2d88dd417052905692ee79981
>>>>> 1..5
>>>>> # Start of aarch64 tests
>>>>> # Start of net tests
>>>>> # Start of can tests
>>>>> # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-2396.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-2396.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine xlnx-zcu102 -object can-bus,id=canbus -machine canbus0=canbus -machine canbus1=canbus -accel qtest
>>>>> qemu-system-aarch64: Device 'canbus' not found
>>>>>
>>>>> I tried briefly to figure out what the issue is, but I don't really
>>>>> understand the dependencies involved. Hope you can tell us.
>>>>
>>>> Thanks! I forgot to define the preinit method for the qtest accelerator in patch 1.
>>>> I'll verify that fixes the problem and send you a one-off patch if you want to continue
>>>> testing.
>>>
>>> Actually that is not a problem.  qtest qtest_init_accel does nothing, so preinit will do
>>> nothing, so it is OK to not define preinit.
>>>
>>> Still looking.
>>
>> I understand this now.  The old code worked in this order:
>>
>>     qemu_create_early_backends()
>>       ... creates "-object can-bus,id=canbus"
>>     qemu_create_machine()
>>     qemu_apply_machine_options()
>>       applies link property "canbus0" with value canbus, finds canbus object
> 
> Now I am confused.
> 
> I think the current code does:
> 
> qemu_create_machine(machine_opts_dict);
> qemu_create_early_backends();
> qemu_apply_machine_options(machine_opts_dict);
> 
> Isn't the relevant part that we may only apply the machine options after creating the early backends?

Yes, I showed the wrong order for the old code, but our explanations are equivalent.

Perhaps this could be fixed by moving qemu_apply_machine_options after
qemu_create_early_backends in qemu_exit_precreate.  But that seems risky and
fragile,  as there would be a large window of code between qemu_create_machine
and qemu_apply_machine_options where the machine has been created but the machine
options and not known.

- Steve