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

[Jens Axboe <qemu@kernel.dk>]

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