Re: [PATCH 1/2] hw/sd/omap_mmc: Do not reset SDCard until being fully realized

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.