[Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Carlo Marcelo Arenas Belon]

The following patch implements fixes to the CD-ROM IDE/ATAPI emulation
since ide.c revision 1.66 and that prevents installation of OpenSolaris
guests because of timeouts like :

  WARNING: /pci@0,0/pci-ide@1,1/ide@1 (ata1);
          timeout: abort request, target=0 lun=0
  WARNING: /pci@0,0/pci-ide@1,1/ide@1 (ata1):
          timeout: abort device, target=0 lun=0
  WARNING: /pci@0,0/pci-ide@1,1/ide@1 (ata1):
          timeout: reset target, target=0 lun=0
  WARNING: /pci@0,0/pci-ide@1,1/ide@1 (ata1):
          timeout: reset bus, target=0 lun=0

It has been tested in Gentoo Linux 2007.0 amd64 (64bit) and x86 (32bit) hosts
using several different versions of Linux, FreeBSD, OpenBSD, NetBSD,
DragonFlyBSD, OpenSolaris and Windows 2000 guests

The patch implements changes to the following IDE commands as described by :

* change the model name (used by "INQUIRY" and "IDENTIFY DEVICE") to DVD
* recognize and honor "Allocation Length" parameter in "GET CONFIGURATION"
* only set "current profile" for the "GET CONFIGURATION" response if a profile
is current (CD or DVD loaded)
* calculate "data length" including all headers
* refactor code and add comments to help document references to all
implemented standards (ATAPI-4, SPC-3 and MMC-6)

Carlo
---
Index: hw/ide.c
===================================================================
RCS file: /sources/qemu/qemu/hw/ide.c,v
retrieving revision 1.79
diff -u -p -r1.79 ide.c
--- hw/ide.c	24 Dec 2007 14:33:24 -0000	1.79
+++ hw/ide.c	26 Dec 2007 07:11:01 -0000
@@ -1,5 +1,5 @@
 /*
- * QEMU IDE disk and CD-ROM Emulator
+ * QEMU IDE disk and CD/DVD-ROM Emulator
  *
  * Copyright (c) 2003 Fabrice Bellard
  * Copyright (c) 2006 Openedhand Ltd.
@@ -540,7 +540,7 @@ static void ide_atapi_identify(IDEState 
     put_le16(p + 21, 512); /* cache size in sectors */
     put_le16(p + 22, 4); /* ecc bytes */
     padstr((char *)(p + 23), QEMU_VERSION, 8); /* firmware version */
-    padstr((char *)(p + 27), "QEMU CD-ROM", 40); /* model */
+    padstr((char *)(p + 27), "QEMU DVD-ROM", 40); /* model */
     put_le16(p + 48, 1); /* dword I/O (XXX: should not be set on CDROM) */
 #ifdef USE_DMA_CDROM
     put_le16(p + 49, 1 << 9 | 1 << 8); /* DMA and LBA supported */
@@ -1634,12 +1634,13 @@ static void ide_atapi_cmd(IDEState *s)
         buf[6] = 0; /* reserved */
         buf[7] = 0; /* reserved */
         padstr8(buf + 8, 8, "QEMU");
-        padstr8(buf + 16, 16, "QEMU CD-ROM");
+        padstr8(buf + 16, 16, "QEMU DVD-ROM");
         padstr8(buf + 32, 4, QEMU_VERSION);
         ide_atapi_cmd_reply(s, 36, max_len);
         break;
     case GPCMD_GET_CONFIGURATION:
         {
+            uint32_t len;
             uint64_t total_sectors;
 
             /* only feature 0 is supported */
@@ -1648,17 +1649,27 @@ static void ide_atapi_cmd(IDEState *s)
                                     ASC_INV_FIELD_IN_CMD_PACKET);
                 break;
             }
-            memset(buf, 0, 32);
+            max_len = ube16_to_cpu(packet + 7);
             bdrv_get_geometry(s->bs, &total_sectors);
-            buf[3] = 16;
-            buf[7] = total_sectors <= 1433600 ? 0x08 : 0x10; /* current profile */
-            buf[10] = 0x10 | 0x1;
-            buf[11] = 0x08; /* size of profile list */
+            memset(buf, 0, 32);
+            if (total_sectors) {
+                if (total_sectors > 1433600) {
+                    buf[7] = 0x10; /* DVD-ROM */
+                } else {
+                    buf[7] = 0x08; /* CD-ROM */
+                }
+            } else {
+                buf[7] = 0x00; /* no current profile */
+            }
+            buf[10] = 0x02 | 0x01; /* persistent and current */
+            buf[11] = 0x08; /* size of profile list = 4 bytes per profile */
             buf[13] = 0x10; /* DVD-ROM profile */
-            buf[14] = buf[7] == 0x10; /* (in)active */
+            buf[14] = buf[13] == buf[7]; /* (in)active */
             buf[17] = 0x08; /* CD-ROM profile */
-            buf[18] = buf[7] == 0x08; /* (in)active */
-            ide_atapi_cmd_reply(s, 32, 32);
+            buf[18] = buf[17] == buf[7]; /* (in)active */
+            len = 8 + 4 + buf[11]; /* headers + size of profile list */
+            cpu_to_ube32(buf, len - 4); /* data length */
+            ide_atapi_cmd_reply(s, len, max_len);
             break;
         }
     default:

Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Carlo Marcelo Arenas Belon]

Can someone please comment on the mergability of this patch? or in what needs
to be done to it so that it can be committed?

The patch is still current and the bug it fixes would otherwise prevent
OpenSolaris guests to be installed in qemu.  the MMC-6 command it fixes (GET
CONFIGURATION) has been verified to behave as per the SPECs and compared to
behave just like real hardware does.

Carlo

