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

[David Hildenbrand <david@redhat.com>]

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?

-- 
Cheers,

David / dhildenb