| Message ID | 20230918101736.23905-2-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | qdev: Ensure devices are fully realized when calling DeviceReset handler | expand |
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
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.
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; }
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(-)