On Wed, Dec 26, 2007 at 01:36:15AM -0600, Carlo Marcelo Arenas Belon wrote:
> The following patch implements fixes to the CD-ROM IDE/ATAPI emulation
> since ide.c revision 1.66 and that prevents installation of OpenSolaris
> guests because of timeouts like :
> 
>   WARNING: /pci@0,0/pci-ide@1,1/ide@1 (ata1);
>           timeout: abort request, target=0 lun=0
>   WARNING: /pci@0,0/pci-ide@1,1/ide@1 (ata1):
>           timeout: abort device, target=0 lun=0
>   WARNING: /pci@0,0/pci-ide@1,1/ide@1 (ata1):
>           timeout: reset target, target=0 lun=0
>   WARNING: /pci@0,0/pci-ide@1,1/ide@1 (ata1):
>           timeout: reset bus, target=0 lun=0
> 
> It has been tested in Gentoo Linux 2007.0 amd64 (64bit) and x86 (32bit) hosts
> using several different versions of Linux, FreeBSD, OpenBSD, NetBSD,
> DragonFlyBSD, OpenSolaris and Windows 2000 guests
> 
> The patch implements changes to the following IDE commands as described by :
> 
> * change the model name (used by "INQUIRY" and "IDENTIFY DEVICE") to DVD
> * recognize and honor "Allocation Length" parameter in "GET CONFIGURATION"
> * only set "current profile" for the "GET CONFIGURATION" response if a profile
> is current (CD or DVD loaded)
> * calculate "data length" including all headers
> * refactor code and add comments to help document references to all
> implemented standards (ATAPI-4, SPC-3 and MMC-6)
> 
> Carlo
> ---
> Index: hw/ide.c
> ===================================================================
> RCS file: /sources/qemu/qemu/hw/ide.c,v
> retrieving revision 1.79
> diff -u -p -r1.79 ide.c
> --- hw/ide.c	24 Dec 2007 14:33:24 -0000	1.79
> +++ hw/ide.c	26 Dec 2007 07:11:01 -0000
> @@ -1,5 +1,5 @@
>  /*
> - * QEMU IDE disk and CD-ROM Emulator
> + * QEMU IDE disk and CD/DVD-ROM Emulator
>   *
>   * Copyright (c) 2003 Fabrice Bellard
>   * Copyright (c) 2006 Openedhand Ltd.
> @@ -540,7 +540,7 @@ static void ide_atapi_identify(IDEState 
>      put_le16(p + 21, 512); /* cache size in sectors */
>      put_le16(p + 22, 4); /* ecc bytes */
>      padstr((char *)(p + 23), QEMU_VERSION, 8); /* firmware version */
> -    padstr((char *)(p + 27), "QEMU CD-ROM", 40); /* model */
> +    padstr((char *)(p + 27), "QEMU DVD-ROM", 40); /* model */
>      put_le16(p + 48, 1); /* dword I/O (XXX: should not be set on CDROM) */
>  #ifdef USE_DMA_CDROM
>      put_le16(p + 49, 1 << 9 | 1 << 8); /* DMA and LBA supported */
> @@ -1634,12 +1634,13 @@ static void ide_atapi_cmd(IDEState *s)
>          buf[6] = 0; /* reserved */
>          buf[7] = 0; /* reserved */
>          padstr8(buf + 8, 8, "QEMU");
> -        padstr8(buf + 16, 16, "QEMU CD-ROM");
> +        padstr8(buf + 16, 16, "QEMU DVD-ROM");
>          padstr8(buf + 32, 4, QEMU_VERSION);
>          ide_atapi_cmd_reply(s, 36, max_len);
>          break;
>      case GPCMD_GET_CONFIGURATION:
>          {
> +            uint32_t len;
>              uint64_t total_sectors;
>  
>              /* only feature 0 is supported */
> @@ -1648,17 +1649,27 @@ static void ide_atapi_cmd(IDEState *s)
>                                      ASC_INV_FIELD_IN_CMD_PACKET);
>                  break;
>              }
> -            memset(buf, 0, 32);
> +            max_len = ube16_to_cpu(packet + 7);
>              bdrv_get_geometry(s->bs, &total_sectors);
> -            buf[3] = 16;
> -            buf[7] = total_sectors <= 1433600 ? 0x08 : 0x10; /* current profile */
> -            buf[10] = 0x10 | 0x1;
> -            buf[11] = 0x08; /* size of profile list */
> +            memset(buf, 0, 32);
> +            if (total_sectors) {
> +                if (total_sectors > 1433600) {
> +                    buf[7] = 0x10; /* DVD-ROM */
> +                } else {
> +                    buf[7] = 0x08; /* CD-ROM */
> +                }
> +            } else {
> +                buf[7] = 0x00; /* no current profile */
> +            }
> +            buf[10] = 0x02 | 0x01; /* persistent and current */
> +            buf[11] = 0x08; /* size of profile list = 4 bytes per profile */
>              buf[13] = 0x10; /* DVD-ROM profile */
> -            buf[14] = buf[7] == 0x10; /* (in)active */
> +            buf[14] = buf[13] == buf[7]; /* (in)active */
>              buf[17] = 0x08; /* CD-ROM profile */
> -            buf[18] = buf[7] == 0x08; /* (in)active */
> -            ide_atapi_cmd_reply(s, 32, 32);
> +            buf[18] = buf[17] == buf[7]; /* (in)active */
> +            len = 8 + 4 + buf[11]; /* headers + size of profile list */
> +            cpu_to_ube32(buf, len - 4); /* data length */
> +            ide_atapi_cmd_reply(s, len, max_len);
>              break;
>          }
>      default:
> 
> 

Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Rob Landley]

On Friday 04 January 2008 04:02:07 Carlo Marcelo Arenas Belon wrote:
> Can someone please comment on the mergability of this patch? or in what
> needs to be done to it so that it can be committed?
>
> The patch is still current and the bug it fixes would otherwise prevent
> OpenSolaris guests to be installed in qemu.  the MMC-6 command it fixes
> (GET CONFIGURATION) has been verified to behave as per the SPECs and
> compared to behave just like real hardware does.
>
> Carlo

Well, I'm no expert but the patch is small enough that I can read it.

> >      padstr((char *)(p + 23), QEMU_VERSION, 8); /* firmware version */
> > -    padstr((char *)(p + 27), "QEMU CD-ROM", 40); /* model */
> > +    padstr((char *)(p + 27), "QEMU DVD-ROM", 40); /* model */
> >      put_le16(p + 48, 1); /* dword I/O (XXX: should not be set on CDROM)
> > */ #ifdef USE_DMA_CDROM
> >      put_le16(p + 49, 1 << 9 | 1 << 8); /* DMA and LBA supported */
> > @@ -1634,12 +1634,13 @@ static void ide_atapi_cmd(IDEState *s)
> >          buf[6] = 0; /* reserved */
> >          buf[7] = 0; /* reserved */
> >          padstr8(buf + 8, 8, "QEMU");
> > -        padstr8(buf + 16, 16, "QEMU CD-ROM");
> > +        padstr8(buf + 16, 16, "QEMU DVD-ROM");
> >          padstr8(buf + 32, 4, QEMU_VERSION);

