* [PATCH 0/2] qdev: Ensure devices are fully realized when calling DeviceReset handler
@ 2023-09-18 10:17 Philippe Mathieu-Daudé
2023-09-18 10:17 ` [PATCH 1/2] hw/sd/omap_mmc: Do not reset SDCard until being fully realized Philippe Mathieu-Daudé
2023-09-18 10:17 ` [PATCH 2/2] qdev: Ensure devices are fully realized when calling DeviceReset handler Philippe Mathieu-Daudé
0 siblings, 2 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-09-18 10:17 UTC (permalink / raw)
To: qemu-devel
Cc: Richard Henderson, Daniel P . Berrange, Eduardo Habkost,
Alistair Francis, Paolo Bonzini, Anton Johansson,
Edgar E . Iglesias, Markus Armbruster, Alex Bennée,
Cédric Le Goater, Mark Cave-Ayland, Bernhard Beschow,
Peter Maydell, Alessandro Di Federico, qemu-arm, Luc Michel,
Philippe Mathieu-Daudé
In the process of clarifying QOM/QDev design, we want to enforce
the QDev state machine, in particular RESET can only happens *after*
REALIZE.
Besides CPUs (which are still particular), the only case I noticed
is in a non-QDev type (OMAP MMC) accessing a QDev one (SD/MMC card).
The first patch first handle the OMAP MMC case doing minor surgery
(passing a 'realized' state to omap_mmc_reset(), the function doing
reset out of normal QDev RESET tree propagation).
The second patch add an assertion in QDev core code, in order to
avoid further devices being added out of (QDev) order.
Please review,
Phil.
Philippe Mathieu-Daudé (2):
hw/sd/omap_mmc: Do not reset SDCard until being fully realized
qdev: Ensure devices are fully realized when calling DeviceReset
handler
include/hw/arm/omap.h | 2 +-
hw/arm/omap1.c | 2 +-
hw/arm/omap2.c | 2 +-
hw/core/qdev.c | 3 +++
hw/sd/omap_mmc.c | 21 ++++++++++++---------
5 files changed, 18 insertions(+), 12 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 1/2] hw/sd/omap_mmc: Do not reset SDCard until being fully realized 2023-09-18 10:17 [PATCH 0/2] qdev: Ensure devices are fully realized when calling DeviceReset handler Philippe Mathieu-Daudé @ 2023-09-18 10:17 ` Philippe Mathieu-Daudé 2023-09-18 11:00 ` Peter Maydell 2023-09-18 10:17 ` [PATCH 2/2] qdev: Ensure devices are fully realized when calling DeviceReset handler Philippe Mathieu-Daudé 1 sibling, 1 reply; 5+ messages in thread From: Philippe Mathieu-Daudé @ 2023-09-18 10:17 UTC (permalink / raw) To: qemu-devel Cc: Richard Henderson, Daniel P . Berrange, Eduardo Habkost, Alistair Francis, Paolo Bonzini, Anton Johansson, Edgar E . Iglesias, Markus Armbruster, Alex Bennée, Cédric Le Goater, Mark Cave-Ayland, Bernhard Beschow, Peter Maydell, Alessandro Di Federico, qemu-arm, Luc Michel, Philippe Mathieu-Daudé We shouldn't call QDev DeviceReset() before DeviceRealize(). Since the OMAP MMC model is not QDev'ified, it has to manually call the SDCard reset() handler. This breaks QDev assumptions that DeviceReset() is never called before a device is fully realized. In order to avoid that, pass a 'realized' argument to omap_mmc_reset(). All cases are explicit manual resets, except in omap_mmc_write() where we expect the sdcard to be realized. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/arm/omap.h | 2 +- hw/arm/omap1.c | 2 +- hw/arm/omap2.c | 2 +- hw/sd/omap_mmc.c | 21 ++++++++++++--------- 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h index 067e9419f7..d331467946 100644 --- a/include/hw/arm/omap.h +++ b/include/hw/arm/omap.h @@ -808,7 +808,7 @@ struct omap_mmc_s *omap_mmc_init(hwaddr base, struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta, BlockBackend *blk, qemu_irq irq, qemu_irq dma[], omap_clk fclk, omap_clk iclk); -void omap_mmc_reset(struct omap_mmc_s *s); +void omap_mmc_reset(struct omap_mmc_s *s, bool realized); void omap_mmc_handlers(struct omap_mmc_s *s, qemu_irq ro, qemu_irq cover); void omap_mmc_enable(struct omap_mmc_s *s, int enable); diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c index d5438156ee..3afeba6f86 100644 --- a/hw/arm/omap1.c +++ b/hw/arm/omap1.c @@ -3728,7 +3728,7 @@ static void omap1_mpu_reset(void *opaque) omap_uart_reset(mpu->uart[0]); omap_uart_reset(mpu->uart[1]); omap_uart_reset(mpu->uart[2]); - omap_mmc_reset(mpu->mmc); + omap_mmc_reset(mpu->mmc, false); omap_mpuio_reset(mpu->mpuio); omap_uwire_reset(mpu->microwire); omap_pwl_reset(mpu->pwl); diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c index d5a2ae7af6..ef9b0dd60a 100644 --- a/hw/arm/omap2.c +++ b/hw/arm/omap2.c @@ -2253,7 +2253,7 @@ static void omap2_mpu_reset(void *opaque) omap_uart_reset(mpu->uart[0]); omap_uart_reset(mpu->uart[1]); omap_uart_reset(mpu->uart[2]); - omap_mmc_reset(mpu->mmc); + omap_mmc_reset(mpu->mmc, false); omap_mcspi_reset(mpu->mcspi[0]); omap_mcspi_reset(mpu->mcspi[1]); cpu_reset(CPU(mpu->cpu)); diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c index edd3cf2a1e..3c906993eb 100644 --- a/hw/sd/omap_mmc.c +++ b/hw/sd/omap_mmc.c @@ -287,7 +287,7 @@ static void omap_mmc_pseudo_reset(struct omap_mmc_s *host) host->fifo_len = 0; } -void omap_mmc_reset(struct omap_mmc_s *host) +void omap_mmc_reset(struct omap_mmc_s *host, bool realized) { host->last_cmd = 0; memset(host->rsp, 0, sizeof(host->rsp)); @@ -314,11 +314,14 @@ void omap_mmc_reset(struct omap_mmc_s *host) omap_mmc_pseudo_reset(host); - /* Since we're still using the legacy SD API the card is not plugged - * into any bus, and we must reset it manually. When omap_mmc is - * QOMified this must move into the QOM reset function. - */ - device_cold_reset(DEVICE(host->card)); + if (realized) { + /* + * Since we're still using the legacy SD API the card is not plugged + * into any bus, and we must reset it manually. When omap_mmc is + * QOMified this must move into the QOM reset function. + */ + device_cold_reset(DEVICE(host->card)); + } } static uint64_t omap_mmc_read(void *opaque, hwaddr offset, unsigned size) @@ -556,7 +559,7 @@ static void omap_mmc_write(void *opaque, hwaddr offset, break; case 0x64: /* MMC_SYSC */ if (value & (1 << 2)) /* SRTS */ - omap_mmc_reset(s); + omap_mmc_reset(s, true); break; case 0x68: /* MMC_SYSS */ OMAP_RO_REG(offset); @@ -613,7 +616,7 @@ struct omap_mmc_s *omap_mmc_init(hwaddr base, exit(1); } - omap_mmc_reset(s); + omap_mmc_reset(s, false); return s; } @@ -643,7 +646,7 @@ struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta, s->cdet = qemu_allocate_irq(omap_mmc_cover_cb, s, 0); sd_set_cb(s->card, NULL, s->cdet); - omap_mmc_reset(s); + omap_mmc_reset(s, false); return s; } -- 2.41.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] hw/sd/omap_mmc: Do not reset SDCard until being fully realized 2023-09-18 10:17 ` [PATCH 1/2] hw/sd/omap_mmc: Do not reset SDCard until being fully realized Philippe Mathieu-Daudé @ 2023-09-18 11:00 ` Peter Maydell 2023-09-18 15:22 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 5+ messages in thread From: Peter Maydell @ 2023-09-18 11:00 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: qemu-devel, Richard Henderson, Daniel P . Berrange, Eduardo Habkost, Alistair Francis, Paolo Bonzini, Anton Johansson, Edgar E . Iglesias, Markus Armbruster, Alex Bennée, Cédric Le Goater, Mark Cave-Ayland, Bernhard Beschow, Alessandro Di Federico, qemu-arm, Luc Michel On Mon, 18 Sept 2023 at 11:17, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > We shouldn't call QDev DeviceReset() before DeviceRealize(). > > Since the OMAP MMC model is not QDev'ified, it has to manually > call the SDCard reset() handler. This breaks QDev assumptions > that DeviceReset() is never called before a device is fully > realized. In order to avoid that, pass a 'realized' argument > to omap_mmc_reset(). All cases are explicit manual resets, > except in omap_mmc_write() where we expect the sdcard to be > realized. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > include/hw/arm/omap.h | 2 +- > hw/arm/omap1.c | 2 +- > hw/arm/omap2.c | 2 +- > hw/sd/omap_mmc.c | 21 ++++++++++++--------- > 4 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h > index 067e9419f7..d331467946 100644 > --- a/include/hw/arm/omap.h > +++ b/include/hw/arm/omap.h > @@ -808,7 +808,7 @@ struct omap_mmc_s *omap_mmc_init(hwaddr base, > struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta, > BlockBackend *blk, qemu_irq irq, qemu_irq dma[], > omap_clk fclk, omap_clk iclk); > -void omap_mmc_reset(struct omap_mmc_s *s); > +void omap_mmc_reset(struct omap_mmc_s *s, bool realized); > void omap_mmc_handlers(struct omap_mmc_s *s, qemu_irq ro, qemu_irq cover); > void omap_mmc_enable(struct omap_mmc_s *s, int enable); > > diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c > index d5438156ee..3afeba6f86 100644 > --- a/hw/arm/omap1.c > +++ b/hw/arm/omap1.c > @@ -3728,7 +3728,7 @@ static void omap1_mpu_reset(void *opaque) > omap_uart_reset(mpu->uart[0]); > omap_uart_reset(mpu->uart[1]); > omap_uart_reset(mpu->uart[2]); > - omap_mmc_reset(mpu->mmc); > + omap_mmc_reset(mpu->mmc, false); > omap_mpuio_reset(mpu->mpuio); > omap_uwire_reset(mpu->microwire); > omap_pwl_reset(mpu->pwl); > diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c > index d5a2ae7af6..ef9b0dd60a 100644 > --- a/hw/arm/omap2.c > +++ b/hw/arm/omap2.c > @@ -2253,7 +2253,7 @@ static void omap2_mpu_reset(void *opaque) > omap_uart_reset(mpu->uart[0]); > omap_uart_reset(mpu->uart[1]); > omap_uart_reset(mpu->uart[2]); > - omap_mmc_reset(mpu->mmc); > + omap_mmc_reset(mpu->mmc, false); > omap_mcspi_reset(mpu->mcspi[0]); > omap_mcspi_reset(mpu->mcspi[1]); > cpu_reset(CPU(mpu->cpu)); These are reset functions registered via qemu_register_reset(). They should be OK to assume the SD card is realized. This matters, because this is the only place that the SD card will get reset -- as the comment in omap_mmc_reset() notes, the SD card object isn't going to be plugged into any bus, so it won't get auto-reset when the simulation starts, and these reset function are the place that does the manual reset. > diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c > index edd3cf2a1e..3c906993eb 100644 > --- a/hw/sd/omap_mmc.c > +++ b/hw/sd/omap_mmc.c > @@ -287,7 +287,7 @@ static void omap_mmc_pseudo_reset(struct omap_mmc_s *host) > host->fifo_len = 0; > } > > -void omap_mmc_reset(struct omap_mmc_s *host) > +void omap_mmc_reset(struct omap_mmc_s *host, bool realized) > { > host->last_cmd = 0; > memset(host->rsp, 0, sizeof(host->rsp)); > @@ -314,11 +314,14 @@ void omap_mmc_reset(struct omap_mmc_s *host) > > omap_mmc_pseudo_reset(host); > > - /* Since we're still using the legacy SD API the card is not plugged > - * into any bus, and we must reset it manually. When omap_mmc is > - * QOMified this must move into the QOM reset function. > - */ > - device_cold_reset(DEVICE(host->card)); > + if (realized) { > + /* > + * Since we're still using the legacy SD API the card is not plugged > + * into any bus, and we must reset it manually. When omap_mmc is > + * QOMified this must move into the QOM reset function. > + */ > + device_cold_reset(DEVICE(host->card)); > + } > } > > static uint64_t omap_mmc_read(void *opaque, hwaddr offset, unsigned size) > @@ -556,7 +559,7 @@ static void omap_mmc_write(void *opaque, hwaddr offset, > break; > case 0x64: /* MMC_SYSC */ > if (value & (1 << 2)) /* SRTS */ > - omap_mmc_reset(s); > + omap_mmc_reset(s, true); > break; > case 0x68: /* MMC_SYSS */ > OMAP_RO_REG(offset); > @@ -613,7 +616,7 @@ struct omap_mmc_s *omap_mmc_init(hwaddr base, > exit(1); > } > > - omap_mmc_reset(s); > + omap_mmc_reset(s, false); > > return s; > } > @@ -643,7 +646,7 @@ struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta, > s->cdet = qemu_allocate_irq(omap_mmc_cover_cb, s, 0); > sd_set_cb(s->card, NULL, s->cdet); > > - omap_mmc_reset(s); > + omap_mmc_reset(s, false); These calls from omap_mmc_init() are probably safe to remove, but I don't understand why they result in our resetting a non-realized SD card object. In both cases, the call happens after we call sd_init(). sd_init() both creates and realizes the SD card, so it should be realized at the point when it gets reset. In a truly ideal world we would QOMify the omap-mmc device: it is the only remaining user of the legacy sd_init function... thanks -- PMM ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] hw/sd/omap_mmc: Do not reset SDCard until being fully realized 2023-09-18 11:00 ` Peter Maydell @ 2023-09-18 15:22 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 5+ messages in thread From: Philippe Mathieu-Daudé @ 2023-09-18 15:22 UTC (permalink / raw) To: Peter Maydell Cc: qemu-devel, Richard Henderson, Daniel P . Berrange, Eduardo Habkost, Alistair Francis, Paolo Bonzini, Anton Johansson, Edgar E . Iglesias, Markus Armbruster, Alex Bennée, Cédric Le Goater, Mark Cave-Ayland, Bernhard Beschow, Alessandro Di Federico, qemu-arm, Luc Michel On 18/9/23 13:00, Peter Maydell wrote: > On Mon, 18 Sept 2023 at 11:17, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> We shouldn't call QDev DeviceReset() before DeviceRealize(). >> >> Since the OMAP MMC model is not QDev'ified, it has to manually >> call the SDCard reset() handler. This breaks QDev assumptions >> that DeviceReset() is never called before a device is fully >> realized. In order to avoid that, pass a 'realized' argument >> to omap_mmc_reset(). All cases are explicit manual resets, >> except in omap_mmc_write() where we expect the sdcard to be >> realized. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> include/hw/arm/omap.h | 2 +- >> hw/arm/omap1.c | 2 +- >> hw/arm/omap2.c | 2 +- >> hw/sd/omap_mmc.c | 21 ++++++++++++--------- >> 4 files changed, 15 insertions(+), 12 deletions(-) >> >> diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h >> index 067e9419f7..d331467946 100644 >> --- a/include/hw/arm/omap.h >> +++ b/include/hw/arm/omap.h >> @@ -808,7 +808,7 @@ struct omap_mmc_s *omap_mmc_init(hwaddr base, >> struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta, >> BlockBackend *blk, qemu_irq irq, qemu_irq dma[], >> omap_clk fclk, omap_clk iclk); >> -void omap_mmc_reset(struct omap_mmc_s *s); >> +void omap_mmc_reset(struct omap_mmc_s *s, bool realized); >> void omap_mmc_handlers(struct omap_mmc_s *s, qemu_irq ro, qemu_irq cover); >> void omap_mmc_enable(struct omap_mmc_s *s, int enable); >> >> diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c >> index d5438156ee..3afeba6f86 100644 >> --- a/hw/arm/omap1.c >> +++ b/hw/arm/omap1.c >> @@ -3728,7 +3728,7 @@ static void omap1_mpu_reset(void *opaque) >> omap_uart_reset(mpu->uart[0]); >> omap_uart_reset(mpu->uart[1]); >> omap_uart_reset(mpu->uart[2]); >> - omap_mmc_reset(mpu->mmc); >> + omap_mmc_reset(mpu->mmc, false); >> omap_mpuio_reset(mpu->mpuio); >> omap_uwire_reset(mpu->microwire); >> omap_pwl_reset(mpu->pwl); >> diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c >> index d5a2ae7af6..ef9b0dd60a 100644 >> --- a/hw/arm/omap2.c >> +++ b/hw/arm/omap2.c >> @@ -2253,7 +2253,7 @@ static void omap2_mpu_reset(void *opaque) >> omap_uart_reset(mpu->uart[0]); >> omap_uart_reset(mpu->uart[1]); >> omap_uart_reset(mpu->uart[2]); >> - omap_mmc_reset(mpu->mmc); >> + omap_mmc_reset(mpu->mmc, false); >> omap_mcspi_reset(mpu->mcspi[0]); >> omap_mcspi_reset(mpu->mcspi[1]); >> cpu_reset(CPU(mpu->cpu)); > > These are reset functions registered via qemu_register_reset(). > They should be OK to assume the SD card is realized. > This matters, because this is the only place that the SD > card will get reset -- as the comment in omap_mmc_reset() notes, > the SD card object isn't going to be plugged into any bus, so > it won't get auto-reset when the simulation starts, and these > reset function are the place that does the manual reset. > >> diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c >> index edd3cf2a1e..3c906993eb 100644 >> --- a/hw/sd/omap_mmc.c >> +++ b/hw/sd/omap_mmc.c >> @@ -287,7 +287,7 @@ static void omap_mmc_pseudo_reset(struct omap_mmc_s *host) >> host->fifo_len = 0; >> } >> >> -void omap_mmc_reset(struct omap_mmc_s *host) >> +void omap_mmc_reset(struct omap_mmc_s *host, bool realized) >> { >> host->last_cmd = 0; >> memset(host->rsp, 0, sizeof(host->rsp)); >> @@ -314,11 +314,14 @@ void omap_mmc_reset(struct omap_mmc_s *host) >> >> omap_mmc_pseudo_reset(host); >> >> - /* Since we're still using the legacy SD API the card is not plugged >> - * into any bus, and we must reset it manually. When omap_mmc is >> - * QOMified this must move into the QOM reset function. >> - */ >> - device_cold_reset(DEVICE(host->card)); >> + if (realized) { >> + /* >> + * Since we're still using the legacy SD API the card is not plugged >> + * into any bus, and we must reset it manually. When omap_mmc is >> + * QOMified this must move into the QOM reset function. >> + */ >> + device_cold_reset(DEVICE(host->card)); >> + } >> } >> >> static uint64_t omap_mmc_read(void *opaque, hwaddr offset, unsigned size) >> @@ -556,7 +559,7 @@ static void omap_mmc_write(void *opaque, hwaddr offset, >> break; >> case 0x64: /* MMC_SYSC */ >> if (value & (1 << 2)) /* SRTS */ >> - omap_mmc_reset(s); >> + omap_mmc_reset(s, true); >> break; >> case 0x68: /* MMC_SYSS */ >> OMAP_RO_REG(offset); >> @@ -613,7 +616,7 @@ struct omap_mmc_s *omap_mmc_init(hwaddr base, >> exit(1); >> } >> >> - omap_mmc_reset(s); >> + omap_mmc_reset(s, false); >> >> return s; >> } >> @@ -643,7 +646,7 @@ struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta, >> s->cdet = qemu_allocate_irq(omap_mmc_cover_cb, s, 0); >> sd_set_cb(s->card, NULL, s->cdet); >> >> - omap_mmc_reset(s); >> + omap_mmc_reset(s, false); > > These calls from omap_mmc_init() are probably safe to remove, but I > don't understand why they result in our resetting a non-realized > SD card object. In both cases, the call happens after we call > sd_init(). sd_init() both creates and realizes the SD card, so > it should be realized at the point when it gets reset. Indeed, sd_realize() is called: (lldb) bt * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1 * frame #0: 0x1001f047c qemu-system-arm`sd_realize [inlined] SD_CARD(obj=0x103b339e0) at sd.h:94:1 frame #1: 0x1001f047c qemu-system-arm`sd_realize(dev=0x103b339e0, errp=0x16fdfeaa0) at sd.c:2193:19 frame #2: 0x1001f03e8 qemu-system-arm`sd_init(blk=<unavailable>, is_spi=<unavailable>) at sd.c:765:5 frame #3: 0x1001fa1c8 qemu-system-arm`omap2_mmc_init(ta=0x104838060, blk=<unavailable>, irq=0x0000600001720080, dma=0x103b28c88, fclk=0x1048333f0, iclk=<unavailable>) at omap_mmc.c:638:15 frame #4: 0x10033cf40 qemu-system-arm`omap2420_mpu_init(sdram=<unavailable>, cpu_type=<unavailable>) at omap2.c:2487:14 frame #5: 0x10031fe6c qemu-system-arm`n8x0_init(machine=0x103b1fc00, binfo=0x100dbf5a8, model=800) at nseries.c:1317:14 frame #6: 0x100093d48 qemu-system-arm`machine_run_board_init(machine=0x103b1fc00, mem_path=<unavailable>, errp=<unavailable>) at machine.c:1469:5 frame #7: 0x100299fe8 qemu-system-arm`qmp_x_exit_preconfig [inlined] qemu_init_board at vl.c:2580:5 frame #8: 0x100299fb4 qemu-system-arm`qmp_x_exit_preconfig(errp=<unavailable>) at vl.c:2671:5 frame #9: 0x10029d77c qemu-system-arm`qemu_init(argc=<unavailable>, argv=<unavailable>) at vl.c:3721:9 frame #10: 0x1004df880 qemu-system-arm`main(argc=<unavailable>, argv=<unavailable>) at main.c:47:5 frame #11: 0x1884b7f28 dyld`start + 2236 But then I'm getting: frame #3: 0x18871ce44 libsystem_c.dylib`__assert_rtn + 272 * frame #4: 0x1006dbbe4 qemu-system-arm`device_cold_reset.cold.1 at qdev.c:256:5 frame #5: 0x1004e1c44 qemu-system-arm`device_cold_reset(dev=0x10347dde0) at qdev.c:256:5 frame #6: 0x1001fa290 qemu-system-arm`omap2_mmc_init [inlined] omap_mmc_reset(host=0x10347d960) at omap_mmc.c:321:5 frame #7: 0x1001fa1fc qemu-system-arm`omap2_mmc_init(ta=0x104025860, blk=<unavailable>, irq=0x000060000171d580, dma=0x103472f38, fclk=0x1040119f0, iclk=<unavailable>) at omap_mmc.c:646:5 frame #8: 0x10033cf40 qemu-system-arm`omap2420_mpu_init(sdram=<unavailable>, cpu_type=<unavailable>) at omap2.c:2487:14 frame #9: 0x10031fe6c qemu-system-arm`n8x0_init(machine=0x1033dec60, binfo=0x100dbf5a8, model=800) at nseries.c:1317:14 frame #10: 0x100093d48 qemu-system-arm`machine_run_board_init(machine=0x1033dec60, mem_path=<unavailable>, errp=<unavailable>) at machine.c:1469:5 frame #11: 0x100299fe8 qemu-system-arm`qmp_x_exit_preconfig [inlined] qemu_init_board at vl.c:2580:5 frame #12: 0x100299fb4 qemu-system-arm`qmp_x_exit_preconfig(errp=<unavailable>) at vl.c:2671:5 frame #13: 0x10029d77c qemu-system-arm`qemu_init(argc=<unavailable>, argv=<unavailable>) at vl.c:3721:9 frame #14: 0x1004df880 qemu-system-arm`main(argc=<unavailable>, argv=<unavailable>) at main.c:47:5 Because this isn't a full qdev_realize() call. Then when using: -- >8 -- diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 4823befdef..db7d644fb7 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -763,7 +763,7 @@ SDState *sd_init(BlockBackend *blk, bool is_spi) object_ref(obj); object_unparent(obj); sd_realize(dev, &err); - if (err) { + if (err || !object_property_set_bool(OBJECT(dev), "realized", true, &err)) { error_reportf_err(err, "sd_init failed: "); return NULL; } --- I get: frame #3: 0x18871ce44 libsystem_c.dylib`__assert_rtn + 272 * frame #4: 0x1006dbcac qemu-system-arm`qdev_assert_realized_properly_cb.cold.2 at qdev.c:320:9 frame #5: 0x1004e1e04 qemu-system-arm`qdev_assert_realized_properly_cb(obj=0x10337e2e0, opaque=0x000000000) at qdev.c:320:9 frame #6: 0x1004e7a8c qemu-system-arm`do_object_child_foreach(obj=0x0000600000c0df50, fn=(qemu-system-arm`qdev_assert_realized_properly_cb at qdev.c:313), opaque=0x000000000, recurse=true) at object.c:1120:19 frame #7: 0x1004e7aa8 qemu-system-arm`do_object_child_foreach(obj=0x1039de470, fn=(qemu-system-arm`qdev_assert_realized_properly_cb at qdev.c:313), opaque=0x000000000, recurse=true) at object.c:1125:23 frame #8: 0x1004e7aa8 qemu-system-arm`do_object_child_foreach(obj=<unavailable>, fn=(qemu-system-arm`qdev_assert_realized_properly_cb at qdev.c:313), opaque=0x000000000, recurse=true) at object.c:1125:23 frame #9: 0x1004e7ae8 qemu-system-arm`object_child_foreach_recursive(obj=<unavailable>, fn=<unavailable>, opaque=<unavailable>) at object.c:1145:12 [artificial] frame #10: 0x1004e1d70 qemu-system-arm`qdev_assert_realized_properly at qdev.c:327:5 [artificial] frame #11: 0x100093fd4 qemu-system-arm`qdev_machine_creation_done at machine.c:1503:5 frame #12: 0x10029a12c qemu-system-arm`qmp_x_exit_preconfig [inlined] qemu_machine_creation_done at vl.c:2644:5 frame #13: 0x10029a0c8 qemu-system-arm`qmp_x_exit_preconfig(errp=<unavailable>) at vl.c:2673:5 frame #14: 0x10029d77c qemu-system-arm`qemu_init(argc=<unavailable>, argv=<unavailable>) at vl.c:3721:9 frame #15: 0x1004df880 qemu-system-arm`main(argc=<unavailable>, argv=<unavailable>) at main.c:47:5 > In a truly ideal world we would QOMify the omap-mmc device: it > is the only remaining user of the legacy sd_init function... Yeah, and I remember I told you I already did it 3 years ago :) IIRC it was hard to just convert one device, and I wasn't sure it was worth it. Anyhow, I'll check my backups in a pair of weeks. Thanks for the review, Phil. ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] qdev: Ensure devices are fully realized when calling DeviceReset handler 2023-09-18 10:17 [PATCH 0/2] qdev: Ensure devices are fully realized when calling DeviceReset handler Philippe Mathieu-Daudé 2023-09-18 10:17 ` [PATCH 1/2] hw/sd/omap_mmc: Do not reset SDCard until being fully realized Philippe Mathieu-Daudé @ 2023-09-18 10:17 ` Philippe Mathieu-Daudé 1 sibling, 0 replies; 5+ messages in thread From: Philippe Mathieu-Daudé @ 2023-09-18 10:17 UTC (permalink / raw) To: qemu-devel Cc: Richard Henderson, Daniel P . Berrange, Eduardo Habkost, Alistair Francis, Paolo Bonzini, Anton Johansson, Edgar E . Iglesias, Markus Armbruster, Alex Bennée, Cédric Le Goater, Mark Cave-Ayland, Bernhard Beschow, Peter Maydell, Alessandro Di Federico, qemu-arm, Luc Michel, Philippe Mathieu-Daudé We shouldn't call a DeviceReset() handler on unrealized device. In order to enforce that, add an assertion. CPU devices are a bit particular because we manually reset them in various places, so corner case them out for now. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/core/qdev.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 43d863b0c5..c51b4c98de 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -252,6 +252,8 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id, void device_cold_reset(DeviceState *dev) { + /* TODO: Also cover CPUs once we removed manual calls to cpu_reset() */ + assert(object_dynamic_cast(OBJECT(dev), TYPE_CPU) || dev->realized); resettable_reset(OBJECT(dev), RESET_TYPE_COLD); } @@ -280,6 +282,7 @@ static void device_reset_child_foreach(Object *obj, ResettableChildCallback cb, bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp) { assert(!dev->realized && !dev->parent_bus); + assert(!device_is_in_reset(dev)); if (bus) { if (!qdev_set_parent_bus(dev, bus, errp)) { -- 2.41.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-09-18 15:23 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-09-18 10:17 [PATCH 0/2] qdev: Ensure devices are fully realized when calling DeviceReset handler Philippe Mathieu-Daudé 2023-09-18 10:17 ` [PATCH 1/2] hw/sd/omap_mmc: Do not reset SDCard until being fully realized Philippe Mathieu-Daudé 2023-09-18 11:00 ` Peter Maydell 2023-09-18 15:22 ` Philippe Mathieu-Daudé 2023-09-18 10:17 ` [PATCH 2/2] qdev: Ensure devices are fully realized when calling DeviceReset handler Philippe Mathieu-Daudé
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).