ide: Linux reports drive diagnostic failures on boot

ide: Linux reports drive diagnostic failures on boot

[Mark Cave-Ayland]

During my latest OpenBIOS boot tests I've noticed the following IDE diagnostics 
failure message appearing in dmesg at Linux boot time when booting from CDROM on both 
SPARC64 and PPC:

[    9.347342] scsi host0: pata_cmd64x
[    9.369055] scsi host1: pata_cmd64x
[    9.371622] ata1: PATA max UDMA/33 cmd 0x1fe02008000 ctl 0x1fe02008080 bmdma 
0x1fe02008200 irq 8
[    9.372805] ata2: PATA max UDMA/33 cmd 0x1fe02008100 ctl 0x1fe02008180 bmdma 
0x1fe02008208 irq 8
[    9.711740] ata2.00: ATAPI: QEMU DVD-ROM, 2.5+, max UDMA/100
[    9.712591] ata2.00: Drive reports diagnostics failure. This may indicate a drive
[    9.713256] ata2.00: fault or invalid emulation. Contact drive vendor for information.
[    9.737677] ata2.00: configured for UDMA/33
[    9.790179] scsi 1:0:0:0: CD-ROM            QEMU     QEMU DVD-ROM     2.5+ PQ: 0 
ANSI: 5
[   10.381172] hme 0000:01:01.1 enp1s1f1: renamed from eth0
[   10.508819] sr 1:0:0:0: [sr0] scsi3-mmc drive: 4x/4x cd/rw xa/form2 tray
[   10.509805] cdrom: Uniform CD-ROM driver Revision: 3.20


A session with git bisect points to the following commit:

55adb3c45620c31f29978f209e2a44a08d34e2da is the first bad commit
commit 55adb3c45620c31f29978f209e2a44a08d34e2da
Author: John Snow <jsnow@redhat.com>
Date:   Fri Jul 24 01:23:00 2020 -0400

     ide: cancel pending callbacks on SRST

     The SRST implementation did not keep up with the rest of IDE; it is
     possible to perform a weak reset on an IDE device to remove the BSY/DRQ
     bits, and then issue writes to the control/device registers which can
     cause chaos with the state machine.

     Fix that by actually performing a real reset.

     Reported-by: Alexander Bulekov <alxndr@bu.edu>
     Fixes: https://bugs.launchpad.net/qemu/+bug/1878253
     Fixes: https://bugs.launchpad.net/qemu/+bug/1887303
     Fixes: https://bugs.launchpad.net/qemu/+bug/1887309
     Signed-off-by: John Snow <jsnow@redhat.com>

:040000 040000 70a7c1cfbafb22fa815d3ae4d7ed075ac3918188 
3f37762f20e9ca9d2874eaf819d7175a1dca1dd9 M      hw


John/Alexander: any chance you could take a look at this? The message above is really 
simple to reproduce under qemu-system-sparc64 using 
http://cdimage.debian.org/cdimage/ports/9.0/sparc64/iso-cd/debian-9.0-sparc64-NETINST-1.iso 
and the following command line:

./qemu-system-sparc64 \
     -cdrom debian-9.0-sparc64-NETINST-1.iso \
     -m 256 \
     -nographic \
     -boot d


ATB,

Mark.

Re: ide: Linux reports drive diagnostic failures on boot

[John Snow]

On 10/13/20 6:59 AM, Mark Cave-Ayland wrote:
> During my latest OpenBIOS boot tests I've noticed the following IDE 
> diagnostics failure message appearing in dmesg at Linux boot time when 
> booting from CDROM on both SPARC64 and PPC:
> 

Sorry for the inconvenience.