I take it Solaris is actually checking these strings?  Or is this a cosmetic 
change?  (Not a _bad_ change, I'm just curious.)

> > @@ -1648,17 +1649,27 @@ static void ide_atapi_cmd(IDEState *s)
> >                                      ASC_INV_FIELD_IN_CMD_PACKET);
> >                  break;
> >              }
> > -            memset(buf, 0, 32);

This moved a few lines down, but that just seems to be churn.

> > +            max_len = ube16_to_cpu(packet + 7);

Why are you doing math on a value _before_ you adjust its endianness from a 
potentially foreign format into the CPU native one?  I take it that gives you 
a pointer from which a 16 byte value is fetched?

> >              bdrv_get_geometry(s->bs, &total_sectors);

Calculating stuff to use later rather than modifying buf[].  Ok.

> > -            buf[3] = 16;

The new code doesn't directly set buf[3] anymore, leaving it 0 from the 
memset.  I take it this is now handled by the cpu_to_ube32() targeting 4 
bytes of buf[] much later?

> > -            buf[7] = total_sectors <= 1433600 ? 0x08 : 0x10; /* current
> > profile */

This becomes 3-state now.  You added a state 0 when there's no cdrom in the 
drive.  The less intrusive change would be keeping the above line and adding 
a second line: "if (!total_sectors) buf[7] = 0;"  Not actually any larger, 
codewise, and a less intrusive change.

Where does the constant come from, anyway?

> > -            buf[10] = 0x10 | 0x1; 

This turns into 0x02 | 0x01 further down.  Why the change?

> > -            buf[11] = 0x08; /* size of profile list */

Set to the same value later, just a comment change.  Ok.

> > +            memset(buf, 0, 32);
> > +            if (total_sectors) {
> > +                if (total_sectors > 1433600) {
> > +                    buf[7] = 0x10; /* DVD-ROM */
> > +                } else {
> > +                    buf[7] = 0x08; /* CD-ROM */
> > +                }
> > +            } else {
> > +                buf[7] = 0x00; /* no current profile */
> > +            }
> > +            buf[10] = 0x02 | 0x01; /* persistent and current */
> > +            buf[11] = 0x08; /* size of profile list = 4 bytes per
> > profile */

Commented on already, above.

> > buf[13] = 0x10; /* DVD-ROM profile */ 

This is new.  This means "drive is DVD capable", not "DVD is in drive", 
correct?

> > -            buf[14] = buf[7] == 0x10; /* (in)active */
> > +            buf[14] = buf[13] == buf[7]; /* (in)active */

Um...  Ok?  This change is a NOP?  buf[13] is always 0x10 due to the previous 
line, so you're always comparing with 0x10...

The result seems to indicate "a dvd is in the drive", in a yes/no fashion 
rather than looking for 0x10 in position 7.  I'll assume the spec requires 
this?

> >              buf[17] = 0x08; /* CD-ROM profile */
> > -            buf[18] = buf[7] == 0x08; /* (in)active */
> > -            ide_atapi_cmd_reply(s, 32, 32);
> > +            buf[18] = buf[17] == buf[7]; /* (in)active */

Again, the added line is not actually a change.  What's the difference?

> > +            len = 8 + 4 + buf[11]; /* headers + size of profile list */

Once again, buf[11] is a constant (0x08) that you just set above.  So this is 
actually a constant value, but unless the constant propagation is good enough 
to track individual array members, you're forcing the machine to do 
unnecessary math.  Is there a reason for this?

> > +            cpu_to_ube32(buf, len - 4); /* data length */

Here's what replaced the assignment to buf[3] above.  This might be the meat 
of the patch?

> > +            ide_atapi_cmd_reply(s, len, max_len);

And this moved down from the call with (s, 32, 32) two hunks back.  You just 
calculated len (much unless I missed something must _always_ be 20), but 
max_len was calculated above as:

  max_len = ube16_to_cpu(packet + 7);

So max_len is adjusted for endianness, but len isn't?  Presumably because 
max_len came from the packet, but len is calculated?

> >              break;
> >          }
> >      default:

Well, I had several questions but it seems vaguely sane.  I assume you tested 
it and that solaris does indeed boot now.  I encountered the same hang trying 
out the first GNU/Solaris bootable CD ("indiana") wandered by on LWN.  I was 
curious about booting GNU/Solaris, since popularizing that name would 
probably annoy Richard Stallman.

I can test this patch and see if it boots my indiana ISO, assuming I can find 
said ISO...

Rob
-- 
"One of my most productive days was throwing away 1000 lines of code."
  - Ken Thompson.

Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Stuart Brady]

On Fri, Jan 04, 2008 at 06:25:25PM -0600, Rob Landley wrote:
> On Friday 04 January 2008 04:02:07 Carlo Marcelo Arenas Belon wrote:
> 
> > > -            buf[7] = total_sectors <= 1433600 ? 0x08 : 0x10; /* current
> > > profile */
> 
> Where does the constant come from, anyway?

1433600?  Seems it's the number of 512 KiB blocks in a 700 MiB CD image 
(700 * 1024 * 2).
-- 
Stuart Brady

Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Stuart Brady]

On Sat, Jan 05, 2008 at 01:02:30AM +0000, Stuart Brady wrote:
> 1433600?  Seems it's the number of 512 KiB blocks in a 700 MiB CD image 
> (700 * 1024 * 2).

Sorry, I mean 512 *byte* blocks.
-- 
Stuart Brady

Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Carlo Marcelo Arenas Belon]

On Sat, Jan 05, 2008 at 01:02:30AM +0000, Stuart Brady wrote:
> On Fri, Jan 04, 2008 at 06:25:25PM -0600, Rob Landley wrote:
> > On Friday 04 January 2008 04:02:07 Carlo Marcelo Arenas Belon wrote:
> > 
> > > > -            buf[7] = total_sectors <= 1433600 ? 0x08 : 0x10; /* current
> > > > profile */
> > 
> > Where does the constant come from, anyway?
> 
> 1433600?  Seems it's the number of 512 KiB blocks in a 700 MiB CD image 
> (700 * 1024 * 2).

