chardev/char.c | 32 +++++-- hw/arm/omap1.c | 8 +- hw/arm/omap2.c | 25 +++--- hw/char/omap_uart.c | 2 +- hw/char/serial-isa.c | 12 ++- hw/char/serial-pci-multi.c | 55 +++++++----- hw/char/serial-pci.c | 18 +++- hw/char/serial.c | 170 ++++++++++++++++++++++++++++------- hw/core/qdev-properties.c | 50 ----------- hw/core/qdev.c | 15 +--- hw/core/sysbus.c | 32 ------- hw/cris/axis_dev88.c | 4 - hw/display/sm501.c | 33 +++++-- hw/dma/sparc32_dma.c | 2 +- hw/gpio/omap_gpio.c | 42 ++++----- hw/i2c/omap_i2c.c | 19 ++-- hw/i2c/smbus_eeprom.c | 17 ++-- hw/i386/pc.c | 7 +- hw/i386/vmmouse.c | 8 +- hw/input/pckbd.c | 8 +- hw/intc/etraxfs_pic.c | 26 +----- hw/intc/grlib_irqmp.c | 35 +------- hw/intc/omap_intc.c | 17 ++-- hw/m68k/q800.c | 3 +- hw/mips/boston.c | 2 +- hw/mips/cps.c | 2 +- hw/mips/mips_jazz.c | 3 +- hw/mips/mips_malta.c | 2 +- hw/mips/mips_mipssim.c | 14 ++- hw/net/dp8393x.c | 7 +- hw/net/etraxfs_eth.c | 44 ++++++--- hw/net/lance.c | 5 +- hw/net/pcnet-pci.c | 2 +- hw/net/pcnet.h | 2 +- hw/sh4/r2d.c | 2 +- hw/sparc/leon3.c | 15 +++- include/hw/arm/omap.h | 52 +++++++++++ include/hw/char/serial.h | 43 ++++++--- include/hw/cris/etraxfs.h | 20 +---- include/hw/input/i8042.h | 4 +- include/hw/qdev-properties.h | 27 ------ include/hw/sysbus.h | 13 +-- include/qemu/id.h | 1 + qom/qom-qmp-cmds.c | 10 --- target/cris/cpu.c | 8 ++ target/cris/cpu.h | 1 + util/id.c | 1 + 47 files changed, 497 insertions(+), 423 deletions(-)
Hi, QDEV_PROP_PTR is marked in multiple places as "FIXME/TODO/remove me". In most cases, it can be easily replaced with QDEV_PROP_LINK when the pointer points to an Object. There are a few places where such substitution isn't possible. For those places, it seems reasonable to use a specific setter method instead, and keep the user_creatable = false. In other places, proper usage of qdev or other facilies is the solution. The serial code wasn't converted to qdev, which makes it a bit more archaic to deal with. Let's convert it first, so we can more easily embed it from other devices, and re-export some properties and drop QDEV_PROP_PTR usage. v4: (after Peter & Philippe reviews) - replaced "self" variable names with abbreviations - split "mips: inline serial_init()" - new patches: "mips: use sysbus_mmio_get_region() instead of internal fields", "leon3: use qdev gpio facilities for the PIL", "qdev: use g_strcmp0() instead of open-coding it", "qdev/qom: remove some TODO limitations now that PROP_PTR is gone" - dropped patches: "sparc: move PIL irq handling to cpu.c" & "serial: add "instance-id" property" - various comments / commit message tweaks - added r-b tags v3: - introduce SerialMM and SerialIO sysbus devices - remove serial_mm_connect introduced in v2 - replace "base" property introduced in v2, use "instance-id" for vmstate purpose only - add a few preliminary clean-ups v2: - qom-ify serial - embed the serial from sm501, and expose a "chardev" property - add "leon3: use qemu_irq framework instead of callback as property" - add "sparc: move PIL irq handling to cpu.c" - add "cris: improve passing PIC interrupt vector to the CPU" - misc comment/todo changes, add r-b tags Marc-André Lureau (37): qdev: remove unused qdev_prop_int64 sysbus: remove unused sysbus_try_create* sysbus: remove outdated comment chardev: generate an internal id when none given serial-pci-multi: factor out multi_serial_get_port_count() serial: initial qom-ification serial: register vmsd with DeviceClass serial: add "chardev" property serial: add "baudbase" property serial: realize the serial device serial: replace serial_exit_core() with unrealize serial: start making SerialMM a sysbus device serial-mm: add "regshift" property serial-mm: add endianness property serial-mm: use sysbus facilities serial: make SerialIO a sysbus device mips: inline serial_init() mips: baudbase is 115200 by default mips: use sysbus_add_io() mips: use sysbus_mmio_get_region() instead of internal fields sm501: make SerialMM a child, export chardev property vmmouse: replace PROP_PTR with PROP_LINK lance: replace PROP_PTR with PROP_LINK etraxfs: remove PROP_PTR usage dp8393x: replace PROP_PTR with PROP_LINK leon3: use qemu_irq framework instead of callback as property leon3: use qdev gpio facilities for the PIL qdev: use g_strcmp0() instead of open-coding it RFC: mips/cps: fix setting saar property cris: improve passing PIC interrupt vector to the CPU smbus-eeprom: remove PROP_PTR omap-intc: remove PROP_PTR omap-i2c: remove PROP_PTR omap-gpio: remove PROP_PTR qdev: remove PROP_MEMORY_REGION qdev: remove QDEV_PROP_PTR qdev/qom: remove some TODO limitations now that PROP_PTR is gone chardev/char.c | 32 +++++-- hw/arm/omap1.c | 8 +- hw/arm/omap2.c | 25 +++--- hw/char/omap_uart.c | 2 +- hw/char/serial-isa.c | 12 ++- hw/char/serial-pci-multi.c | 55 +++++++----- hw/char/serial-pci.c | 18 +++- hw/char/serial.c | 170 ++++++++++++++++++++++++++++------- hw/core/qdev-properties.c | 50 ----------- hw/core/qdev.c | 15 +--- hw/core/sysbus.c | 32 ------- hw/cris/axis_dev88.c | 4 - hw/display/sm501.c | 33 +++++-- hw/dma/sparc32_dma.c | 2 +- hw/gpio/omap_gpio.c | 42 ++++----- hw/i2c/omap_i2c.c | 19 ++-- hw/i2c/smbus_eeprom.c | 17 ++-- hw/i386/pc.c | 7 +- hw/i386/vmmouse.c | 8 +- hw/input/pckbd.c | 8 +- hw/intc/etraxfs_pic.c | 26 +----- hw/intc/grlib_irqmp.c | 35 +------- hw/intc/omap_intc.c | 17 ++-- hw/m68k/q800.c | 3 +- hw/mips/boston.c | 2 +- hw/mips/cps.c | 2 +- hw/mips/mips_jazz.c | 3 +- hw/mips/mips_malta.c | 2 +- hw/mips/mips_mipssim.c | 14 ++- hw/net/dp8393x.c | 7 +- hw/net/etraxfs_eth.c | 44 ++++++--- hw/net/lance.c | 5 +- hw/net/pcnet-pci.c | 2 +- hw/net/pcnet.h | 2 +- hw/sh4/r2d.c | 2 +- hw/sparc/leon3.c | 15 +++- include/hw/arm/omap.h | 52 +++++++++++ include/hw/char/serial.h | 43 ++++++--- include/hw/cris/etraxfs.h | 20 +---- include/hw/input/i8042.h | 4 +- include/hw/qdev-properties.h | 27 ------ include/hw/sysbus.h | 13 +-- include/qemu/id.h | 1 + qom/qom-qmp-cmds.c | 10 --- target/cris/cpu.c | 8 ++ target/cris/cpu.h | 1 + util/id.c | 1 + 47 files changed, 497 insertions(+), 423 deletions(-) -- 2.24.0
Hi On Wed, Nov 20, 2019 at 7:25 PM Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > > Hi, > > QDEV_PROP_PTR is marked in multiple places as "FIXME/TODO/remove > me". In most cases, it can be easily replaced with QDEV_PROP_LINK when > the pointer points to an Object. > > There are a few places where such substitution isn't possible. For > those places, it seems reasonable to use a specific setter method > instead, and keep the user_creatable = false. In other places, > proper usage of qdev or other facilies is the solution. > > The serial code wasn't converted to qdev, which makes it a bit more > archaic to deal with. Let's convert it first, so we can more easily > embed it from other devices, and re-export some properties and drop > QDEV_PROP_PTR usage. Before v5, is there any other comment for the following patches: - "qdev: remove unused qdev_prop_int64" I spotted dead-code, never used. Peter would rather keep it. Worth it? - "chardev: generate an internal id when none given" As explained, this is necessary for qdev_prop_set_chr() - "serial: register vmsd with DeviceClass" This is standard qdev-ification, however it breaks backward migration, but that's just how qdev_set_legacy_instance_id() works. - "RFC: mips/cps: fix setting saar property" Perhaps I should have used FIX instead of RFC, because this should actually be a real fix. However I could use someone help to exercise the code path. - "sm501: make SerialMM a child, export chardev property" review? - "qdev/qom: remove some TODO limitations now that PROP_PTR is gone" This should be straightforward. > > v4: (after Peter & Philippe reviews) > - replaced "self" variable names with abbreviations > - split "mips: inline serial_init()" > - new patches: "mips: use sysbus_mmio_get_region() instead of internal > fields", "leon3: use qdev gpio facilities for the PIL", "qdev: use > g_strcmp0() instead of open-coding it", "qdev/qom: remove some TODO > limitations now that PROP_PTR is gone" > - dropped patches: "sparc: move PIL irq handling to cpu.c" & "serial: > add "instance-id" property" > - various comments / commit message tweaks > - added r-b tags > > v3: > - introduce SerialMM and SerialIO sysbus devices > - remove serial_mm_connect introduced in v2 > - replace "base" property introduced in v2, use "instance-id" for > vmstate purpose only > - add a few preliminary clean-ups > > v2: > - qom-ify serial > - embed the serial from sm501, and expose a "chardev" property > - add "leon3: use qemu_irq framework instead of callback as property" > - add "sparc: move PIL irq handling to cpu.c" > - add "cris: improve passing PIC interrupt vector to the CPU" > - misc comment/todo changes, add r-b tags > > Marc-André Lureau (37): > qdev: remove unused qdev_prop_int64 > sysbus: remove unused sysbus_try_create* > sysbus: remove outdated comment > chardev: generate an internal id when none given > serial-pci-multi: factor out multi_serial_get_port_count() > serial: initial qom-ification > serial: register vmsd with DeviceClass > serial: add "chardev" property > serial: add "baudbase" property > serial: realize the serial device > serial: replace serial_exit_core() with unrealize > serial: start making SerialMM a sysbus device > serial-mm: add "regshift" property > serial-mm: add endianness property > serial-mm: use sysbus facilities > serial: make SerialIO a sysbus device > mips: inline serial_init() > mips: baudbase is 115200 by default > mips: use sysbus_add_io() > mips: use sysbus_mmio_get_region() instead of internal fields > sm501: make SerialMM a child, export chardev property > vmmouse: replace PROP_PTR with PROP_LINK > lance: replace PROP_PTR with PROP_LINK > etraxfs: remove PROP_PTR usage > dp8393x: replace PROP_PTR with PROP_LINK > leon3: use qemu_irq framework instead of callback as property > leon3: use qdev gpio facilities for the PIL > qdev: use g_strcmp0() instead of open-coding it > RFC: mips/cps: fix setting saar property > cris: improve passing PIC interrupt vector to the CPU > smbus-eeprom: remove PROP_PTR > omap-intc: remove PROP_PTR > omap-i2c: remove PROP_PTR > omap-gpio: remove PROP_PTR > qdev: remove PROP_MEMORY_REGION > qdev: remove QDEV_PROP_PTR > qdev/qom: remove some TODO limitations now that PROP_PTR is gone > > chardev/char.c | 32 +++++-- > hw/arm/omap1.c | 8 +- > hw/arm/omap2.c | 25 +++--- > hw/char/omap_uart.c | 2 +- > hw/char/serial-isa.c | 12 ++- > hw/char/serial-pci-multi.c | 55 +++++++----- > hw/char/serial-pci.c | 18 +++- > hw/char/serial.c | 170 ++++++++++++++++++++++++++++------- > hw/core/qdev-properties.c | 50 ----------- > hw/core/qdev.c | 15 +--- > hw/core/sysbus.c | 32 ------- > hw/cris/axis_dev88.c | 4 - > hw/display/sm501.c | 33 +++++-- > hw/dma/sparc32_dma.c | 2 +- > hw/gpio/omap_gpio.c | 42 ++++----- > hw/i2c/omap_i2c.c | 19 ++-- > hw/i2c/smbus_eeprom.c | 17 ++-- > hw/i386/pc.c | 7 +- > hw/i386/vmmouse.c | 8 +- > hw/input/pckbd.c | 8 +- > hw/intc/etraxfs_pic.c | 26 +----- > hw/intc/grlib_irqmp.c | 35 +------- > hw/intc/omap_intc.c | 17 ++-- > hw/m68k/q800.c | 3 +- > hw/mips/boston.c | 2 +- > hw/mips/cps.c | 2 +- > hw/mips/mips_jazz.c | 3 +- > hw/mips/mips_malta.c | 2 +- > hw/mips/mips_mipssim.c | 14 ++- > hw/net/dp8393x.c | 7 +- > hw/net/etraxfs_eth.c | 44 ++++++--- > hw/net/lance.c | 5 +- > hw/net/pcnet-pci.c | 2 +- > hw/net/pcnet.h | 2 +- > hw/sh4/r2d.c | 2 +- > hw/sparc/leon3.c | 15 +++- > include/hw/arm/omap.h | 52 +++++++++++ > include/hw/char/serial.h | 43 ++++++--- > include/hw/cris/etraxfs.h | 20 +---- > include/hw/input/i8042.h | 4 +- > include/hw/qdev-properties.h | 27 ------ > include/hw/sysbus.h | 13 +-- > include/qemu/id.h | 1 + > qom/qom-qmp-cmds.c | 10 --- > target/cris/cpu.c | 8 ++ > target/cris/cpu.h | 1 + > util/id.c | 1 + > 47 files changed, 497 insertions(+), 423 deletions(-) > > -- > 2.24.0 > > -- Marc-André Lureau
Hi On Sun, Dec 1, 2019 at 2:19 PM Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > > - "chardev: generate an internal id when none given" > > As explained, this is necessary for qdev_prop_set_chr() ping > > - "serial: register vmsd with DeviceClass" > > This is standard qdev-ification, however it breaks backward migration, > but that's just how qdev_set_legacy_instance_id() works. See thread, someone could review or nack (if backward migration is a problem). > > - "sm501: make SerialMM a child, export chardev property" > > review? ping > > - "qdev/qom: remove some TODO limitations now that PROP_PTR is gone" > > This should be straightforward. ping thanks -- Marc-André Lureau
Hi Marc-André, On 12/11/19 1:01 PM, Marc-André Lureau wrote: > Hi > > On Sun, Dec 1, 2019 at 2:19 PM Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: >> >> >> - "chardev: generate an internal id when none given" >> >> As explained, this is necessary for qdev_prop_set_chr() > > ping > >> >> - "serial: register vmsd with DeviceClass" >> >> This is standard qdev-ification, however it breaks backward migration, >> but that's just how qdev_set_legacy_instance_id() works. > > See thread, someone could review or nack (if backward migration is a problem). > >> >> - "sm501: make SerialMM a child, export chardev property" >> >> review? > > ping This would be simpler you include "hw/display/sm501: Always map the UART0" previous your changes. To finish the serial QOM-ification, we need to make serial_reset a DeviceReset, then we can remove the qemu_register_reset() call. >> >> - "qdev/qom: remove some TODO limitations now that PROP_PTR is gone" >> >> This should be straightforward. > > ping
Hi Still trying to make progress on this series (which is preliminary to other pending work..): On Wed, Dec 11, 2019 at 4:01 PM Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > Hi > > On Sun, Dec 1, 2019 at 2:19 PM Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: > > > > > > - "chardev: generate an internal id when none given" > > > > As explained, this is necessary for qdev_prop_set_chr() > > ping > ping > > > > - "serial: register vmsd with DeviceClass" > > > > This is standard qdev-ification, however it breaks backward migration, > > but that's just how qdev_set_legacy_instance_id() works. > > See thread, someone could review or nack (if backward migration is a problem). > ping > > > > - "sm501: make SerialMM a child, export chardev property" > > > > review? > > ping > rebased on top of "hw/display/sm501: Always map the UART0" as requested by Philippe. Can wait for v5 to get review. > > > > - "qdev/qom: remove some TODO limitations now that PROP_PTR is gone" > > > > This should be straightforward. > > ping ping thanks -- Marc-André Lureau
On Sun, 1 Dec 2019 at 10:19, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > - "serial: register vmsd with DeviceClass" > > This is standard qdev-ification, however it breaks backward migration, > but that's just how qdev_set_legacy_instance_id() works. I don't understand this part. Surely the whole point of setting a legacy instance ID is exactly to preserve migration compatibility? If it doesn't do that then what does setting legacy ID value do? thanks -- PMM
Hi On Sun, Dec 1, 2019 at 9:18 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Sun, 1 Dec 2019 at 10:19, Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: > > > > - "serial: register vmsd with DeviceClass" > > > > This is standard qdev-ification, however it breaks backward migration, > > but that's just how qdev_set_legacy_instance_id() works. > > I don't understand this part. Surely the whole point > of setting a legacy instance ID is exactly to preserve > migration compatibility? If it doesn't do that then what > does setting legacy ID value do? > It works in old->new direction only, because new code can match the legacy instance id. But when going from new->old, the legacy instance id is lost, as it uses new 0-based instance_id. This is just the way we have done so far, but I wish to be corrected if I am wrong. See the commit description for a bit more details. -- Marc-André Lureau
On Sun, 1 Dec 2019 at 17:27, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > Hi > > On Sun, Dec 1, 2019 at 9:18 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Sun, 1 Dec 2019 at 10:19, Marc-André Lureau > > <marcandre.lureau@gmail.com> wrote: > > > > > > - "serial: register vmsd with DeviceClass" > > > > > > This is standard qdev-ification, however it breaks backward migration, > > > but that's just how qdev_set_legacy_instance_id() works. > > > > I don't understand this part. Surely the whole point > > of setting a legacy instance ID is exactly to preserve > > migration compatibility? If it doesn't do that then what > > does setting legacy ID value do? > > > > It works in old->new direction only, because new code can match the > legacy instance id. > > But when going from new->old, the legacy instance id is lost, as it > uses new 0-based instance_id. I still don't understand. My mental model of the situation is: * in the old (current) version of the code, the instance ID is some random thing resulting from what the old code does * in the new version of the code, we use qdev_set_legacy_instance_id, and so instead of using the ID you'd naturally get as a written-from-scratch qdev device, it uses the legacy value you pass in * thus the device/board in both old and new versions of QEMU uses the same value and migration in both directions works I don't understand why we would ever be using a "new 0-based instance_id" -- it seems to me that the whole point of setting a legacy ID value is that we will use it always, and I don't understand how the board code can know that it's going to be the target of an old->new migration as opposed to being the source of a new->old migration such that it can end up with a different ID value in the latter case. If qdev_set_legacy_instance_id() doesn't work the way I think it does above, what *does* it do ? thanks -- PMM
Hi On Sun, Dec 1, 2019 at 10:10 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Sun, 1 Dec 2019 at 17:27, Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: > > > > Hi > > > > On Sun, Dec 1, 2019 at 9:18 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > On Sun, 1 Dec 2019 at 10:19, Marc-André Lureau > > > <marcandre.lureau@gmail.com> wrote: > > > > > > > > - "serial: register vmsd with DeviceClass" > > > > > > > > This is standard qdev-ification, however it breaks backward migration, > > > > but that's just how qdev_set_legacy_instance_id() works. > > > > > > I don't understand this part. Surely the whole point > > > of setting a legacy instance ID is exactly to preserve > > > migration compatibility? If it doesn't do that then what > > > does setting legacy ID value do? > > > > > > > It works in old->new direction only, because new code can match the > > legacy instance id. > > > > But when going from new->old, the legacy instance id is lost, as it > > uses new 0-based instance_id. > > I still don't understand. My mental model of the situation is: > > * in the old (current) version of the code, the instance ID > is some random thing resulting from what the old code does right > * in the new version of the code, we use qdev_set_legacy_instance_id, > and so instead of using the ID you'd naturally get as a > written-from-scratch qdev device, it uses the legacy value > you pass in no, it only sets the SaveStateEntry.alias_id, which is only used during incoming migration in find_se(). Iow, it only works old->new. > * thus the device/board in both old and new versions of QEMU > uses the same value and migration in both directions works sadly no > > I don't understand why we would ever be using a "new 0-based > instance_id" -- it seems to me that the whole point of setting > a legacy ID value is that we will use it always, and I don't > understand how the board code can know that it's going to be > the target of an old->new migration as opposed to being the > source of a new->old migration such that it can end up with > a different ID value in the latter case. The target will find the "legacy" alias with find_se() on incoming migration, but any new outgoing migration will use the new 0-based instance_id > > If qdev_set_legacy_instance_id() doesn't work the way I > think it does above, what *does* it do ? just set the old alias_id for incoming migration. David, is that correct? thanks -- Marc-André Lureau
Apologies for the delay. * Marc-André Lureau (marcandre.lureau@gmail.com) wrote: > Hi > > On Sun, Dec 1, 2019 at 10:10 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Sun, 1 Dec 2019 at 17:27, Marc-André Lureau > > <marcandre.lureau@gmail.com> wrote: > > > > > > Hi > > > > > > On Sun, Dec 1, 2019 at 9:18 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > > > On Sun, 1 Dec 2019 at 10:19, Marc-André Lureau > > > > <marcandre.lureau@gmail.com> wrote: > > > > > > > > > > - "serial: register vmsd with DeviceClass" > > > > > > > > > > This is standard qdev-ification, however it breaks backward migration, > > > > > but that's just how qdev_set_legacy_instance_id() works. > > > > > > > > I don't understand this part. Surely the whole point > > > > of setting a legacy instance ID is exactly to preserve > > > > migration compatibility? If it doesn't do that then what > > > > does setting legacy ID value do? > > > > > > > > > > It works in old->new direction only, because new code can match the > > > legacy instance id. > > > > > > But when going from new->old, the legacy instance id is lost, as it > > > uses new 0-based instance_id. > > > > I still don't understand. My mental model of the situation is: > > > > * in the old (current) version of the code, the instance ID > > is some random thing resulting from what the old code does > > right > > > * in the new version of the code, we use qdev_set_legacy_instance_id, > > and so instead of using the ID you'd naturally get as a > > written-from-scratch qdev device, it uses the legacy value > > you pass in > > no, it only sets the SaveStateEntry.alias_id, which is only used > during incoming migration in find_se(). > > Iow, it only works old->new. > > > * thus the device/board in both old and new versions of QEMU > > uses the same value and migration in both directions works > > sadly no > > > > > I don't understand why we would ever be using a "new 0-based > > instance_id" -- it seems to me that the whole point of setting > > a legacy ID value is that we will use it always, and I don't > > understand how the board code can know that it's going to be > > the target of an old->new migration as opposed to being the > > source of a new->old migration such that it can end up with > > a different ID value in the latter case. > > The target will find the "legacy" alias with find_se() on incoming > migration, but any new outgoing migration will use the new 0-based > instance_id > > > > > If qdev_set_legacy_instance_id() doesn't work the way I > > think it does above, what *does* it do ? > > just set the old alias_id for incoming migration. > > David, is that correct? Yes, I think it is. However, I'm curious which devices you're finding are explicitly setting their id's; there aren't many - although there are some that probably should! For example, running an x86 image with: -device isa-parallel,chardev=... -device isa-serial -device isa-serial -trace enable=qemu_loadvm_state_section_startfull shows: qemu_loadvm_state_section_startfull(uint32_t section_id, const char *idstr, uint32_t instance_id, uint32_t version_id) "%u(%s) %u %u" 165217@1576179638.856300:qemu_loadvm_state_section_startfull 41(serial) 0 3 165217@1576179638.856307:qemu_loadvm_state_section_startfull 42(serial) 1 3 165217@1576179638.856311:qemu_loadvm_state_section_startfull 43(parallel_isa) 0 1 so those two serial devices are instances '0' and '1' I think by luck of their command line order, rather than having specified their base address (which would have been safer). Dave > thanks > > > -- > Marc-André Lureau > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Hi On Fri, Dec 13, 2019 at 12:18 AM Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > Apologies for the delay. > > * Marc-André Lureau (marcandre.lureau@gmail.com) wrote: > > Hi > > > > On Sun, Dec 1, 2019 at 10:10 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > On Sun, 1 Dec 2019 at 17:27, Marc-André Lureau > > > <marcandre.lureau@gmail.com> wrote: > > > > > > > > Hi > > > > > > > > On Sun, Dec 1, 2019 at 9:18 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > > > > > On Sun, 1 Dec 2019 at 10:19, Marc-André Lureau > > > > > <marcandre.lureau@gmail.com> wrote: > > > > > > > > > > > > - "serial: register vmsd with DeviceClass" > > > > > > > > > > > > This is standard qdev-ification, however it breaks backward migration, > > > > > > but that's just how qdev_set_legacy_instance_id() works. > > > > > > > > > > I don't understand this part. Surely the whole point > > > > > of setting a legacy instance ID is exactly to preserve > > > > > migration compatibility? If it doesn't do that then what > > > > > does setting legacy ID value do? > > > > > > > > > > > > > It works in old->new direction only, because new code can match the > > > > legacy instance id. > > > > > > > > But when going from new->old, the legacy instance id is lost, as it > > > > uses new 0-based instance_id. > > > > > > I still don't understand. My mental model of the situation is: > > > > > > * in the old (current) version of the code, the instance ID > > > is some random thing resulting from what the old code does > > > > right > > > > > * in the new version of the code, we use qdev_set_legacy_instance_id, > > > and so instead of using the ID you'd naturally get as a > > > written-from-scratch qdev device, it uses the legacy value > > > you pass in > > > > no, it only sets the SaveStateEntry.alias_id, which is only used > > during incoming migration in find_se(). > > > > Iow, it only works old->new. > > > > > * thus the device/board in both old and new versions of QEMU > > > uses the same value and migration in both directions works > > > > sadly no > > > > > > > > I don't understand why we would ever be using a "new 0-based > > > instance_id" -- it seems to me that the whole point of setting > > > a legacy ID value is that we will use it always, and I don't > > > understand how the board code can know that it's going to be > > > the target of an old->new migration as opposed to being the > > > source of a new->old migration such that it can end up with > > > a different ID value in the latter case. > > > > The target will find the "legacy" alias with find_se() on incoming > > migration, but any new outgoing migration will use the new 0-based > > instance_id > > > > > > > > If qdev_set_legacy_instance_id() doesn't work the way I > > > think it does above, what *does* it do ? > > > > just set the old alias_id for incoming migration. > > > > David, is that correct? > > Yes, I think it is. > However, I'm curious which devices you're finding are explicitly setting > their id's; there aren't many - although there are some that probably > should! > For example, running an x86 image with: > -device isa-parallel,chardev=... -device isa-serial -device isa-serial -trace enable=qemu_loadvm_state_section_startfull > > shows: > qemu_loadvm_state_section_startfull(uint32_t section_id, const char *idstr, uint32_t instance_id, uint32_t version_id) "%u(%s) %u %u" > > 165217@1576179638.856300:qemu_loadvm_state_section_startfull 41(serial) 0 3 > 165217@1576179638.856307:qemu_loadvm_state_section_startfull 42(serial) 1 3 > 165217@1576179638.856311:qemu_loadvm_state_section_startfull 43(parallel_isa) 0 1 > > so those two serial devices are instances '0' and '1' I think by luck of > their command line order, rather than having specified their base > address (which would have been safer). None of the qdev devices using vmsd use a hard-coded instance id afaik. In device_set_realized(), vmstate_register_with_alias_id() is called with instance_id = -1, so it relies on calculate_new_instance_id(se->idstr) > > Dave > > > > > thanks > > > > > > -- > > Marc-André Lureau > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > -- Marc-André Lureau
On Sunday, December 1, 2019, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > - "RFC: mips/cps: fix setting saar property" > > Perhaps I should have used FIX instead of RFC, because this should > actually be a real fix. However I could use someone help to exercise > the code path. > > Marc-André, hi. There is a work in progress on fixing this. Can we in MIPS submit the fix independently, since it involves some additional pieces of code that are really deeply mips-specific? We acknowledge the bug, and want to develop the real solution. Can you simply skip this RFC patch in your series, since the issues will be handled separately in our patch, hopefully soon after the merge window is open? For all other mips parts of your series, you have my "reviewed-by"s , in case I forgot to send them explicitely. Regards, Aleksandar > - " >
Hi Aleksandar On Sun, Dec 1, 2019 at 4:15 PM Aleksandar Markovic <aleksandar.m.mail@gmail.com> wrote: > > > > On Sunday, December 1, 2019, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > >> >> - "RFC: mips/cps: fix setting saar property" >> >> Perhaps I should have used FIX instead of RFC, because this should >> actually be a real fix. However I could use someone help to exercise >> the code path. >> > > Marc-André, hi. > > There is a work in progress on fixing this. Can we in MIPS submit the fix independently, since it involves some additional pieces of code that are really deeply mips-specific? We acknowledge the bug, and want to develop the real solution. Can you simply skip this RFC patch in your series, since the issues will be handled separately in our patch, hopefully soon after the merge window is open? > > For all other mips parts of your series, you have my "reviewed-by"s , in case I forgot to send them explicitely. > This is a one-liner, and it is required to achieve the goal of the series, to remove PROP_PTR. If you prefer, I can instead comment the line with a FIXME, since it is apparently broken anyway? If you manage to get your fix merged earlier, then this patch can be dropped. Else, is it a problem for the later fixes? thanks -- Marc-André Lureau
On Sunday, December 1, 2019, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi Aleksandar > > On Sun, Dec 1, 2019 at 4:15 PM Aleksandar Markovic > <aleksandar.m.mail@gmail.com> wrote: > > > > > > > > On Sunday, December 1, 2019, Marc-André Lureau < > marcandre.lureau@gmail.com> wrote: > > > >> > >> - "RFC: mips/cps: fix setting saar property" > >> > >> Perhaps I should have used FIX instead of RFC, because this should > >> actually be a real fix. However I could use someone help to exercise > >> the code path. > >> > > > > Marc-André, hi. > > > > There is a work in progress on fixing this. Can we in MIPS submit the > fix independently, since it involves some additional pieces of code that > are really deeply mips-specific? We acknowledge the bug, and want to > develop the real solution. Can you simply skip this RFC patch in your > series, since the issues will be handled separately in our patch, hopefully > soon after the merge window is open? > > > > For all other mips parts of your series, you have my "reviewed-by"s , in > case I forgot to send them explicitely. > > > > This is a one-liner, and it is required to achieve the goal of the > series, to remove PROP_PTR. > > If you prefer, I can instead comment the line with a FIXME, since it > is apparently broken anyway? > > If you manage to get your fix merged earlier, then this patch can be > dropped. Else, is it a problem for the later fixes? > > OK, Marc-André, Please go ahead with this patch, so that the goal of the series is achieved, and we will later submitt a wider patch that will address the root cause. Just remove RFC from subject, everything else looks fine to me. You can add my "reviewed-by". Yours, Aleksandar > thanks > > > -- > Marc-André Lureau >
On Sunday, December 1, 2019, Aleksandar Markovic < aleksandar.m.mail@gmail.com> wrote: > > > On Sunday, December 1, 2019, Marc-André Lureau <marcandre.lureau@gmail.com> > wrote: > >> Hi Aleksandar >> >> On Sun, Dec 1, 2019 at 4:15 PM Aleksandar Markovic >> <aleksandar.m.mail@gmail.com> wrote: >> > >> > >> > >> > On Sunday, December 1, 2019, Marc-André Lureau < >> marcandre.lureau@gmail.com> wrote: >> > >> >> >> >> - "RFC: mips/cps: fix setting saar property" >> >> >> >> Perhaps I should have used FIX instead of RFC, because this should >> >> actually be a real fix. However I could use someone help to exercise >> >> the code path. >> >> >> > >> > Marc-André, hi. >> > >> > There is a work in progress on fixing this. Can we in MIPS submit the >> fix independently, since it involves some additional pieces of code that >> are really deeply mips-specific? We acknowledge the bug, and want to >> develop the real solution. Can you simply skip this RFC patch in your >> series, since the issues will be handled separately in our patch, hopefully >> soon after the merge window is open? >> > >> > For all other mips parts of your series, you have my "reviewed-by"s , >> in case I forgot to send them explicitely. >> > >> >> This is a one-liner, and it is required to achieve the goal of the >> series, to remove PROP_PTR. >> >> If you prefer, I can instead comment the line with a FIXME, since it >> is apparently broken anyway? >> >> If you manage to get your fix merged earlier, then this patch can be >> dropped. Else, is it a problem for the later fixes? >> >> > OK, Marc-André, > > Please go ahead with this patch, so that the goal of the series is > achieved, and we will later submitt a wider patch that will address the > root cause. Just remove RFC from subject, everything else looks fine to me. > You can add my "reviewed-by". > > I mean, yes, you are right, it is broken, with or without the patch, so go ahead, at least your series will fulfill its purpose, and I'll have enough time to integrate the fix later on, without interfering each other. Thanks for the series! Yours, Aleksandar > > > > >> thanks >> >> >> -- >> Marc-André Lureau >> >
© 2016 - 2024 Red Hat, Inc.