On 1/18/24 20:07, Igor Mammedov wrote:
On Thu, 18 Jan 2024 14:48:50 +0800
Kai <kai.k...@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.k...@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
