CD-ROM boot broken on current revision?

Hi,

I'm a new subscriber to seabios.org. Not much exp. with mailing lists nor C++, so feel free to straighten me out as 
necessary. I found out an issue: Booting from CD-ROM doesn't work anymore, though related make options have been set. I 
tried to debug it a little bit. To me it looks that order changes related to media sensing/capacity request broke it up.

In 1.6.3,  cdb_read_capacity(line 204 in src/cdrom.c) takes place before cdb_get_sense(line 209) resulting in a break.

In current revision, this has been changed, has it?
cdrom_boot(src/cdrom.cline 188) calls scsi_is_ready(line 197), which looks similar to atapi_is_ready from 1.6.3/src/cdrom.c.

However: scsi_is_ready is missing

struct cdbres_read_capacity info;
   ret = cdb_read_capacity(op, &info);
   if (!ret)
   // Success
   break;

contrary to 1.6.3. In line cdb_get_sense returns 0xC with a medium in the drive, but this seems to result into an 
endless loop due to continue;

For me, I added above snippet to blockcmd.c right before ret = cdb_get_sense(..) and it worked.
Later CD-ROM boots using boot managers(e.g. grub4dos or PloP) work fine without changes, though.

Debugging was done in Qemu 1.0.1. Since I'm on Windows, I used this build:
http://lassauge.free.fr/qemu/release/Qemu-1.0.1-windows.zip

Please confirm or advise.

Kind Regards,
fellaw

[Paolo Bonzini]

Il 18/03/2012 22:47, [e-mail] ha scritto:
> Hi,
>
> I'm a new subscriber to seabios.org. Not much exp. with mailing lists
> nor C++, so feel free to straighten me out as necessary. I found out an
> issue: Booting from CD-ROM doesn't work anymore, though related make
> options have been set. I tried to debug it a little bit. To me it looks
> that order changes related to media sensing/capacity request broke it up.
>
> In 1.6.3,  cdb_read_capacity(line 204 in src/cdrom.c) takes place before
> cdb_get_sense(line 209) resulting in a break.
>
> In current revision, this has been changed, has it?
> cdrom_boot(src/cdrom.cline 188) calls scsi_is_ready(line 197), which
> looks similar to atapi_is_ready from 1.6.3/src/cdrom.c.
>
> However: scsi_is_ready is missing
>
> struct cdbres_read_capacity info;
>   ret = cdb_read_capacity(op, &info);
>   if (!ret)
>   // Success
>   break;
>
> contrary to 1.6.3. In line cdb_get_sense returns 0xC with a medium in
> the drive, but this seems to result into an endless loop due to continue;
>
> For me, I added above snippet to blockcmd.c right before ret =
> cdb_get_sense(..) and it worked.
> Later CD-ROM boots using boot managers(e.g. grub4dos or PloP) work fine
> without changes, though.
>
> Debugging was done in Qemu 1.0.1. Since I'm on Windows, I used this build:
> http://lassauge.free.fr/qemu/release/Qemu-1.0.1-windows.zip
>
> Please confirm or advise.

This works for me, but only after a time out on test_unit_ready which
more or less confirms your bug.  I'll reply to this message with a patch
that fixes it for me.  Can you please test it?

Paolo

[PATCH] ata: send TEST UNIT READY correctly

[Paolo Bonzini]

The ATAPI driver does not need to support writes, but it does needs to
avoid the PIO transfer and DRQ check when TEST UNIT READY is sent.
Since TEST UNIT READY has no payload, checking for not busy is enough.

This fixes a timeout when booting from CD/DVD, which [e-mail]
reported to cause boot failures.

Signed-off-by: Paolo Bonzini 
---
 src/ata.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/ata.c b/src/ata.c
index 76e4f20..c37691a 100644
--- a/src/ata.c
+++ b/src/ata.c
@@ -645,13 +645,15 @@ atapi_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize)
         ret = -2;
         goto fail;
     }
-    if (!(status & ATA_CB_STAT_DRQ)) {
-        dprintf(6, "send_atapi_cmd : DRQ not set (status %02x)\n", status);
-        ret = -3;
-        goto fail;
-    }
+    if (blocksize) {
+        if (!(status & ATA_CB_STAT_DRQ)) {
+            dprintf(6, "send_atapi_cmd : DRQ not set (status %02x)\n", status);
+            ret = -3;
+            goto fail;
+        }

-    ret = ata_pio_transfer(op, 0, blocksize);
+        ret = ata_pio_transfer(op, 0, blocksize);
+    }

fail:
     // Enable interrupts

[PATCH] ata: send TEST UNIT READY correctly

Confirmed working.

Paolo Bonzini schrieb:
> The ATAPI driver does not need to support writes, but it does needs to
> avoid the PIO transfer and DRQ check when TEST UNIT READY is sent.
> Since TEST UNIT READY has no payload, checking for not busy is enough.
>
> This fixes a timeout when booting from CD/DVD, which [e-mail]
> reported to cause boot failures.
> Signed-off-by: Paolo Bonzini
>
> src/ata.c |   14 ++++++++------
>   1 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/src/ata.c b/src/ata.c
> index 76e4f20..c37691a 100644
> --- a/src/ata.c
> +++ b/src/ata.c
> @@ -645,13 +645,15 @@ atapi_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize)
>           ret = -2;
>           goto fail;
>       }
> if (!(status&  ATA_CB_STAT_DRQ)) {
>    dprintf(6, "send_atapi_cmd : DRQ not set (status %02x)\n", status);
>    ret = -3;
>    goto fail;
> }
> if (blocksize) {
>    if (!(status&  ATA_CB_STAT_DRQ)) {
>        dprintf(6, "send_atapi_cmd : DRQ not set (status %02x)\n", status);
>        ret = -3;
>        goto fail;
>    }
>
> ret = ata_pio_transfer(op, 0, blocksize);
>    ret = ata_pio_transfer(op, 0, blocksize);
>
> }
>
> fail:
>  // Enable interrupts

[PATCH] ata: send TEST UNIT READY correctly

[Kevin O'Connor]

On Mon, Mar 19, 2012 at 11:41:09AM +0100, Paolo Bonzini wrote:
> The ATAPI driver does not need to support writes, but it does needs to
> avoid the PIO transfer and DRQ check when TEST UNIT READY is sent.
> Since TEST UNIT READY has no payload, checking for not busy is enough.
>
> This fixes a timeout when booting from CD/DVD, which [e-mail]
> reported to cause boot failures.

Thanks - I commited this change.

-Kevin