* [Qemu-devel] cdrom disc type - is this patch correct? (unbreaks recent FreeBSD guest's -cdrom access)
@ 2007-11-13 21:22 Juergen Lock
2007-11-14 12:02 ` Jens Axboe
0 siblings, 1 reply; 4+ messages in thread
From: Juergen Lock @ 2007-11-13 21:22 UTC (permalink / raw)
To: qemu-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] cdrom disc type - is this patch correct? (unbreaks recent FreeBSD guest's -cdrom access)
2007-11-13 21:22 [Qemu-devel] cdrom disc type - is this patch correct? (unbreaks recent FreeBSD guest's -cdrom access) Juergen Lock
@ 2007-11-14 12:02 ` Jens Axboe
2007-11-18 23:37 ` Juergen Lock
0 siblings, 1 reply; 4+ messages in thread
From: Jens Axboe @ 2007-11-14 12:02 UTC (permalink / raw)
To: qemu-devel
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] cdrom disc type - is this patch correct? (unbreaks recent FreeBSD guest's -cdrom access)
2007-11-14 12:02 ` Jens Axboe
@ 2007-11-18 23:37 ` Juergen Lock
2007-11-19 9:21 ` Jens Axboe
0 siblings, 1 reply; 4+ messages in thread
From: Juergen Lock @ 2007-11-18 23:37 UTC (permalink / raw)
To: qemu; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] cdrom disc type - is this patch correct? (unbreaks recent FreeBSD guest's -cdrom access)
2007-11-18 23:37 ` Juergen Lock
@ 2007-11-19 9:21 ` Jens Axboe
0 siblings, 0 replies; 4+ messages in thread
From: Jens Axboe @ 2007-11-19 9:21 UTC (permalink / raw)
To: Juergen Lock; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-11-19 9:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-11-13 21:22 [Qemu-devel] cdrom disc type - is this patch correct? (unbreaks recent FreeBSD guest's -cdrom access) Juergen Lock
2007-11-14 12:02 ` Jens Axboe
2007-11-18 23:37 ` Juergen Lock
2007-11-19 9:21 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).