On Mon, Nov 19 2007, Juergen Lock wrote: > Oops, I seem to have missed this post, sorry for not answering earlier... > > In article <[EMAIL PROTECTED]> 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
