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

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

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

The new code fails:

   qemu_create_machine()
   qemu_apply_machine_options()
     applies link property "canbus0" with value canbus,
     error because fails to find canbus object
   ...
   qemu_exit_precreate()
     qemu_create_early_backends()
       ... creates "-object can-bus,id=canbus"

The fix is to provide a new function, called before qemu_create_machine,
which creates only the backends that are needed to create the machine.
AFAIK can-bus is the only one, because the xlnx-zcu102 machine has
link properties.

- Steve