> [    9.347342] scsi host0: pata_cmd64x
> [    9.369055] scsi host1: pata_cmd64x
> [    9.371622] ata1: PATA max UDMA/33 cmd 0x1fe02008000 ctl 
> 0x1fe02008080 bmdma 0x1fe02008200 irq 8
> [    9.372805] ata2: PATA max UDMA/33 cmd 0x1fe02008100 ctl 
> 0x1fe02008180 bmdma 0x1fe02008208 irq 8
> [    9.711740] ata2.00: ATAPI: QEMU DVD-ROM, 2.5+, max UDMA/100
> [    9.712591] ata2.00: Drive reports diagnostics failure. This may 
> indicate a drive
> [    9.713256] ata2.00: fault or invalid emulation. Contact drive vendor 
> for information.
> [    9.737677] ata2.00: configured for UDMA/33
> [    9.790179] scsi 1:0:0:0: CD-ROM            QEMU     QEMU DVD-ROM     
> 2.5+ PQ: 0 ANSI: 5
> [   10.381172] hme 0000:01:01.1 enp1s1f1: renamed from eth0
> [   10.508819] sr 1:0:0:0: [sr0] scsi3-mmc drive: 4x/4x cd/rw xa/form2 tray
> [   10.509805] cdrom: Uniform CD-ROM driver Revision: 3.20
> 
> 
> A session with git bisect points to the following commit:
> 
> 55adb3c45620c31f29978f209e2a44a08d34e2da is the first bad commit
> commit 55adb3c45620c31f29978f209e2a44a08d34e2da
> Author: John Snow <jsnow@redhat.com>
> Date:   Fri Jul 24 01:23:00 2020 -0400
> 
>      ide: cancel pending callbacks on SRST
> 
>      The SRST implementation did not keep up with the rest of IDE; it is
>      possible to perform a weak reset on an IDE device to remove the 
> BSY/DRQ
>      bits, and then issue writes to the control/device registers which can
>      cause chaos with the state machine.
> 
>      Fix that by actually performing a real reset.
> 
>      Reported-by: Alexander Bulekov <alxndr@bu.edu>
>      Fixes: https://bugs.launchpad.net/qemu/+bug/1878253
>      Fixes: https://bugs.launchpad.net/qemu/+bug/1887303
>      Fixes: https://bugs.launchpad.net/qemu/+bug/1887309
>      Signed-off-by: John Snow <jsnow@redhat.com>
> 
> :040000 040000 70a7c1cfbafb22fa815d3ae4d7ed075ac3918188 
> 3f37762f20e9ca9d2874eaf819d7175a1dca1dd9 M      hw
> 
> 
> John/Alexander: any chance you could take a look at this? The message 
> above is really simple to reproduce under qemu-system-sparc64 using 
> http://cdimage.debian.org/cdimage/ports/9.0/sparc64/iso-cd/debian-9.0-sparc64-NETINST-1.iso 
> and the following command line:
> 
> ./qemu-system-sparc64 \
>      -cdrom debian-9.0-sparc64-NETINST-1.iso \
>      -m 256 \
>      -nographic \
>      -boot d
> 
> 
> ATB,
> 
> Mark.
> 

Shucks.

This patch happened because the old SRST code reset the IDE state 
machine (cleared the status register) but then didn't cancel any of the 
pending callbacks, so it was possible to shuffle the state machine off 
the rails onto junk data. Obviously bad.

Now, SRST actually cancels the callbacks which I thought would have been 
safe, but it's possible that doing a "real" reset here is touching more 
registers than it ought to.

Let's take a look at the linux source code ...

/* Let the user know. We don't want to disallow opens for 

    rescue purposes, or in case the vendor is just a blithering 

    idiot. Do this after the dev_config call as some controllers 

    with buggy firmware may want to avoid reporting false device 

    bugs */

Ah, always a nice day to be called an idiot. Thank you for your service, 
Alan Cox.

This message gets printed when ATA_HORKAGE_DIAGNOSTIC is set. 
libata-sff.c suggests this happens when the error register* comes back 
0x00 after an SRST.

(*I think that tf.feature is only feature on writes, but error on reads. 
Same address.)

Now, ide_reset -- which we use for initialization and resets both always 
sets the error register to 0x00. libata thinks that 0x00 means a failed 
diagnostics test, though.

This ought to be covered by ATA8-APT. Section 9.2, Software reset protocol.

