Re: [PATCH] qdev: not add devices to bus in reverse order

[Kai <kai.kang@windriver.com>]

On 1/18/24 20:07, Igor Mammedov wrote:
> On Thu, 18 Jan 2024 14:48:50 +0800
> Kai <kai.kang@windriver.com> wrote:
>
>> On 1/18/24 01:31, Peter Maydell wrote:
>>> (cc'd the people listed for this file in MAINTAINERS)
>>>
>>> On Tue, 9 Jan 2024 at 13:53, Kai Kang <kai.kang@windriver.com> wrote:
>>>> When this section of source codes were added via commit:
>>>>
>>>> * 02e2da45c4 Add common BusState
>>>>
>>>> it added devices to bus with LIST_INSERT_HEAD() which operated on the
>>>> single direction list. It didn't have something like LIST_INSERT_TAIL()
>>>> at that time and kept that way when turned to QTAILQ.
>>>>
>>>> Then it causes the fist device in qemu command line inserted at the end
>>>> of the bus child link list. And when realize them, the first device will
>>>> be the last one to be realized.
>>>>
>>>> Replace QTAILQ_INSERT_HEAD_RCU() with QTAILQ_INSERT_TAIL_RCU() to make
>>>> sure that devices are added to bus with the sequence in the command
>>>> line.
>>> What are the problems being caused by the the list items being added
>>> in reverse order? Your commit message doesn't say what specific
>>> bug or problem it's trying to fix.
>> The problem I met was just as I asked for for help in the maillist on
>> Dec 18, 2023.
>>
>> The indexes of serial isa devices changes with the commit dcdbfaafe90a
>> since qemu 6.2.0.
>> Before the commit, it creates devices literally with "1" & "2":
>>
>> @@ -1252,8 +1222,6 @@ static void build_isa_devices_aml(Aml *table)
>>            aml_append(scope, build_fdc_device_aml(fdc));
>>        }
>>        aml_append(scope, build_lpt_device_aml());
>> -    build_com_device_aml(scope, 1);
>> -    build_com_device_aml(scope, 2);
>>
>> After apply the commit, it uses the 'aml builder' way and the devices
>> are handled in reverse way.
>> Then the indexes are reversed. It affects guest os such as freebsd. When
>> run `pstat -t` on freebsd
>> with qemu, the sequence of the output is not right.
>>
>> root@freebsd:~ # pstat -t
>>         LINE   INQ  CAN  LIN  LOW  OUTQ  USE  LOW   COL  SESS  PGID STATE
>>        ttyu2     0    0    0    0     0    0    0     0     0     0 IC
>>        ttyu3     0    0    0    0     0    0    0     0     0     0 IC
>>        ttyu1     0    0    0    0     0    0    0     0     0     0 IC
>>        ttyu0  1920    0    0  192  1984    0  199     0   664   668 ICOi
>>
>> It is expected with ascend order which keeps the same behavior with
>> older version qemu.

Hi Peter & Igor,

Thanks for your reply.

> this problem description should be in commit message.

Will do next time.


> As for fixing it I'd wouldn't touch bus order as Peter already noted
> it has high chances to break behavior elsewhere.
>
> current state of COM naming:
>     1: QEMU 6.1  all machine types: COM1 { uid: 1, irq: 4}, COM2 { uid: 2, irq: 3}
>     2: QEMU 6.2+ all machine types: COM1 { uid: 2, irq: 4}, COM1 { uid: 1, irq: 3}
> all of above in default case where user doesn't supply 'index' explicitly.
>
> With 'index' provided explicitly old case #1 might break due to
> hardcoded resource values in former build_com_device_aml().
> #2 (current code) doesn't have issues with resource values
> when explicit 'index' is used (which can be a possible workaround)

How to assign explicit 'index' in the command line? I don't figure it 
out the option for it.


>
> So we essentially have changed enumeration for 6.1 and older
> machine types in incompatible way with QEMU-6.2+ builds.
> (and that in it's turn breaks exiting VM config when it
> is started on QEMU-6.2+)
>
> Kai,
> Does above sum-up the issue you are encountering?

Yes, it does.

Thanks,
Kai

>
>> Regards,
>> Kai
>>
>>> In general this kind of patch is something I'm very cautious about,
>>> because it seems very likely that various bits of the code where
>>> order does matter will currently be expecting (and working around)
>>> the reverse-order behaviour, because that's what has been done by
>>> bus_add_child() for the last 20-plus years. (As one concrete example,
>>> see the big comment at the top of create_virtio_devices() in
>>> hw/arm/virt.c. There are probably others where we didn't comment
>>> on the ordering but just assume it.)
>>>   
>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>>> index 43d863b0c5..5e2ff43715 100644
>>>> --- a/hw/core/qdev.c
>>>> +++ b/hw/core/qdev.c
>>>> @@ -89,7 +89,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>>>>        kid->child = child;
>>>>        object_ref(OBJECT(kid->child));
>>>>
>>>> -    QTAILQ_INSERT_HEAD_RCU(&bus->children, kid, sibling);
>>>> +    QTAILQ_INSERT_TAIL_RCU(&bus->children, kid, sibling);
>>>>
>>>>        /* This transfers ownership of kid->child to the property.  */
>>>>        snprintf(name, sizeof(name), "child[%d]", kid->index);
>>> thanks
>>> -- PMM
>>

-- 
Kai Kang
Wind River Linux