yes, that magic constant corresponds to the maximum expected size for a CD-ROM
and is used to detect if the profile used should be for DVD-ROM or not.

It came from the original implementation patch for "GET CONFIGURATION" 
committed in revision 1.66 of ide.c by Filip Navarra as 
"Partial IDE DVD emulation"

http://cvs.savannah.nongnu.org/viewvc/qemu/hw/ide.c?root=qemu&r1=1.65&r2=1.66

Carlo

Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Carlo Marcelo Arenas Belon]

On Fri, Jan 04, 2008 at 06:25:25PM -0600, Rob Landley wrote:
> On Friday 04 January 2008 04:02:07 Carlo Marcelo Arenas Belon wrote:
> > Can someone please comment on the mergability of this patch? or in what
> > needs to be done to it so that it can be committed?
> >
> > The patch is still current and the bug it fixes would otherwise prevent
> > OpenSolaris guests to be installed in qemu.  the MMC-6 command it fixes
> > (GET CONFIGURATION) has been verified to behave as per the SPECs and
> > compared to behave just like real hardware does.
> >
> > Carlo
> 
> Well, I'm no expert but the patch is small enough that I can read it.

thanks

> > >      padstr((char *)(p + 23), QEMU_VERSION, 8); /* firmware version */
> > > -    padstr((char *)(p + 27), "QEMU CD-ROM", 40); /* model */
> > > +    padstr((char *)(p + 27), "QEMU DVD-ROM", 40); /* model */
> > >      put_le16(p + 48, 1); /* dword I/O (XXX: should not be set on CDROM)
> > > */ #ifdef USE_DMA_CDROM
> > >      put_le16(p + 49, 1 << 9 | 1 << 8); /* DMA and LBA supported */
> > > @@ -1634,12 +1634,13 @@ static void ide_atapi_cmd(IDEState *s)
> > >          buf[6] = 0; /* reserved */
> > >          buf[7] = 0; /* reserved */
> > >          padstr8(buf + 8, 8, "QEMU");
> > > -        padstr8(buf + 16, 16, "QEMU CD-ROM");
> > > +        padstr8(buf + 16, 16, "QEMU DVD-ROM");
> > >          padstr8(buf + 32, 4, QEMU_VERSION);
> 
> I take it Solaris is actually checking these strings?  Or is this a cosmetic 
> change?  (Not a _bad_ change, I'm just curious.)

this is just cosmetic.

in 2 of the RESENDs I did it was actually a second optional patch on a series
of 2.

this RESEND had it all together just because I though I had probably a better
chance of having it reviewed if it was 1 mail instead of 3 (including the
patch series description) and since I got mostly no feedback until now.

the real fix is on the "GET CONFIGURATION" implementation which is done below.

> > > @@ -1648,17 +1649,27 @@ static void ide_atapi_cmd(IDEState *s)
> > >                                      ASC_INV_FIELD_IN_CMD_PACKET);
> > >                  break;
> > >              }
> > > -            memset(buf, 0, 32);
> 
> This moved a few lines down, but that just seems to be churn.

it is structured in a way that could be later structured away into a function
when/if more profiles are added.

this way the first part of the code reads the input parameters and initialized
the buffer it will use later to generate a response in the logical order.

> > > +            max_len = ube16_to_cpu(packet + 7);
> 
> Why are you doing math on a value _before_ you adjust its endianness from a 
> potentially foreign format into the CPU native one?  I take it that gives you 
> a pointer from which a 16 byte value is fetched?

yes, this adds not to the value but the pointer where the packet is stored and
that contains on byte 7 (MSB) and 8 (LSB) the value for the Allocation Length
parameter as described in Table 275 of T10/1836-D Revision 1 (*)

> > >              bdrv_get_geometry(s->bs, &total_sectors);
> 
> Calculating stuff to use later rather than modifying buf[].  Ok.

I just kept this from the original patch, but it could be extracted instead
from s->nb_sectors as well, but though it would be probably easier if I just
kept the unrelated changes to a minimum.

> > > -            buf[3] = 16;
> 
> The new code doesn't directly set buf[3] anymore, leaving it 0 from the 
> memset.  I take it this is now handled by the cpu_to_ube32() targeting 4 
> bytes of buf[] much later?

yes, the response as described in Table 277 of T10/1836-D Revision 1 contains
a 4 byte "Data Length" field (LSB is byte 3) and I am calculating it at the
end instead of hardcoding it at the beginning, so that this code could adapt
if later extended to add more profiles (CD-RW, HD-DVD, ..).

> > > -            buf[7] = total_sectors <= 1433600 ? 0x08 : 0x10; /* current
> > > profile */
> 
> This becomes 3-state now.  You added a state 0 when there's no cdrom in the 
> drive.  The less intrusive change would be keeping the above line and adding 
> a second line: "if (!total_sectors) buf[7] = 0;"  Not actually any larger, 
> codewise, and a less intrusive change.

I refactored this code so that it is hopefully more clear in the intended
effect, which is to set the default profile to either of the available
profiles depending on the kind of media that was available, and as it is done
by real hardware.

Again, even if the refactoring was more of an intrusive change, it also allows
for more flexibility to expand this code to support more profiles in a cleaner
way.

> > > -            buf[10] = 0x10 | 0x1; 
> 
> This turns into 0x02 | 0x01 further down.  Why the change?

the original implementation got the bits to be enabled in the flags wrong,
0x10 is part of the Version Field and is meant to be 0 as detailed in table 87
of T10/1836-D Revision 1.

for the type of query issued the response flags returned were meant to be
persistent and current and that corresponds to the bit 1 and 0 of byte 2
respectively.

> > > -            buf[11] = 0x08; /* size of profile list */
> 
> Set to the same value later, just a comment change.  Ok.

yes this was part of the refactoring of this code to be clearer on the
intention and follow the documentation in an organized way.

> > > +            memset(buf, 0, 32);
> > > +            if (total_sectors) {
> > > +                if (total_sectors > 1433600) {
> > > +                    buf[7] = 0x10; /* DVD-ROM */
> > > +                } else {
> > > +                    buf[7] = 0x08; /* CD-ROM */
> > > +                }
> > > +            } else {
> > > +                buf[7] = 0x00; /* no current profile */
> > > +            }
> > > +            buf[10] = 0x02 | 0x01; /* persistent and current */
> > > +            buf[11] = 0x08; /* size of profile list = 4 bytes per
> > > profile */
> 
> Commented on already, above.

