[Qemu-devel] cdrom disc type - is this patch correct? (unbreaks recent FreeBSD guest's -cdrom access)

[Qemu-devel] cdrom disc type - is this patch correct? (unbreaks recent FreeBSD guest's -cdrom access)

[Juergen Lock]

Hi!

 Yesterday I learned that FreeBSD 7.0-BETA2 guests will no longer
read from the emulated cd drive, apparently because of this commit:
	http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/dev/ata/atapi-cd.c.diff?r1=1.193;r2=1.193.2.1
The following patch file added to the qemu-devel port fixes the issue
for me, is it also correct?   (making the guest see a dvd in the drive
when it is inserted, previously it saw the drive as empty.)

 The second hunk is already in qemu cvs so remove it if you want to
test on that.  ISO used for testing:
	ftp://ftp.freebsd.org:/pub/FreeBSD/ISO-IMAGES-i386/7.0/7.0-BETA2-i386-disc1.iso
(test by either selecting fixit->cdrom or by trying to install, just
booting it will always work because that goes thru the bios.)

Index: qemu/hw/ide.c
@@ -1339,6 +1341,8 @@
                 case 0x2a:
                     cpu_to_ube16(&buf[0], 28 + 6);
                     buf[2] = 0x70;
+                    if (bdrv_is_inserted(s->bs))
+                        buf[2] = 0x40;
                     buf[3] = 0;
                     buf[4] = 0;
                     buf[5] = 0;
@@ -1347,7 +1351,7 @@
 
                     buf[8] = 0x2a;
                     buf[9] = 0x12;
-                    buf[10] = 0x00;
+                    buf[10] = 0x08;
                     buf[11] = 0x00;
                     
                     buf[12] = 0x70;

 Thanx,
	Juergen

Re: [Qemu-devel] cdrom disc type - is this patch correct? (unbreaks recent FreeBSD guest's -cdrom access)

[Jens Axboe]

On Tue, Nov 13 2007, Juergen Lock wrote:
> Hi!
> 
>  Yesterday I learned that FreeBSD 7.0-BETA2 guests will no longer
> read from the emulated cd drive, apparently because of this commit:
> 	http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/dev/ata/atapi-cd.c.diff?r1=1.193;r2=1.193.2.1
> The following patch file added to the qemu-devel port fixes the issue
> for me, is it also correct?   (making the guest see a dvd in the drive
> when it is inserted, previously it saw the drive as empty.)
> 
>  The second hunk is already in qemu cvs so remove it if you want to
> test on that.  ISO used for testing:
> 	ftp://ftp.freebsd.org:/pub/FreeBSD/ISO-IMAGES-i386/7.0/7.0-BETA2-i386-disc1.iso
> (test by either selecting fixit->cdrom or by trying to install, just
> booting it will always work because that goes thru the bios.)
> 
> Index: qemu/hw/ide.c
> @@ -1339,6 +1341,8 @@
>                  case 0x2a:
>                      cpu_to_ube16(&buf[0], 28 + 6);
>                      buf[2] = 0x70;
> +                    if (bdrv_is_inserted(s->bs))
> +                        buf[2] = 0x40;

medium type code has been obsoleted since at least 1999. Looking back at
even older docs, 0x70 is 'door closed, no disc present'. 0x40 is a
reserved value though, I would not suggest using that. Given that
freebsd breaks, my suggest change would be the below - keep the 0x70 for
when no disc is really inserted, but don't set anything if there is.

diff --git a/hw/ide.c b/hw/ide.c
index 5f76c27..52d4c78 100644
--- a/hw/ide.c
+++ b/hw/ide.c
@@ -1344,7 +1344,10 @@ static void ide_atapi_cmd(IDEState *s)
                     break;
                 case 0x2a:
                     cpu_to_ube16(&buf[0], 28 + 6);
-                    buf[2] = 0x70;
+		    if (!bdrv_is_inserted(s->bs))
+			buf[2] = 0x70;
+		    else
+			buf[2] = 0;
                     buf[3] = 0;
                     buf[4] = 0;
                     buf[5] = 0;

-- 
Jens Axboe

Re: [Qemu-devel] cdrom disc type - is this patch correct? (unbreaks recent FreeBSD guest's -cdrom access)

[Juergen Lock]

Oops, I seem to have missed this post, sorry for not answering earlier...

In article <20071114120203.GD5064@kernel.dk> you write:
>On Tue, Nov 13 2007, Juergen Lock wrote:
>> Hi!
>> 
>>  Yesterday I learned that FreeBSD 7.0-BETA2 guests will no longer
>> read from the emulated cd drive, apparently because of this commit:
>> 
>http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/dev/ata/atapi-cd.c.diff?r1=1.193;r2=1.193.2.1
>> The following patch file added to the qemu-devel port fixes the issue
>> for me, is it also correct?   (making the guest see a dvd in the drive
>> when it is inserted, previously it saw the drive as empty.)
>> 
>>  The second hunk is already in qemu cvs so remove it if you want to
>> test on that.  ISO used for testing:
>> 
>ftp://ftp.freebsd.org:/pub/FreeBSD/ISO-IMAGES-i386/7.0/7.0-BETA2-i386-disc1.iso
>> (test by either selecting fixit->cdrom or by trying to install, just
>> booting it will always work because that goes thru the bios.)
>> 
>> Index: qemu/hw/ide.c
>> @@ -1339,6 +1341,8 @@
>>                  case 0x2a:
>>                      cpu_to_ube16(&buf[0], 28 + 6);
>>                      buf[2] = 0x70;
>> +                    if (bdrv_is_inserted(s->bs))
>> +                        buf[2] = 0x40;
>
>medium type code has been obsoleted since at least 1999. Looking back at
>even older docs, 0x70 is 'door closed, no disc present'. 0x40 is a
>reserved value though, I would not suggest using that. Given that
>freebsd breaks, my suggest change would be the below - keep the 0x70 for
>when no disc is really inserted, but don't set anything if there is.
>
>diff --git a/hw/ide.c b/hw/ide.c
>index 5f76c27..52d4c78 100644
>--- a/hw/ide.c
>+++ b/hw/ide.c
>@@ -1344,7 +1344,10 @@ static void ide_atapi_cmd(IDEState *s)
>                     break;
>                 case 0x2a:
>                     cpu_to_ube16(&buf[0], 28 + 6);
>-                    buf[2] = 0x70;
>+		    if (!bdrv_is_inserted(s->bs))
>+			buf[2] = 0x70;
>+		    else
>+			buf[2] = 0;
>                     buf[3] = 0;
>                     buf[4] = 0;
>                     buf[5] = 0;
>

 Well that (0) doesn't work.  The relevant FreeBSD header,
	http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/dev/ata/atapi-cd.h?rev=1.46.2.1;content-type=text%2Fx-cvsweb-markup
, defines the following:

[...]
/* CDROM Capabilities and Mechanical Status Page */
struct cappage {
    /* mode page data header */
    u_int16_t   data_length;
    u_int8_t    medium_type;
#define MST_TYPE_MASK_LOW       0x0f
#define MST_FMT_NONE            0x00
#define MST_DATA_120            0x01
#define MST_AUDIO_120           0x02
#define MST_COMB_120            0x03
#define MST_PHOTO_120           0x04
#define MST_DATA_80             0x05
#define MST_AUDIO_80            0x06
#define MST_COMB_80             0x07
#define MST_PHOTO_80            0x08

#define MST_TYPE_MASK_HIGH      0x70
#define MST_CDROM               0x00
#define MST_CDR                 0x10
#define MST_CDRW                0x20
#define MST_DVD                 0x40

#define MST_NO_DISC             0x70
#define MST_DOOR_OPEN           0x71
#define MST_FMT_ERROR           0x72

    u_int8_t    dev_spec;
    u_int16_t   unused;
    u_int16_t   blk_desc_len;

    /* capabilities page */
    u_int8_t    page_code;
#define ATAPI_CDROM_CAP_PAGE    0x2a

    u_int8_t    param_len;

    u_int16_t   media;
#define MST_READ_CDR            0x0001
#define MST_READ_CDRW           0x0002
#define MST_READ_PACKET         0x0004
#define MST_READ_DVDROM         0x0008
#define MST_READ_DVDR           0x0010
#define MST_READ_DVDRAM         0x0020
#define MST_WRITE_CDR           0x0100
#define MST_WRITE_CDRW          0x0200
#define MST_WRITE_TEST          0x0400
#define MST_WRITE_DVDR          0x1000
#define MST_WRITE_DVDRAM        0x2000

    u_int16_t   capabilities;
#define MST_AUDIO_PLAY          0x0001
#define MST_COMPOSITE           0x0002
#define MST_AUDIO_P1            0x0004
#define MST_AUDIO_P2            0x0008
#define MST_MODE2_f1            0x0010
#define MST_MODE2_f2            0x0020
#define MST_MULTISESSION        0x0040
#define MST_BURNPROOF           0x0080
#define MST_READ_CDDA           0x0100
#define MST_CDDA_STREAM         0x0200
#define MST_COMBINED_RW         0x0400
#define MST_CORRECTED_RW        0x0800
#define MST_SUPPORT_C2          0x1000
#define MST_ISRC                0x2000
#define MST_UPC                 0x4000

    u_int8_t    mechanism;
#define MST_LOCKABLE            0x01
#define MST_LOCKED              0x02
#define MST_PREVENT             0x04
#define MST_EJECT               0x08
#define MST_MECH_MASK           0xe0
#define MST_MECH_CADDY          0x00
#define MST_MECH_TRAY           0x20
#define MST_MECH_POPUP          0x40
#define MST_MECH_CHANGER        0x80
#define MST_MECH_CARTRIDGE      0xa0

    uint8_t     audio;
#define MST_SEP_VOL             0x01
#define MST_SEP_MUTE            0x02

    u_int16_t   max_read_speed;         /* max raw data rate in bytes/1000 */
    u_int16_t   max_vol_levels;         /* number of discrete volume levels */
    u_int16_t   buf_size;               /* internal buffer size in bytes/1024 */
    u_int16_t   cur_read_speed;         /* current data rate in bytes/1000  */

    u_int8_t    reserved3;
    u_int8_t    misc;

    u_int16_t   max_write_speed;        /* max raw data rate in bytes/1000 */
    u_int16_t   cur_write_speed;        /* current data rate in bytes/1000  */
    u_int16_t   copy_protect_rev;
    u_int16_t   reserved4;
};

[...]

 and in
	http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/dev/ata/atapi-cd.c?rev=1.193.2.1;content-type=text%2Fx-cvsweb-markup
a check is done like this:

[...]
static int
acd_geom_access(struct g_provider *pp, int dr, int dw, int de)
{
    device_t dev = pp->geom->softc;
    struct acd_softc *cdp = device_get_ivars(dev);
    int timeout = 60, track;

    /* check for media present, waiting for loading medium just in case */
    while (timeout--) {
	if (!acd_mode_sense(dev, ATAPI_CDROM_CAP_PAGE,
			    (caddr_t)&cdp->cap, sizeof(cdp->cap)) &&
			    cdp->cap.page_code == ATAPI_CDROM_CAP_PAGE) {
	    if ((cdp->cap.medium_type == MST_FMT_NONE) ||
		(cdp->cap.medium_type == MST_NO_DISC) ||
		(cdp->cap.medium_type == MST_DOOR_OPEN) ||
		(cdp->cap.medium_type == MST_FMT_ERROR))
		return EIO;
	    else
		break;
	}
	pause("acdld", hz / 2);
    }
[...]

 There have been reports of this also being broken on real hw tho, like,
	http://lists.freebsd.org/pipermail/freebsd-current/2007-November/079760.html
so I'm not sure what to make of this...

 Thanx,
	Juergen

Re: [Qemu-devel] cdrom disc type - is this patch correct? (unbreaks recent FreeBSD guest's -cdrom access)

[Jens Axboe]

On Mon, Nov 19 2007, Juergen Lock wrote:
> Oops, I seem to have missed this post, sorry for not answering earlier...
> 
> In article <20071114120203.GD5064@kernel.dk> you write:
> >On Tue, Nov 13 2007, Juergen Lock wrote:
> >> Hi!
> >> 
> >>  Yesterday I learned that FreeBSD 7.0-BETA2 guests will no longer
> >> read from the emulated cd drive, apparently because of this commit:
> >> 
> >http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/dev/ata/atapi-cd.c.diff?r1=1.193;r2=1.193.2.1
> >> The following patch file added to the qemu-devel port fixes the issue
> >> for me, is it also correct?   (making the guest see a dvd in the drive
> >> when it is inserted, previously it saw the drive as empty.)
> >> 
> >>  The second hunk is already in qemu cvs so remove it if you want to
> >> test on that.  ISO used for testing:
> >> 
> >ftp://ftp.freebsd.org:/pub/FreeBSD/ISO-IMAGES-i386/7.0/7.0-BETA2-i386-disc1.iso
> >> (test by either selecting fixit->cdrom or by trying to install, just
> >> booting it will always work because that goes thru the bios.)
> >> 
> >> Index: qemu/hw/ide.c
> >> @@ -1339,6 +1341,8 @@
> >>                  case 0x2a:
> >>                      cpu_to_ube16(&buf[0], 28 + 6);
> >>                      buf[2] = 0x70;
> >> +                    if (bdrv_is_inserted(s->bs))
> >> +                        buf[2] = 0x40;
> >
> >medium type code has been obsoleted since at least 1999. Looking back at
> >even older docs, 0x70 is 'door closed, no disc present'. 0x40 is a
> >reserved value though, I would not suggest using that. Given that
> >freebsd breaks, my suggest change would be the below - keep the 0x70 for
> >when no disc is really inserted, but don't set anything if there is.
> >
> >diff --git a/hw/ide.c b/hw/ide.c
> >index 5f76c27..52d4c78 100644
> >--- a/hw/ide.c
> >+++ b/hw/ide.c
> >@@ -1344,7 +1344,10 @@ static void ide_atapi_cmd(IDEState *s)
> >                     break;
> >                 case 0x2a:
> >                     cpu_to_ube16(&buf[0], 28 + 6);
> >-                    buf[2] = 0x70;
> >+		    if (!bdrv_is_inserted(s->bs))
> >+			buf[2] = 0x70;
> >+		    else
> >+			buf[2] = 0;
> >                     buf[3] = 0;
> >                     buf[4] = 0;
> >                     buf[5] = 0;
> >
> 
>  Well that (0) doesn't work.  The relevant FreeBSD header,
> 	http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/dev/ata/atapi-cd.h?rev=1.46.2.1;content-type=text%2Fx-cvsweb-markup
> , defines the following:
> 
> [...]
> /* CDROM Capabilities and Mechanical Status Page */
> struct cappage {
>     /* mode page data header */
>     u_int16_t   data_length;
>     u_int8_t    medium_type;
> #define MST_TYPE_MASK_LOW       0x0f
> #define MST_FMT_NONE            0x00
> #define MST_DATA_120            0x01
> #define MST_AUDIO_120           0x02
> #define MST_COMB_120            0x03
> #define MST_PHOTO_120           0x04
> #define MST_DATA_80             0x05
> #define MST_AUDIO_80            0x06
> #define MST_COMB_80             0x07
> #define MST_PHOTO_80            0x08
> 
> #define MST_TYPE_MASK_HIGH      0x70
> #define MST_CDROM               0x00
> #define MST_CDR                 0x10
> #define MST_CDRW                0x20
> #define MST_DVD                 0x40
> 
> #define MST_NO_DISC             0x70
> #define MST_DOOR_OPEN           0x71
> #define MST_FMT_ERROR           0x72
> 
>     u_int8_t    dev_spec;
>     u_int16_t   unused;
>     u_int16_t   blk_desc_len;
> 
>     /* capabilities page */
>     u_int8_t    page_code;
> #define ATAPI_CDROM_CAP_PAGE    0x2a
> 
>     u_int8_t    param_len;
> 
>     u_int16_t   media;
> #define MST_READ_CDR            0x0001
> #define MST_READ_CDRW           0x0002
> #define MST_READ_PACKET         0x0004
> #define MST_READ_DVDROM         0x0008
> #define MST_READ_DVDR           0x0010
> #define MST_READ_DVDRAM         0x0020
> #define MST_WRITE_CDR           0x0100
> #define MST_WRITE_CDRW          0x0200
> #define MST_WRITE_TEST          0x0400
> #define MST_WRITE_DVDR          0x1000
> #define MST_WRITE_DVDRAM        0x2000
> 
>     u_int16_t   capabilities;
> #define MST_AUDIO_PLAY          0x0001
> #define MST_COMPOSITE           0x0002
> #define MST_AUDIO_P1            0x0004
> #define MST_AUDIO_P2            0x0008
> #define MST_MODE2_f1            0x0010
> #define MST_MODE2_f2            0x0020
> #define MST_MULTISESSION        0x0040
> #define MST_BURNPROOF           0x0080
> #define MST_READ_CDDA           0x0100
> #define MST_CDDA_STREAM         0x0200
> #define MST_COMBINED_RW         0x0400
> #define MST_CORRECTED_RW        0x0800
> #define MST_SUPPORT_C2          0x1000
> #define MST_ISRC                0x2000
> #define MST_UPC                 0x4000
> 
>     u_int8_t    mechanism;
> #define MST_LOCKABLE            0x01
> #define MST_LOCKED              0x02
> #define MST_PREVENT             0x04
> #define MST_EJECT               0x08
> #define MST_MECH_MASK           0xe0
> #define MST_MECH_CADDY          0x00
> #define MST_MECH_TRAY           0x20
> #define MST_MECH_POPUP          0x40
> #define MST_MECH_CHANGER        0x80
> #define MST_MECH_CARTRIDGE      0xa0
> 
>     uint8_t     audio;
> #define MST_SEP_VOL             0x01
> #define MST_SEP_MUTE            0x02
> 
>     u_int16_t   max_read_speed;         /* max raw data rate in bytes/1000 */
>     u_int16_t   max_vol_levels;         /* number of discrete volume levels */
>     u_int16_t   buf_size;               /* internal buffer size in bytes/1024 */
>     u_int16_t   cur_read_speed;         /* current data rate in bytes/1000  */
> 
>     u_int8_t    reserved3;
>     u_int8_t    misc;
> 
>     u_int16_t   max_write_speed;        /* max raw data rate in bytes/1000 */
>     u_int16_t   cur_write_speed;        /* current data rate in bytes/1000  */
>     u_int16_t   copy_protect_rev;
>     u_int16_t   reserved4;
> };
> 
> [...]
> 
>  and in
> 	http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/dev/ata/atapi-cd.c?rev=1.193.2.1;content-type=text%2Fx-cvsweb-markup
> a check is done like this:
> 
> [...]
> static int
> acd_geom_access(struct g_provider *pp, int dr, int dw, int de)
> {
>     device_t dev = pp->geom->softc;
>     struct acd_softc *cdp = device_get_ivars(dev);
>     int timeout = 60, track;
> 
>     /* check for media present, waiting for loading medium just in case */
>     while (timeout--) {
> 	if (!acd_mode_sense(dev, ATAPI_CDROM_CAP_PAGE,
> 			    (caddr_t)&cdp->cap, sizeof(cdp->cap)) &&
> 			    cdp->cap.page_code == ATAPI_CDROM_CAP_PAGE) {
> 	    if ((cdp->cap.medium_type == MST_FMT_NONE) ||
> 		(cdp->cap.medium_type == MST_NO_DISC) ||
> 		(cdp->cap.medium_type == MST_DOOR_OPEN) ||
> 		(cdp->cap.medium_type == MST_FMT_ERROR))
> 		return EIO;
> 	    else
> 		break;
> 	}
> 	pause("acdld", hz / 2);
>     }
> [...]
> 
>  There have been reports of this also being broken on real hw tho,
>  like,
>  http://lists.freebsd.org/pipermail/freebsd-current/2007-November/079760.html
>  so I'm not sure what to make of this...

Well if you ask me (I used to maintain the linux atapi driver), the
freebsd driver suffers from a classic case of 'but the specs says so!'
syndrome. In this case it's even ancient documentation. Drivers should
never try to be 100% spec oriented, they also need a bit of real life
sensibility. The code you quote right above this text is clearly too
anal.

-- 
Jens Axboe