Cliff notes:

- Host writes to SRST and waits for 5 μs.
- Both devices obey the SRST write, regardless of drive selection.
- Each device clears their registers and sets BSY (within 400ns.)
- Host clears SRST and waits for at least 2ms.
- Host polls devices, waiting for BSY to be 0.


device0 can set bit7 in the error register to 0 or 1, depending on the 
presence or absence of device1 and how it behaves following a diagnostic 
test.


Device 1 is absent: bit7 is cleared.
Device 1 is present:
   - If PDIAG- is asserted, bit7 is cleared.
   - If PDIAG- is not asserted within 31 seconds, bit7 is set.

Then, ah:

The EXECUTE DEVICE DIAGNOSTICS diagnostic code shall be placed in bits 
(6:0) of the Error register (See Clause 0). The device shall set the 
signature values (See Clause 0). The content of the Features register is 
undefined.

I got this pretty wrong. I'm seeing a few problems:

1. I thought SRST was triggered on the falling edge, but that's not 
entirely true. BSY should be set immediately and the SRST can begin as 
soon as possible. The device does not seem to have any interaction with 
the SRST bit being cleared from what I can tell.

2. We aren't running the diagnostic command, actually. That should fix 
this particular case. The old version of the code had an open-coded 
version of this, but it wasn't clear at the time this is what it was doing.

3. There are likely other things to handle relating to the 
presence/absence of device1 that we have never done for either version 
of the code.



Try this patch:


diff --git a/hw/ide/core.c b/hw/ide/core.c
index 693b352d5e..98cea7ad45 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2254,10 +2254,8 @@ static void ide_perform_srst(IDEState *s)
      /* Cancel PIO callback, reset registers/signature, etc */
      ide_reset(s);

-    if (s->drive_kind == IDE_CD) {
-        /* ATAPI drives do not set READY or SEEK */
-        s->status = 0x00;
-    }
+    /* perform diagnostic */
+    cmd_exec_dev_diagnostic(s, WIN_DIAGNOSE);
  }

  static void ide_bus_perform_srst(void *opaque)
@@ -2282,9 +2280,7 @@ void ide_ctrl_write(void *opaque, uint32_t addr, 
uint32_t val)

      /* Device0 and Device1 each have their own control register,
       * but QEMU models it as just one register in the controller. */