this header is described in Table 86 for R10/1836-D Revision 1

> > > buf[13] = 0x10; /* DVD-ROM profile */ 
> 
> This is new.  This means "drive is DVD capable", not "DVD is in drive", 
> correct?

correct, "DVD is in drive" is set by the next byte which checks if the profile
is active or not by looking at the "Current Profile" in buf[7] and matching it
with the profile identification in this byte.

> > > -            buf[14] = buf[7] == 0x10; /* (in)active */
> > > +            buf[14] = buf[13] == buf[7]; /* (in)active */
> 
> Um...  Ok?  This change is a NOP?  buf[13] is always 0x10 due to the previous 
> line, so you're always comparing with 0x10...

see below

> The result seems to indicate "a dvd is in the drive", in a yes/no fashion 
> rather than looking for 0x10 in position 7.  I'll assume the spec requires 
> this?

yes, the SPEC requires redundant information by having the "Current Profile"
indicate which kind of profile the device is, and also by indicating in the
list of profiles that the Profile actually in use has its "active" bit
enabled.

the "Profile Description" data is described in Table 90 of T10/1836-D Revision
1

> > >              buf[17] = 0x08; /* CD-ROM profile */
> > > -            buf[18] = buf[7] == 0x08; /* (in)active */
> > > -            ide_atapi_cmd_reply(s, 32, 32);
> > > +            buf[18] = buf[17] == buf[7]; /* (in)active */
> 
> Again, the added line is not actually a change.  What's the difference?

I think it is clearer this way, and matches better the wording of the
specification which says :

"The CurrentP bit when set to one, shall indicate that this profile is
currently active.  If no mediums is present, no profile should be active"

Of course as I said before, setting up profiles and marking which one is
active could be later be done through a macro or inline function instead just
by reproducing the logic above.

> > > +            len = 8 + 4 + buf[11]; /* headers + size of profile list */
> 
> Once again, buf[11] is a constant (0x08) that you just set above.  So this is 
> actually a constant value, but unless the constant propagation is good enough 
> to track individual array members, you're forcing the machine to do 
> unnecessary math.  Is there a reason for this?

It is clearer on why the size of the response includes the profile definitions
plus the 2 headers, remember that buf[11] is now a constant because we are
defining only 2 profiles by hand, but the aim was to clean the code and make
it extensible (and I hoped self explanatory) even if it makes the computer do
some math or is not as compact as the original one, after all this code
doesn't need to be optimized for speed and, well I trust compilers ;)

> > > +            cpu_to_ube32(buf, len - 4); /* data length */
> 
> Here's what replaced the assignment to buf[3] above.  This might be the meat 
> of the patch?

as you saw there were several changes that were required overall to the
previous implementation and that I described probably better in the original
post as can be seen in :

  http://lists.gnu.org/archive/html/qemu-devel/2007-11/msg00852.html

> 
> > > +            ide_atapi_cmd_reply(s, len, max_len);
> 
> And this moved down from the call with (s, 32, 32) two hunks back.  You just 
> calculated len (much unless I missed something must _always_ be 20), but 
> max_len was calculated above as:
> 
>   max_len = ube16_to_cpu(packet + 7);
> 
> So max_len is adjusted for endianness, but len isn't?  Presumably because 
> max_len came from the packet, but len is calculated?

yes

> Well, I had several questions but it seems vaguely sane.  I assume you tested 
> it and that solaris does indeed boot now.  I encountered the same hang trying 
> out the first GNU/Solaris bootable CD ("indiana") wandered by on LWN.  I was 
> curious about booting GNU/Solaris, since popularizing that name would 
> probably annoy Richard Stallman.

yes I'd tested and has been distributed with the Gentoo unofficial packages of
kvm I maintain for more than a month and committed into KVM for the last 2
releases.

> I can test this patch and see if it boots my indiana ISO, assuming I can find 
> said ISO...

good luck, and thanks

Carlo

(*) MMC6 DRAFT availeble at http://www.t10.org/ftp/t10/drafts/mmc6/mmc6r01.pdf

Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Rob Landley]

On Friday 04 January 2008 19:02:30 Stuart Brady wrote:
> On Fri, Jan 04, 2008 at 06:25:25PM -0600, Rob Landley wrote:
> > On Friday 04 January 2008 04:02:07 Carlo Marcelo Arenas Belon wrote:
> > > > -            buf[7] = total_sectors <= 1433600 ? 0x08 : 0x10; /*
> > > > current profile */
> >
> > Where does the constant come from, anyway?
>
> 1433600?  Seems it's the number of 512 KiB blocks in a 700 MiB CD image
> (700 * 1024 * 2).

Except that according to http://en.wikipedia.org/wiki/CD-ROM it's actually 703 
and 1/8 binary megabytes (360,000 sectors *2048 bytes), which would be 
1440000.

And we all know wikipedia's never wrong:
  http://www.theonion.com/content/node/50902 

Rob
-- 
"One of my most productive days was throwing away 1000 lines of code."
  - Ken Thompson.

Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Stuart Brady]

On Fri, Jan 04, 2008 at 09:53:09PM -0600, Rob Landley wrote:
> Except that according to http://en.wikipedia.org/wiki/CD-ROM it's actually 703 
> and 1/8 binary megabytes (360,000 sectors *2048 bytes), which would be 
> 1440000.

Apparently that value comes from 75 sectors per second * 80 minutes...
75*80*60 = 360000, and of course, 360000*2048/512 = 1440000, although
it actually seems that it should be one sector less than 80 minutes, 
which is 359999 2048-byte sectors or 1439996 512-byte chunks.

BTW, there are/were also 90 and 99 minute 'CD-Rs' -- Wikipedia's page on 
CD-Rs describes them, but they were never very popular, and a lot of 
drives can't read the discs.
-- 
Stuart Brady

Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Carlo Marcelo Arenas Belon]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

