* [PATCH] qdev: not add devices to bus in reverse order @ 2024-01-09 9:52 kai.kang 2024-01-17 4:23 ` Kai 0 siblings, 1 reply; 9+ messages in thread From: kai.kang @ 2024-01-09 9:52 UTC (permalink / raw) To: qemu-devel From: Kai Kang <kai.kang@windriver.com> 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. Signed-off-by: Kai Kang <kai.kang@windriver.com> --- hw/core/qdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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); -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] qdev: not add devices to bus in reverse order 2024-01-09 9:52 [PATCH] qdev: not add devices to bus in reverse order kai.kang @ 2024-01-17 4:23 ` Kai 0 siblings, 0 replies; 9+ messages in thread From: Kai @ 2024-01-17 4:23 UTC (permalink / raw) To: qemu-devel On 1/9/24 17:52, kai.kang@windriver.com wrote: > From: Kai Kang <kai.kang@windriver.com> > > 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. Ping. > > Signed-off-by: Kai Kang <kai.kang@windriver.com> > --- > hw/core/qdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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); -- Kai Kang Wind River Linux ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] qdev: not add devices to bus in reverse order @ 2024-01-09 9:20 Kai Kang 2024-01-17 17:31 ` Peter Maydell 0 siblings, 1 reply; 9+ messages in thread From: Kai Kang @ 2024-01-09 9:20 UTC (permalink / raw) To: qemu-devel 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. Signed-off-by: Kai Kang <kai.kang@windriver.com> --- hw/core/qdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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); -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] qdev: not add devices to bus in reverse order 2024-01-09 9:20 Kai Kang @ 2024-01-17 17:31 ` Peter Maydell 2024-01-18 6:48 ` Kai 0 siblings, 1 reply; 9+ messages in thread From: Peter Maydell @ 2024-01-17 17:31 UTC (permalink / raw) To: Kai Kang; +Cc: qemu-devel, Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost (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. 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] qdev: not add devices to bus in reverse order 2024-01-17 17:31 ` Peter Maydell @ 2024-01-18 6:48 ` Kai 2024-01-18 10:05 ` Peter Maydell 2024-01-18 12:07 ` Igor Mammedov 0 siblings, 2 replies; 9+ messages in thread From: Kai @ 2024-01-18 6:48 UTC (permalink / raw) To: Peter Maydell Cc: qemu-devel, Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost 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. 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] qdev: not add devices to bus in reverse order 2024-01-18 6:48 ` Kai @ 2024-01-18 10:05 ` Peter Maydell 2024-01-18 12:07 ` Igor Mammedov 1 sibling, 0 replies; 9+ messages in thread From: Peter Maydell @ 2024-01-18 10:05 UTC (permalink / raw) To: Kai; +Cc: qemu-devel, Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost On Thu, 18 Jan 2024 at 06:49, 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. Thanks. This seems like something that should be fixed in the AML related code, not here. -- PMM ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] qdev: not add devices to bus in reverse order 2024-01-18 6:48 ` Kai 2024-01-18 10:05 ` Peter Maydell @ 2024-01-18 12:07 ` Igor Mammedov 2024-01-22 1:59 ` Kai 1 sibling, 1 reply; 9+ messages in thread From: Igor Mammedov @ 2024-01-18 12:07 UTC (permalink / raw) To: Kai Cc: Peter Maydell, qemu-devel, Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost, mst 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. this problem description should be in commit message. 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) 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? > 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] qdev: not add devices to bus in reverse order 2024-01-18 12:07 ` Igor Mammedov @ 2024-01-22 1:59 ` Kai 2024-01-24 16:11 ` Igor Mammedov 0 siblings, 1 reply; 9+ messages in thread From: Kai @ 2024-01-22 1:59 UTC (permalink / raw) To: Igor Mammedov Cc: Peter Maydell, qemu-devel, Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost, mst 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] qdev: not add devices to bus in reverse order 2024-01-22 1:59 ` Kai @ 2024-01-24 16:11 ` Igor Mammedov 0 siblings, 0 replies; 9+ messages in thread From: Igor Mammedov @ 2024-01-24 16:11 UTC (permalink / raw) To: Kai Cc: Peter Maydell, qemu-devel, Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost, mst On Mon, 22 Jan 2024 09:59:06 +0800 Kai <kai.kang@windriver.com> wrote: > 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. -chardev sometype,id=foo[,...] -device isa-serial,chardev=foo,index=0[,...] > > 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. I don't observe index [nor IRQ/IO] swapping in ACPI tables. test CLI: $ qemu -M pc -serial file:/tmp/test1 -serial file:/tmp/test2 QEMU v5.0 (before dcdbfaafe90a) has following serial descriptors: Device (COM1) { Name (_HID, EisaId ("PNP0501") /* 16550A-compatible COM Serial Port */) // _HID: Hardware ID Name (_UID, One) // _UID: Unique ID Method (_STA, 0, NotSerialized) // _STA: Status { ... } Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { IO (Decode16, 0x03F8, // Range Minimum 0x03F8, // Range Maximum 0x00, // Alignment 0x08, // Length ) IRQNoFlags () {4} }) } Device (COM2) { Name (_HID, EisaId ("PNP0501") /* 16550A-compatible COM Serial Port */) // _HID: Hardware ID Name (_UID, 0x02) // _UID: Unique ID Method (_STA, 0, NotSerialized) // _STA: Status { ... } Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { IO (Decode16, 0x02F8, // Range Minimum 0x02F8, // Range Maximum 0x00, // Alignment 0x08, // Length ) IRQNoFlags () {3} }) } current master branch (v 9.0+): Device (COM2) { Name (_HID, EisaId ("PNP0501") /* 16550A-compatible COM Serial Port */) // _HID: Hardware ID Name (_UID, 0x02) // _UID: Unique ID Name (_STA, 0x0F) // _STA: Status Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { IO (Decode16, 0x02F8, // Range Minimum 0x02F8, // Range Maximum 0x00, // Alignment 0x08, // Length ) IRQNoFlags () {3} }) } Device (COM1) { Name (_HID, EisaId ("PNP0501") /* 16550A-compatible COM Serial Port */) // _HID: Hardware ID Name (_UID, One) // _UID: Unique ID Name (_STA, 0x0F) // _STA: Status Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { IO (Decode16, 0x03F8, // Range Minimum 0x03F8, // Range Maximum 0x00, // Alignment 0x08, // Length ) IRQNoFlags () {4} }) } The only difference I see is the order in which ports are described in DSDT but otherwise I'd say descriptors are identical and within ACPI spec. testing with RHEL9 guest image, serial ports are persistent and the same between v5.0 and current master QEMU branch (1st is ttyS0 and 2nd is ttyS1). All of above hints it's unlikely to be a QEMU issue. I'd suggest to look into freebsd code and check how it enumerates serial ports. > 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 > >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-01-24 16:12 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-09 9:52 [PATCH] qdev: not add devices to bus in reverse order kai.kang 2024-01-17 4:23 ` Kai -- strict thread matches above, loose matches on Subject: below -- 2024-01-09 9:20 Kai Kang 2024-01-17 17:31 ` Peter Maydell 2024-01-18 6:48 ` Kai 2024-01-18 10:05 ` Peter Maydell 2024-01-18 12:07 ` Igor Mammedov 2024-01-22 1:59 ` Kai 2024-01-24 16:11 ` Igor Mammedov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).