-    if ((bus->cmd & IDE_CTRL_RESET) &&
-        !(val & IDE_CTRL_RESET)) {
-        /* SRST triggers on falling edge */
+    if (!(bus->cmd & IDE_CTRL_RESET) && (val & IDE_CTRL_RESET)) {
          for (i = 0; i < 2; i++) {
              s = &bus->ifs[i];
              s->status |= BUSY_STAT;

Re: ide: Linux reports drive diagnostic failures on boot

[Mark Cave-Ayland]

On 13/10/2020 19:39, John Snow wrote:

> On 10/13/20 6:59 AM, Mark Cave-Ayland wrote:
>> During my latest OpenBIOS boot tests I've noticed the following IDE diagnostics 
>> failure message appearing in dmesg at Linux boot time when booting from CDROM on 
>> both SPARC64 and PPC:
>> 
> Sorry for the inconvenience.

Well it wasn't too bad - in my case the kernel was able to recover so it wasn't a 
complete showstopper for my tests. It seemed worth mentioning in case this causes 
other failures elsewhere though.

>> [    9.347342] scsi host0: pata_cmd64x
>> [    9.369055] scsi host1: pata_cmd64x
>> [    9.371622] ata1: PATA max UDMA/33 cmd 0x1fe02008000 ctl 0x1fe02008080 bmdma 
>> 0x1fe02008200 irq 8
>> [    9.372805] ata2: PATA max UDMA/33 cmd 0x1fe02008100 ctl 0x1fe02008180 bmdma 
>> 0x1fe02008208 irq 8
>> [    9.711740] ata2.00: ATAPI: QEMU DVD-ROM, 2.5+, max UDMA/100
>> [    9.712591] ata2.00: Drive reports diagnostics failure. This may indicate a drive
>> [    9.713256] ata2.00: fault or invalid emulation. Contact drive vendor for 
>> information.
>> [    9.737677] ata2.00: configured for UDMA/33
>> [    9.790179] scsi 1:0:0:0: CD-ROM            QEMU     QEMU DVD-ROM 2.5+ PQ: 0 
>> ANSI: 5
>> [   10.381172] hme 0000:01:01.1 enp1s1f1: renamed from eth0
>> [   10.508819] sr 1:0:0:0: [sr0] scsi3-mmc drive: 4x/4x cd/rw xa/form2 tray
>> [   10.509805] cdrom: Uniform CD-ROM driver Revision: 3.20
>>
>>
>> A session with git bisect points to the following commit:
>>
>> 55adb3c45620c31f29978f209e2a44a08d34e2da is the first bad commit
>> commit 55adb3c45620c31f29978f209e2a44a08d34e2da
>> Author: John Snow <jsnow@redhat.com>
>> Date:   Fri Jul 24 01:23:00 2020 -0400
>>
>>      ide: cancel pending callbacks on SRST
>>
>>      The SRST implementation did not keep up with the rest of IDE; it is
>>      possible to perform a weak reset on an IDE device to remove the BSY/DRQ
>>      bits, and then issue writes to the control/device registers which can
>>      cause chaos with the state machine.
>>
>>      Fix that by actually performing a real reset.
>>
>>      Reported-by: Alexander Bulekov <alxndr@bu.edu>
>>      Fixes: https://bugs.launchpad.net/qemu/+bug/1878253
>>      Fixes: https://bugs.launchpad.net/qemu/+bug/1887303
>>      Fixes: https://bugs.launchpad.net/qemu/+bug/1887309
>>      Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> :040000 040000 70a7c1cfbafb22fa815d3ae4d7ed075ac3918188 
>> 3f37762f20e9ca9d2874eaf819d7175a1dca1dd9 M      hw
>>
>>
>> John/Alexander: any chance you could take a look at this? The message above is 
>> really simple to reproduce under qemu-system-sparc64 using 
>> http://cdimage.debian.org/cdimage/ports/9.0/sparc64/iso-cd/debian-9.0-sparc64-NETINST-1.iso 
>> and the following command line:
>>
>> ./qemu-system-sparc64 \
>>      -cdrom debian-9.0-sparc64-NETINST-1.iso \
>>      -m 256 \
>>      -nographic \
>>      -boot d
>>
>>
>> ATB,
>>
>> Mark.
>>
> 
> Shucks.
> 
> This patch happened because the old SRST code reset the IDE state machine (cleared 
> the status register) but then didn't cancel any of the pending callbacks, so it was 
> possible to shuffle the state machine off the rails onto junk data. Obviously bad.
> 
> Now, SRST actually cancels the callbacks which I thought would have been safe, but 
> it's possible that doing a "real" reset here is touching more registers than it ought 
> to.
> 
> Let's take a look at the linux source code ...
> 
> /* Let the user know. We don't want to disallow opens for
>     rescue purposes, or in case the vendor is just a blithering
>     idiot. Do this after the dev_config call as some controllers
>     with buggy firmware may want to avoid reporting false device
>     bugs */
> 
> Ah, always a nice day to be called an idiot. Thank you for your service, Alan Cox.
> 
> This message gets printed when ATA_HORKAGE_DIAGNOSTIC is set. libata-sff.c suggests 
> this happens when the error register* comes back 0x00 after an SRST.
> 
> (*I think that tf.feature is only feature on writes, but error on reads. Same address.)
> 
> Now, ide_reset -- which we use for initialization and resets both always sets the 
> error register to 0x00. libata thinks that 0x00 means a failed diagnostics test, though.
> 
> This ought to be covered by ATA8-APT. Section 9.2, Software reset protocol.
> 
> Cliff notes:
> 
> - Host writes to SRST and waits for 5 μs.
> - Both devices obey the SRST write, regardless of drive selection.
> - Each device clears their registers and sets BSY (within 400ns.)
> - Host clears SRST and waits for at least 2ms.
> - Host polls devices, waiting for BSY to be 0.
> 
> 
> device0 can set bit7 in the error register to 0 or 1, depending on the presence or 
> absence of device1 and how it behaves following a diagnostic test.
> 
> 
> Device 1 is absent: bit7 is cleared.
> Device 1 is present:
>    - If PDIAG- is asserted, bit7 is cleared.
>    - If PDIAG- is not asserted within 31 seconds, bit7 is set.
> 
> Then, ah:
> 
> The EXECUTE DEVICE DIAGNOSTICS diagnostic code shall be placed in bits (6:0) of the 
> Error register (See Clause 0). The device shall set the signature values (See Clause 
> 0). The content of the Features register is undefined.
> 
> I got this pretty wrong. I'm seeing a few problems:
> 
> 1. I thought SRST was triggered on the falling edge, but that's not entirely true. 
> BSY should be set immediately and the SRST can begin as soon as possible. The device 
> does not seem to have any interaction with the SRST bit being cleared from what I can 
> tell.
> 
> 2. We aren't running the diagnostic command, actually. That should fix this 
> particular case. The old version of the code had an open-coded version of this, but 
> it wasn't clear at the time this is what it was doing.
> 
> 3. There are likely other things to handle relating to the presence/absence of 
> device1 that we have never done for either version of the code.
> 
> 
> 
> Try this patch:
> 
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 693b352d5e..98cea7ad45 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2254,10 +2254,8 @@ static void ide_perform_srst(IDEState *s)
>       /* Cancel PIO callback, reset registers/signature, etc */
>       ide_reset(s);
> 
> -    if (s->drive_kind == IDE_CD) {
> -        /* ATAPI drives do not set READY or SEEK */
> -        s->status = 0x00;
> -    }
> +    /* perform diagnostic */
> +    cmd_exec_dev_diagnostic(s, WIN_DIAGNOSE);
>   }
> 
>   static void ide_bus_perform_srst(void *opaque)
> @@ -2282,9 +2280,7 @@ void ide_ctrl_write(void *opaque, uint32_t addr, uint32_t val)
> 
>       /* Device0 and Device1 each have their own control register,
>        * but QEMU models it as just one register in the controller. */
> -    if ((bus->cmd & IDE_CTRL_RESET) &&
> -        !(val & IDE_CTRL_RESET)) {
> -        /* SRST triggers on falling edge */
> +    if (!(bus->cmd & IDE_CTRL_RESET) && (val & IDE_CTRL_RESET)) {
>           for (i = 0; i < 2; i++) {
>               s = &bus->ifs[i];
>               s->status |= BUSY_STAT;

I've just given this a quick boot test on qemu-system-sparc64 (both HD and CD) and it 
seems to fix the problem here - thanks!


ATB,

Mark.

Re: ide: Linux reports drive diagnostic failures on boot

[John Snow]

On 10/15/20 4:17 PM, Mark Cave-Ayland wrote:
> On 13/10/2020 19:39, John Snow wrote:
> 
>> On 10/13/20 6:59 AM, Mark Cave-Ayland wrote:
>>> During my latest OpenBIOS boot tests I've noticed the following IDE 
>>> diagnostics failure message appearing in dmesg at Linux boot time 
>>> when booting from CDROM on both SPARC64 and PPC:
>>>
>> Sorry for the inconvenience.
> 
> Well it wasn't too bad - in my case the kernel was able to recover so it 
> wasn't a complete showstopper for my tests. It seemed worth mentioning 
> in case this causes other failures elsewhere though.
> 
>>> [    9.347342] scsi host0: pata_cmd64x
>>> [    9.369055] scsi host1: pata_cmd64x
>>> [    9.371622] ata1: PATA max UDMA/33 cmd 0x1fe02008000 ctl 
>>> 0x1fe02008080 bmdma 0x1fe02008200 irq 8
>>> [    9.372805] ata2: PATA max UDMA/33 cmd 0x1fe02008100 ctl 
>>> 0x1fe02008180 bmdma 0x1fe02008208 irq 8
>>> [    9.711740] ata2.00: ATAPI: QEMU DVD-ROM, 2.5+, max UDMA/100
>>> [    9.712591] ata2.00: Drive reports diagnostics failure. This may 
>>> indicate a drive
>>> [    9.713256] ata2.00: fault or invalid emulation. Contact drive 
>>> vendor for information.
>>> [    9.737677] ata2.00: configured for UDMA/33
>>> [    9.790179] scsi 1:0:0:0: CD-ROM            QEMU     QEMU DVD-ROM 
>>> 2.5+ PQ: 0 ANSI: 5
>>> [   10.381172] hme 0000:01:01.1 enp1s1f1: renamed from eth0
>>> [   10.508819] sr 1:0:0:0: [sr0] scsi3-mmc drive: 4x/4x cd/rw 
>>> xa/form2 tray
>>> [   10.509805] cdrom: Uniform CD-ROM driver Revision: 3.20
>>>
>>>
>>> A session with git bisect points to the following commit:
>>>
>>> 55adb3c45620c31f29978f209e2a44a08d34e2da is the first bad commit
>>> commit 55adb3c45620c31f29978f209e2a44a08d34e2da
>>> Author: John Snow <jsnow@redhat.com>
>>> Date:   Fri Jul 24 01:23:00 2020 -0400
>>>
>>>      ide: cancel pending callbacks on SRST
>>>
>>>      The SRST implementation did not keep up with the rest of IDE; it is
>>>      possible to perform a weak reset on an IDE device to remove the 
>>> BSY/DRQ
>>>      bits, and then issue writes to the control/device registers 
>>> which can
>>>      cause chaos with the state machine.
>>>
>>>      Fix that by actually performing a real reset.
>>>
>>>      Reported-by: Alexander Bulekov <alxndr@bu.edu>
>>>      Fixes: https://bugs.launchpad.net/qemu/+bug/1878253
>>>      Fixes: https://bugs.launchpad.net/qemu/+bug/1887303
>>>      Fixes: https://bugs.launchpad.net/qemu/+bug/1887309
>>>      Signed-off-by: John Snow <jsnow@redhat.com>
>>>
>>> :040000 040000 70a7c1cfbafb22fa815d3ae4d7ed075ac3918188 
>>> 3f37762f20e9ca9d2874eaf819d7175a1dca1dd9 M      hw
>>>
>>>
>>> John/Alexander: any chance you could take a look at this? The message 
>>> above is really simple to reproduce under qemu-system-sparc64 using 
>>> http://cdimage.debian.org/cdimage/ports/9.0/sparc64/iso-cd/debian-9.0-sparc64-NETINST-1.iso 
>>> and the following command line:
>>>
>>> ./qemu-system-sparc64 \
>>>      -cdrom debian-9.0-sparc64-NETINST-1.iso \
>>>      -m 256 \
>>>      -nographic \
>>>      -boot d
>>>
>>>
>>> ATB,
>>>
>>> Mark.
>>>
>>
>> Shucks.
>>
>> This patch happened because the old SRST code reset the IDE state 
>> machine (cleared the status register) but then didn't cancel any of 
>> the pending callbacks, so it was possible to shuffle the state machine 
>> off the rails onto junk data. Obviously bad.
>>
>> Now, SRST actually cancels the callbacks which I thought would have 
>> been safe, but it's possible that doing a "real" reset here is 
>> touching more registers than it ought to.
>>
>> Let's take a look at the linux source code ...
>>
>> /* Let the user know. We don't want to disallow opens for
>>     rescue purposes, or in case the vendor is just a blithering
>>     idiot. Do this after the dev_config call as some controllers
>>     with buggy firmware may want to avoid reporting false device
>>     bugs */
>>
>> Ah, always a nice day to be called an idiot. Thank you for your 
>> service, Alan Cox.
>>
>> This message gets printed when ATA_HORKAGE_DIAGNOSTIC is set. 
>> libata-sff.c suggests this happens when the error register* comes back 
>> 0x00 after an SRST.
>>
>> (*I think that tf.feature is only feature on writes, but error on 
>> reads. Same address.)
>>
>> Now, ide_reset -- which we use for initialization and resets both 
>> always sets the error register to 0x00. libata thinks that 0x00 means 
>> a failed diagnostics test, though.
>>
>> This ought to be covered by ATA8-APT. Section 9.2, Software reset 
>> protocol.
>>
>> Cliff notes:
>>
>> - Host writes to SRST and waits for 5 μs.
>> - Both devices obey the SRST write, regardless of drive selection.
>> - Each device clears their registers and sets BSY (within 400ns.)
>> - Host clears SRST and waits for at least 2ms.
>> - Host polls devices, waiting for BSY to be 0.
>>
>>
>> device0 can set bit7 in the error register to 0 or 1, depending on the 
>> presence or absence of device1 and how it behaves following a 
>> diagnostic test.
>>
>>
>> Device 1 is absent: bit7 is cleared.
>> Device 1 is present:
>>    - If PDIAG- is asserted, bit7 is cleared.
>>    - If PDIAG- is not asserted within 31 seconds, bit7 is set.
>>
>> Then, ah:
>>
>> The EXECUTE DEVICE DIAGNOSTICS diagnostic code shall be placed in bits 
>> (6:0) of the Error register (See Clause 0). The device shall set the 
>> signature values (See Clause 0). The content of the Features register 
>> is undefined.
>>
>> I got this pretty wrong. I'm seeing a few problems:
>>
>> 1. I thought SRST was triggered on the falling edge, but that's not 
>> entirely true. BSY should be set immediately and the SRST can begin as 
>> soon as possible. The device does not seem to have any interaction 
>> with the SRST bit being cleared from what I can tell.
>>
>> 2. We aren't running the diagnostic command, actually. That should fix 
>> this particular case. The old version of the code had an open-coded 
>> version of this, but it wasn't clear at the time this is what it was 
>> doing.
>>
>> 3. There are likely other things to handle relating to the 
>> presence/absence of device1 that we have never done for either version 
>> of the code.
>>
>>
>>
>> Try this patch:
>>
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 693b352d5e..98cea7ad45 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -2254,10 +2254,8 @@ static void ide_perform_srst(IDEState *s)
>>       /* Cancel PIO callback, reset registers/signature, etc */
>>       ide_reset(s);
>>
>> -    if (s->drive_kind == IDE_CD) {
>> -        /* ATAPI drives do not set READY or SEEK */
>> -        s->status = 0x00;
>> -    }
>> +    /* perform diagnostic */
>> +    cmd_exec_dev_diagnostic(s, WIN_DIAGNOSE);
>>   }
>>
>>   static void ide_bus_perform_srst(void *opaque)
>> @@ -2282,9 +2280,7 @@ void ide_ctrl_write(void *opaque, uint32_t addr, 
>> uint32_t val)
>>
>>       /* Device0 and Device1 each have their own control register,
>>        * but QEMU models it as just one register in the controller. */
>> -    if ((bus->cmd & IDE_CTRL_RESET) &&
>> -        !(val & IDE_CTRL_RESET)) {
>> -        /* SRST triggers on falling edge */
>> +    if (!(bus->cmd & IDE_CTRL_RESET) && (val & IDE_CTRL_RESET)) {
>>           for (i = 0; i < 2; i++) {
>>               s = &bus->ifs[i];
>>               s->status |= BUSY_STAT;
> 
> I've just given this a quick boot test on qemu-system-sparc64 (both HD 
> and CD) and it seems to fix the problem here - thanks!
> 
> 
> ATB,
> 
> Mark.
> 

Thanks for testing! I'll spin this into a patch proper and add a T-B 
line for you here.

--js