On Sat, Jan 05, 2008 at 10:28:34AM +0000, Stuart Brady wrote:
> On Fri, Jan 04, 2008 at 09:53:09PM -0600, Rob Landley wrote:
> > Except that according to http://en.wikipedia.org/wiki/CD-ROM it's actually 703 
> > and 1/8 binary megabytes (360,000 sectors *2048 bytes), which would be 
> > 1440000.
> 
> Apparently that value comes from 75 sectors per second * 80 minutes...
> 75*80*60 = 360000, and of course, 360000*2048/512 = 1440000, although
> it actually seems that it should be one sector less than 80 minutes, 
> which is 359999 2048-byte sectors or 1439996 512-byte chunks.
> 
> BTW, there are/were also 90 and 99 minute 'CD-Rs' -- Wikipedia's page on 
> CD-Rs describes them, but they were never very popular, and a lot of 
> drives can't read the discs.

the exact number of sectors is really not that relevant, as the whole point
here is to try to detect if it is a CD (700MB) or a DVD (4.7GB) and the logic
is just assuming that if it has more sectors than you should normally expect
in a CD, then it is a DVD.

attached the program I used in the guests (only works on Linux) to poke the 
emulated drive (or a physical drive if you feel like) and compare the responses
(you will need to take a look at the SPEC tables to interpret the data though)

for my own tests (using a linux guest with -cdrom /dev/cdrom in my linux
host that has a DVD-+RW drive) :

   700MB CD-R = 1374880 (with FreeSBIE 2.0.1)
  4.7GB DVD-R = 6939520 (with SXDE 9/07)

feel free to report back with the value to use then if you happen to have a CD
that is completely full but I had already enough problems trying to get this
merged without trying to change the code that much to try to guess a better
magic number than the one was originally used (I like 1440000 though)

Carlo

[-- Attachment #2: ide-atapi.c --]
[-- Type: text/plain, Size: 2012 bytes --]

/*
    ide-atapi

    Copyright (c) 2007 Carlo Marcelo Arenas Belon

    ide-atapi is free software: you can redistribute it and/or modify
    it under the terms of the GNU General Public License version 2
    as published by the Free Software Foundation.

    This program is distributed in the hope that it will be useful,
    but WITHOUT ANY WARRANTY; without even the implied warranty of
    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
    GNU General Public License for more details.

    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.
*/

#include <sys/ioctl.h>
#include <linux/cdrom.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <string.h>
#include <stdio.h>
#include <unistd.h>

int main (int argc, char *argv[])
{
   struct cdrom_generic_command cgc;
   struct request_sense sense;
   unsigned char buf[250];
   int i;

   if (argc < 2) {
      printf("Usage: %s <device>\n", argv[0]);
      printf("\n");
      printf("  device: where the commands are send\n");
      printf("\n");
      return 1;
   }

   memset (&cgc, 0, sizeof(struct cdrom_generic_command));
   memset (&sense, 0, sizeof(struct request_sense));
   memset (&buf, 0, sizeof(buf));

   int fd = open (argv[1], O_RDONLY | O_NONBLOCK);
   if (fd < 0) {
      printf("couldn't open device %s\n", argv[1]);
      return 1;
   }
   cgc.cmd[0] = GPCMD_GET_CONFIGURATION;
   cgc.cmd[1] = 0x00;
   cgc.cmd[8] = sizeof(buf);
   cgc.timeout = 100;
   cgc.buffer = buf;
   cgc.buflen = sizeof(buf);
   cgc.data_direction = CGC_DATA_READ;
   cgc.sense = &sense;
   cgc.quiet = 0;
   i = ioctl (fd, CDROM_SEND_PACKET, &cgc);
   if (i < 0) {
      printf("command failed\n");
      close (fd);
      return 1;
   }
   printf("Response raw dump:\n");
   for (i = 0; i<sizeof(buf); i++) {
      if (i % 16 == 0) printf("\n");
      printf("%02x ", buf[i]);
   }
   printf("\n");
   close (fd);
   return 0;
}

Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Rob Landley]

I re-downloaded the GNU/Solaris preview CD, linked from here:
  http://lwn.net/Articles/256737/

And fired it up:
  qemu -cdrom solaris-preview.iso -boot d -m 256

Note that it won't boot with the default 128 megs, because Solaris is a pig.

Without the patch at the start of this thread, the GNU/Solaris boot hangs.  
With this patch, it boots just fine.

On Friday 04 January 2008 21:36:41 Carlo Marcelo Arenas Belon wrote:
> On Fri, Jan 04, 2008 at 06:25:25PM -0600, Rob Landley wrote:
> > On Friday 04 January 2008 04:02:07 Carlo Marcelo Arenas Belon wrote:
> > > >          padstr8(buf + 8, 8, "QEMU");
> > > > -        padstr8(buf + 16, 16, "QEMU CD-ROM");
> > > > +        padstr8(buf + 16, 16, "QEMU DVD-ROM");
> > > >          padstr8(buf + 32, 4, QEMU_VERSION);
> >
> > I take it Solaris is actually checking these strings?  Or is this a
> > cosmetic change?  (Not a _bad_ change, I'm just curious.)
>
> this is just cosmetic.

Ok.

> > > > @@ -1648,17 +1649,27 @@ static void ide_atapi_cmd(IDEState *s)
> > > >                                      ASC_INV_FIELD_IN_CMD_PACKET);
> > > >                  break;
> > > >              }
> > > > -            memset(buf, 0, 32);
> >
> > This moved a few lines down, but that just seems to be churn.
>
> it is structured in a way that could be later structured away into a
> function when/if more profiles are added.

Makes sense.

> > > > +            max_len = ube16_to_cpu(packet + 7);
> >
> > Why are you doing math on a value _before_ you adjust its endianness from
> > a potentially foreign format into the CPU native one?  I take it that
> > gives you a pointer from which a 16 byte value is fetched?
>
> yes, this adds not to the value but the pointer where the packet is stored
> and that contains on byte 7 (MSB) and 8 (LSB) the value for the Allocation
> Length parameter as described in Table 275 of T10/1836-D Revision 1 (*)

Is dereferencing a word value at an odd address alignment safe on all the 
potential host platforms?  (I dunno if qemu runs on an arm host, nor if the 
ube16_to_cpu value is already dealing with this.  Just curious.)

> > > > -            buf[3] = 16;
> >
> > The new code doesn't directly set buf[3] anymore, leaving it 0 from the
> > memset.  I take it this is now handled by the cpu_to_ube32() targeting 4
> > bytes of buf[] much later?
>
> yes, the response as described in Table 277 of T10/1836-D Revision 1
> contains a 4 byte "Data Length" field (LSB is byte 3) and I am calculating
> it at the end instead of hardcoding it at the beginning, so that this code
> could adapt if later extended to add more profiles (CD-RW, HD-DVD, ..).

Ok.

> > > > -            buf[7] = total_sectors <= 1433600 ? 0x08 : 0x10; /*
> > > > current profile */
> >
> > This becomes 3-state now.  You added a state 0 when there's no cdrom in
> > the drive.  The less intrusive change would be keeping the above line and
> > adding a second line: "if (!total_sectors) buf[7] = 0;"  Not actually any
> > larger, codewise, and a less intrusive change.
>
> I refactored this code so that it is hopefully more clear in the intended
> effect, which is to set the default profile to either of the available
> profiles depending on the kind of media that was available, and as it is
> done by real hardware.
>
> Again, even if the refactoring was more of an intrusive change, it also
> allows for more flexibility to expand this code to support more profiles in
> a cleaner way.

I take it buf[7]=0 is what real hardware returns when there's no disk in the 
drive?

> > > > -            buf[10] = 0x10 | 0x1;
> >
> > This turns into 0x02 | 0x01 further down.  Why the change?
>
> the original implementation got the bits to be enabled in the flags wrong,
> 0x10 is part of the Version Field and is meant to be 0 as detailed in table
> 87 of T10/1836-D Revision 1.

Explicit bug fix.  Check.

> > > >              buf[17] = 0x08; /* CD-ROM profile */
> > > > -            buf[18] = buf[7] == 0x08; /* (in)active */
> > > > -            ide_atapi_cmd_reply(s, 32, 32);
> > > > +            buf[18] = buf[17] == buf[7]; /* (in)active */
> >
> > Again, the added line is not actually a change.  What's the difference?
>
> I think it is clearer this way, and matches better the wording of the
> specification which says :
>
> "The CurrentP bit when set to one, shall indicate that this profile is
> currently active.  If no mediums is present, no profile should be active"

Not sure how that part addresses the question, but you address it in the next 
hunk.

> > > > +            len = 8 + 4 + buf[11]; /* headers + size of profile list
> > > > */
> >
> > Once again, buf[11] is a constant (0x08) that you just set above.  So
> > this is actually a constant value, but unless the constant propagation is
> > good enough to track individual array members, you're forcing the machine
> > to do unnecessary math.  Is there a reason for this?
>
> It is clearer on why the size of the response includes the profile
> definitions plus the 2 headers, remember that buf[11] is now a constant
> because we are defining only 2 profiles by hand, but the aim was to clean
> the code and make it extensible (and I hoped self explanatory) even if it
> makes the computer do some math or is not as compact as the original one,
> after all this code doesn't need to be optimized for speed and, well I
> trust compilers ;)

I.E. if the value gets set in a more complicated way, it propagates naturally 
to all the places it needs to go.

*shrug*  I suppose I can see trying to have fewer instances of each magic 
constant...

> > > > +            cpu_to_ube32(buf, len - 4); /* data length */
> >
> > Here's what replaced the assignment to buf[3] above.  This might be the
> > meat of the patch?
>
> as you saw there were several changes that were required overall to the
> previous implementation and that I described probably better in the
> original post as can be seen in :
>
>   http://lists.gnu.org/archive/html/qemu-devel/2007-11/msg00852.html

I hadn't read the original patch, but after testing the GNU/Solaris preview CD 
I'm fairly happy with the patch.

> > Well, I had several questions but it seems vaguely sane.  I assume you
> > tested it and that solaris does indeed boot now.  I encountered the same
> > hang trying out the first GNU/Solaris bootable CD ("indiana") wandered by
> > on LWN.  I was curious about booting GNU/Solaris, since popularizing that
> > name would probably annoy Richard Stallman.
>
> yes I'd tested and has been distributed with the Gentoo unofficial packages
> of kvm I maintain for more than a month and committed into KVM for the last
> 2 releases.

The patch works for me.

> > I can test this patch and see if it boots my indiana ISO, assuming I can
> > find said ISO...
>
> good luck, and thanks

I did, and GNU/Solaris successfully booted to a login prompt with your patch.  
(It didn't give me the promised Gnome desktop but stayed in text mode, and 
when I guessed "root" it wanted a password that the documentation doesn't 
mention, but that's Sun's fault, not qemu's.)

> Carlo
>
> (*) MMC6 DRAFT availeble at
> http://www.t10.org/ftp/t10/drafts/mmc6/mmc6r01.pdf

Rob
-- 
"One of my most productive days was throwing away 1000 lines of code."
  - Ken Thompson.

Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Stuart Brady]

On Sat, Jan 05, 2008 at 08:22:33PM -0600, Carlo Marcelo Arenas Belon wrote:

> the exact number of sectors is really not that relevant, as the whole point
> here is to try to detect if it is a CD (700MB) or a DVD (4.7GB) and the logic
> is just assuming that if it has more sectors than you should normally expect
> in a CD, then it is a DVD.

My answer was quite relevant to Rob's question, which was "Where does 
the constant come from, anyway?"

As for the code, there's a choice between using an incorrect value, and 
correctly detecting for the vast majority of cases, and using the 
correct value and correctly detecting for 100% of cases.  Perhaps "only 
marginally broken" is "good enough", seeing as nobody's complained.

> but I had already enough problems trying to get this
> merged without trying to change the code that much to try to guess a better
> magic number than the one was originally used (I like 1440000 though)

Sorry, but did anyone complain?

No.
-- 
Stuart Brady

Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Andreas Färber]

Am 06.01.2008 um 14:57 schrieb Stuart Brady:

> On Sat, Jan 05, 2008 at 08:22:33PM -0600, Carlo Marcelo Arenas Belon  
> wrote:
>
>> the exact number of sectors is really not that relevant, as the  
>> whole point
>> here is to try to detect if it is a CD (700MB) or a DVD (4.7GB) and  
>> the logic
>> is just assuming that if it has more sectors than you should  
>> normally expect
>> in a CD, then it is a DVD.
>
> My answer was quite relevant to Rob's question, which was "Where does
> the constant come from, anyway?"
>
> As for the code, there's a choice between using an incorrect value,  
> and
> correctly detecting for the vast majority of cases, and using the
> correct value and correctly detecting for 100% of cases.  Perhaps  
> "only
> marginally broken" is "good enough", seeing as nobody's complained.

Either way, shouldn't it be a preprocessor define rather than a magic  
number, maybe something like MAX_SECTORS_CD? Then it can more easily  
be found and changed, where necessary.

Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Carlo Marcelo Arenas Belon]

On Sun, Jan 06, 2008 at 01:57:38PM +0000, Stuart Brady wrote:
> On Sat, Jan 05, 2008 at 08:22:33PM -0600, Carlo Marcelo Arenas Belon wrote:
> 
> > the exact number of sectors is really not that relevant, as the whole point
> > here is to try to detect if it is a CD (700MB) or a DVD (4.7GB) and the logic
> > is just assuming that if it has more sectors than you should normally expect
> > in a CD, then it is a DVD.
> 
> My answer was quite relevant to Rob's question, which was "Where does 
> the constant come from, anyway?"

Yes, and my comment didn't meant it wasn't relevant, but that the exact value
isn't as important as finding the upper possible limit that can be used to
assume that anything bigger than that has to be a DVD instead.

when I looked at the patch originally, tried to find something in the
specifications that could be used to define a "standard" CD size but couldn't
find anything, because as other people pointed out, there isn't a "standard"
CD size even if most of the CDs out there are 80min large.

> As for the code, there's a choice between using an incorrect value, and 
> correctly detecting for the vast majority of cases, and using the 
> correct value and correctly detecting for 100% of cases.  Perhaps "only 
> marginally broken" is "good enough", seeing as nobody's complained.

Agree, and that is why I said using 1440000 will be probably better and
provided a tool that can be used to generate this call in the guests (only for
Linux though), so that the maximum value could be found empirically.

Since this is meant to work for ISO images in a file as well as devices with
physical CDs on them, I suspect (and remember the original code which included
this magic value wasn't mine) that the number of sectors might be the only
reliable indication of media size, but will look at it again  and see if there
is any other metadata available in all cases which could be used instead.

> > but I had already enough problems trying to get this
> > merged without trying to change the code that much to try to guess a better
> > magic number than the one was originally used (I like 1440000 though)
> 
> Sorry, but did anyone complain?
> 
> No.

Not sure what you mean by this, but having a patch resent several times with
no comments is IMHO more problematic that complains.

Carlo

Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Carlo Marcelo Arenas Belon]

On Sun, Jan 06, 2008 at 03:32:27PM +0100, Andreas Färber wrote:
> Either way, shouldn't it be a preprocessor define rather than a magic  
> number, maybe something like MAX_SECTORS_CD? Then it can more easily  
> be found and changed, where necessary.

Point taken, will fix that if we still have to rely on the number of sectors
to detect the media type.

Carlo

Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Carlo Marcelo Arenas Belon]

On Sun, Jan 06, 2008 at 03:22:15AM -0600, Rob Landley wrote:
> > > > > @@ -1648,17 +1649,27 @@ static void ide_atapi_cmd(IDEState *s)
> > > > >                                      ASC_INV_FIELD_IN_CMD_PACKET);
> > > > >                  break;
> > > > >              }
> > > > > -            memset(buf, 0, 32);
> > >
> > > This moved a few lines down, but that just seems to be churn.
> >
> > it is structured in a way that could be later structured away into a
> > function when/if more profiles are added.
> 
> Makes sense.

I should also said that unless done after as shown by the patch, reading the
parameter will be imposible, because the buffer used for the IDE commands is
reused for input/output as shown by lines 1295 to 1300 at the beginning of
ide_atapi_cmd :

    const uint8_t *packet;
    uint8_t *buf;
    int max_len;

    packet = s->io_buffer;
    buf = s->io_buffer;

> > > > > +            max_len = ube16_to_cpu(packet + 7);
> > >
> > > Why are you doing math on a value _before_ you adjust its endianness from
> > > a potentially foreign format into the CPU native one?  I take it that
> > > gives you a pointer from which a 16 byte value is fetched?
> >
> > yes, this adds not to the value but the pointer where the packet is stored
> > and that contains on byte 7 (MSB) and 8 (LSB) the value for the Allocation
> > Length parameter as described in Table 275 of T10/1836-D Revision 1 (*)
> 
> Is dereferencing a word value at an odd address alignment safe on all the 
> potential host platforms?  (I dunno if qemu runs on an arm host, nor if the 
> ube16_to_cpu value is already dealing with this.  Just curious.)

good point, and I am affraid that it might be unsafe in those architectures
with strict alignment requirements, but sadly there is no mechanism available
(at least for the ide emulation) to emulate the hardware buffers in a safe
way.

could someone with most experience on this hint into the right direction to
approach this problem and that affects several other emulated commandas as
well?

> > > > > -            buf[7] = total_sectors <= 1433600 ? 0x08 : 0x10; /*
> > > > > current profile */
> > >
> > > This becomes 3-state now.  You added a state 0 when there's no cdrom in
> > > the drive.  The less intrusive change would be keeping the above line and
> > > adding a second line: "if (!total_sectors) buf[7] = 0;"  Not actually any
> > > larger, codewise, and a less intrusive change.
> >
> > I refactored this code so that it is hopefully more clear in the intended
> > effect, which is to set the default profile to either of the available
> > profiles depending on the kind of media that was available, and as it is
> > done by real hardware.
> >
> > Again, even if the refactoring was more of an intrusive change, it also
> > allows for more flexibility to expand this code to support more profiles in
> > a cleaner way.
> 
> I take it buf[7]=0 is what real hardware returns when there's no disk in the 
> drive?

yes

> > It is clearer on why the size of the response includes the profile
> > definitions plus the 2 headers, remember that buf[11] is now a constant
> > because we are defining only 2 profiles by hand, but the aim was to clean
> > the code and make it extensible (and I hoped self explanatory) even if it
> > makes the computer do some math or is not as compact as the original one,
> > after all this code doesn't need to be optimized for speed and, well I
> > trust compilers ;)
> 
> I.E. if the value gets set in a more complicated way, it propagates naturally 
> to all the places it needs to go.
> 
> *shrug*  I suppose I can see trying to have fewer instances of each magic 
> constant...

will try to make a simpler version then that could be used at least as an
intermediate step and that is less intrusive than this one, while avoiding so
may magic values.

Carlo