Skip to contentSkip to conversation

ATAPI pass through v3

PATCH New Bique Alexandre 3 participants 14 messages Aug 6, 2009 – Aug 7, 2009 2 messages not in the archive

Subsystems (2)
Block layer Kbuild
lore
Share & tools
lore

Bique Alexandre

author
#0 lore raw

PATCH 1/1 ATAPI pass through v3

Hi,

Here is the updated ATAPI pass through patches.

What's new ?
- no more switch -cdrom-pt, we rely on bdrv_is_sg() to know if we use the
pass through code or not.
- opening the device with O_NONBLOCK, so it doesn't fail when there is no
media in the drive.
- added a command line switch (-cdrom-allow-fw-upgrade) to enable cdrom
firmware upgrade (see WRITE_BUFFER command). By default the firmware upgrade is disabled.
- fixed REPORT_KEY command.
- fixed READ_12 command.

Thanks.

Signature · 1 line
Alexandre Bique
author
#1 lore raw

PATCH 1/4 ATAPI pass through v3

Better TAG rule

Signature · 1 line
Alexandre Bique
Makefile +2 -1
index 2a4b3f3..df5b5c4 100644
@@ -300,8 +300,9 @@ endif
 test speed: all
 	$(MAKE) -C tests $@
 
+.PHONY: TAGS
 TAGS:
-	etags *.[ch] tests/*.[ch] block/*.[ch] hw/*.[ch]
+	find "$(SRC_PATH)" -name '*.[hc]' -print0 | xargs -0 etags
 
 cscope:
 	rm -f ./cscope.*
#2 lore raw
Quoted 1 line · Bique Alexandre

Better TAG rule

This should be the subject of the patch, and the body should include a short description and a Signed-off-by tag.

Given that it's not directly related to ATAPI support you might want to send it out seperately.

author
#3 lore raw

PATCH 2/4 ATAPI pass through v3

In reply to Bique Alexandre

Few comments

Signature · 1 line
Alexandre Bique
hw/ide.c +6 -6
index 1e56786..16ca80c 100644
@@ -1664,9 +1664,9 @@ static void ide_atapi_cmd(IDEState *s)
             switch(action) {
             case 0: /* current values */
                 switch(code) {
-                case 0x01: /* error recovery */
+                case 0x01: /* read write error recovery parameters */
                     cpu_to_ube16(&buf[0], 16 + 6);
-                    buf[2] = 0x70;
+                    buf[2] = 0x70; /* Obsolete: medium type code */
                     buf[3] = 0;
                     buf[4] = 0;
                     buf[5] = 0;

@@ -1683,17 +1683,17 @@ static void ide_atapi_cmd(IDEState *s)
                     buf[15] = 0x00;
                     ide_atapi_cmd_reply(s, 16, max_len);
                     break;
-                case 0x2a:
+                case 0x2a: /* CD/DVD capabilities & mechanical status */
                     cpu_to_ube16(&buf[0], 28 + 6);
-                    buf[2] = 0x70;
+                    buf[2] = 0x70; /* Obsolete: medium type code */
                     buf[3] = 0;
                     buf[4] = 0;
                     buf[5] = 0;
                     buf[6] = 0;
                     buf[7] = 0;
 
-                    buf[8] = 0x2a;
-                    buf[9] = 0x12;
+                    buf[8] = 0x2a; /* page code */
+                    buf[9] = 0x12; /* page length */
                     buf[10] = 0x00;
                     buf[11] = 0x00;
#4 lore raw
Quoted 1 line · Bique Alexandre

Few comments

Again not a very useful format for the patch.

I'd say something like

Subject: [PATCH n/m] atapi: fix up a few comments

author
#5 lore raw

PATCH 3/4 ATAPI pass through v3

In reply to Bique Alexandre

Updates ATAPI defines.

Signature · 1 line
Alexandre Bique
hw/ide.c +60 -17
index 063f08d..34cdd93 100644
@@ -216,13 +216,17 @@
 
 #define ATAPI_PACKET_SIZE 12
 
-/* The generic packet command opcodes for CD/DVD Logical Units,
- * From Table 57 of the SFF8090 Ver. 3 (Mt. Fuji) draft standard. */
+/*********************************************************************
+ * Generic Packet commands, MMC commands, and such
+ *********************************************************************/
+
+ /* The generic packet command opcodes for CD/DVD Logical Units,
+  * From Table 57 of the SFF8090 Ver. 3 (Mt. Fuji) draft standard. */
 #define GPCMD_BLANK			    0xa1
 #define GPCMD_CLOSE_TRACK		    0x5b
 #define GPCMD_FLUSH_CACHE		    0x35
 #define GPCMD_FORMAT_UNIT		    0x04
-#define GPCMD_GET_CONFIGURATION		    0x46
+#define GPCMD_GET_CONFIGURATION             0x46
 #define GPCMD_GET_EVENT_STATUS_NOTIFICATION 0x4a
 #define GPCMD_GET_PERFORMANCE		    0xac
 #define GPCMD_INQUIRY			    0x12

@@ -238,6 +242,8 @@
 #define GPCMD_PREVENT_ALLOW_MEDIUM_REMOVAL  0x1e
 #define GPCMD_READ_10			    0x28
 #define GPCMD_READ_12			    0xa8
+#define GPCMD_READ_BUFFER		    0x3c
+#define GPCMD_READ_BUFFER_CAPACITY	    0x5c
 #define GPCMD_READ_CDVD_CAPACITY	    0x25
 #define GPCMD_READ_CD			    0xbe
 #define GPCMD_READ_CD_MSF		    0xb9

@@ -246,15 +252,16 @@
 #define GPCMD_READ_FORMAT_CAPACITIES	    0x23
 #define GPCMD_READ_HEADER		    0x44
 #define GPCMD_READ_TRACK_RZONE_INFO	    0x52
-#define GPCMD_READ_SUBCHANNEL		    0x42
-#define GPCMD_READ_TOC_PMA_ATIP		    0x43
+#define GPCMD_READ_SUBCHANNEL	            0x42
+#define GPCMD_READ_TOC_PMA_ATIP             0x43
 #define GPCMD_REPAIR_RZONE_TRACK	    0x58
 #define GPCMD_REPORT_KEY		    0xa4
 #define GPCMD_REQUEST_SENSE		    0x03
 #define GPCMD_RESERVE_RZONE_TRACK	    0x53
+#define GPCMD_SEND_CUE_SHEET		    0x5d
 #define GPCMD_SCAN			    0xba
 #define GPCMD_SEEK			    0x2b
-#define GPCMD_SEND_DVD_STRUCTURE	    0xad
+#define GPCMD_SEND_DVD_STRUCTURE	    0xbf
 #define GPCMD_SEND_EVENT		    0xa2
 #define GPCMD_SEND_KEY			    0xa3
 #define GPCMD_SEND_OPC			    0x54

@@ -263,9 +270,11 @@
 #define GPCMD_START_STOP_UNIT		    0x1b
 #define GPCMD_STOP_PLAY_SCAN		    0x4e
 #define GPCMD_TEST_UNIT_READY		    0x00
-#define GPCMD_VERIFY_10			    0x2f
+#define GPCMD_VERIFY_10                     0x2f
 #define GPCMD_WRITE_10			    0x2a
+#define GPCMD_WRITE_12			    0xaa
 #define GPCMD_WRITE_AND_VERIFY_10	    0x2e
+#define GPCMD_WRITE_BUFFER		    0x3b
 /* This is listed as optional in ATAPI 2.6, but is (curiously)
  * missing from Mt. Fuji, Table 57.  It _is_ mentioned in Mt. Fuji
  * Table 377 as an MMC command for SCSi devices though...  Most ATAPI

@@ -279,18 +288,20 @@
  * older drives only.
  */
 #define GPCMD_GET_MEDIA_STATUS		    0xda
-#define GPCMD_MODE_SENSE_6		    0x1a
+#define GPCMD_MODE_SENSE_6                  0x1a
 
 /* Mode page codes for mode sense/set */
+#define GPMODE_VENDOR_PAGE		0x00
 #define GPMODE_R_W_ERROR_PAGE		0x01
 #define GPMODE_WRITE_PARMS_PAGE		0x05
+#define GPMODE_WCACHING_PAGE		0x08
 #define GPMODE_AUDIO_CTL_PAGE		0x0e
 #define GPMODE_POWER_PAGE		0x1a
 #define GPMODE_FAULT_FAIL_PAGE		0x1c
 #define GPMODE_TO_PROTECT_PAGE		0x1d
 #define GPMODE_CAPABILITIES_PAGE	0x2a
 #define GPMODE_ALL_PAGES		0x3f
-/* Not in Mt. Fuji, but in ATAPI 2.6 -- depricated now in favor
+/* Not in Mt. Fuji, but in ATAPI 2.6 -- deprecated now in favor
  * of MODE_SENSE_POWER_PAGE */
 #define GPMODE_CDROM_PAGE		0x0d
 

@@ -304,10 +315,26 @@
  */
 
 /* Some generally useful CD-ROM information */
-#define CD_MINS                       80 /* max. minutes per CD */
-#define CD_SECS                       60 /* seconds per minute */
-#define CD_FRAMES                     75 /* frames per second */
-#define CD_FRAMESIZE                2048 /* bytes per frame, "cooked" mode */
+#define CD_MINS              80 /* max. minutes per CD */
+#define CD_SECS              60 /* seconds per minute */
+#define CD_FRAMES            75 /* frames per second */
+#define CD_SYNC_SIZE         12 /* 12 sync bytes per raw data frame */
+#define CD_MSF_OFFSET       150 /* MSF numbering offset of first frame */
+#define CD_CHUNK_SIZE        24 /* lowest-level "data bytes piece" */
+#define CD_NUM_OF_CHUNKS     98 /* chunks per frame */
+#define CD_FRAMESIZE_SUB     96 /* subchannel data "frame" size */
+#define CD_HEAD_SIZE          4 /* header (address) bytes per raw data frame */
+#define CD_SUBHEAD_SIZE       8 /* subheader bytes per raw XA data frame */
+#define CD_EDC_SIZE           4 /* bytes EDC per most raw data frame types */
+#define CD_ZERO_SIZE          8 /* bytes zero per yellow book mode 1 frame */
+#define CD_ECC_SIZE         276 /* bytes ECC per most raw data frame types */
+#define CD_FRAMESIZE       2048 /* bytes per frame, "cooked" mode */
+#define CD_FRAMESIZE_RAW   2352 /* bytes per frame, "raw" mode */
+#define CD_FRAMESIZE_RAWER 2646 /* The maximum possible returned bytes */
+/* most drives don't deliver everything: */
+#define CD_FRAMESIZE_RAW1 (CD_FRAMESIZE_RAW-CD_SYNC_SIZE) /*2340*/
+#define CD_FRAMESIZE_RAW0 (CD_FRAMESIZE_RAW-CD_SYNC_SIZE-CD_HEAD_SIZE) /*2336*/
+
 #define CD_MAX_BYTES       (CD_MINS * CD_SECS * CD_FRAMES * CD_FRAMESIZE)
 #define CD_MAX_SECTORS     (CD_MAX_BYTES / 512)
 

@@ -346,12 +373,15 @@
 #define MMC_PROFILE_HDDVD_RW_DL         0x005A
 #define MMC_PROFILE_INVALID             0xFFFF
 
+
 #define ATAPI_INT_REASON_CD             0x01 /* 0 = data transfer */
 #define ATAPI_INT_REASON_IO             0x02 /* 1 = transfer to the host */
 #define ATAPI_INT_REASON_REL            0x04
 #define ATAPI_INT_REASON_TAG            0xf8
 
 /* same constants as bochs */
+#define ASC_NONE                             0x00
+#define ASC_READ_ERROR                       0x11
 #define ASC_ILLEGAL_OPCODE                   0x20
 #define ASC_LOGICAL_BLOCK_OOR                0x21
 #define ASC_INV_FIELD_IN_CMD_PACKET          0x24

@@ -367,10 +397,23 @@
 #define CFA_INVALID_ADDRESS     0x21
 #define CFA_ADDRESS_OVERFLOW    0x2f
 
-#define SENSE_NONE            0
-#define SENSE_NOT_READY       2
-#define SENSE_ILLEGAL_REQUEST 5
-#define SENSE_UNIT_ATTENTION  6
+/* Sense keys */
+#define SENSE_NONE             0
+#define SENSE_RECOVERED_ERROR  1
+#define SENSE_NOT_READY        2
+#define SENSE_MEDIUM_ERROR     3
+#define SENSE_HARDWARE_ERROR   4
+#define SENSE_ILLEGAL_REQUEST  5
+#define SENSE_UNIT_ATTENTION   6
+#define SENSE_DATA_PROTECT     7
+#define SENSE_BLANK_CHECK      8
+#define SENSE_VENDOR_SPECIFIC  9
+#define SENSE_COPY_ABORTED     10
+#define SENSE_ABORTED_COMMAND  11
+#define SENSE_VOLUME_OVERFLOW  13
+#define SENSE_MISCOMPARE       14
+
+#define IO_BUFFER_MAX_SIZE      (IDE_DMA_BUF_SECTORS * 512 + 4)
 
 struct IDEState;
author
#7 lore raw
Quoted 9 lines · On Thursday 06 August 2009 17:12:04 Christoph Hellwig wrote:

Should be

Subject: [PATCH n/m] atapi: protocol define updates

Add some missing ATAPI protocol defintions, and fix up some whitespaces and comments on the way.

Signed-off-by Bique Alexandre <[email protected]>

Sorry about that.

If you want I can re-send the patches.

Signature · 1 line
Alexandre Bique
#8 lore raw
In reply to Bique Alexandre
Quoted 1 line · Bique Alexandre

Updates ATAPI defines.

Appart from Christoph comments, once there, please align correctly the ones that you are moving, please.

Later, Juan.

author
#9 lore raw

PATCH 4/4 ATAPI pass through v3

In reply to Bique Alexandre

The ATAPI pass through feature.

Signature · 1 line
Alexandre Bique
block/raw-posix.c +10 -1
index bdee07f..0510379 100644
@@ -154,6 +154,12 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
     else if (!(bdrv_flags & BDRV_O_CACHE_WB))
         s->open_flags |= O_DSYNC;
 
+    /* If we open a cdrom device, and there is no media inside, we
+     * have to add O_NONBLOCK to open else it will fail */
+    if (bdrv_get_type_hint(bs) == BDRV_TYPE_CDROM &&
+        bdrv_is_sg(bs))
+        s->open_flags |= O_NONBLOCK;
+
     s->fd = -1;
     fd = open(filename, s->open_flags, 0644);
     if (fd < 0) {

@@ -1027,7 +1033,10 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
 
     s->type = FTYPE_FILE;
 #if defined(__linux__) && defined(CONFIG_AIO)
-    if (strstart(filename, "/dev/sg", NULL)) {
+    if (strstart(filename, "/dev/sg", NULL) ||
+        strstart(filename, "/dev/cd", NULL) ||
+        strstart(filename, "/dev/dvd", NULL) ||
+        strstart(filename, "/dev/sr", NULL)) {
         bs->sg = 1;
     }
 #endif
hw/atapi-pt.c new file +1002 -0

1,002 lines not on this page · raw message

hw/atapi-pt.h new file +6 -0
new file mode 100644
index 0000000..61dc443
@@ -0,0 +1,6 @@
+#ifndef ATAPI_PT_H
+# define ATAPI_PT_H
+
+extern int atapi_pt_allow_fw_upgrade;
+
+#endif /* !ATAPI_PT_H */
hw/ide.c +86 -2

136 lines not on this page · raw message

qemu-options.hx +7 -0
index 1b420a3..2761223 100644
@@ -91,6 +91,13 @@ Use @var{file} as CD-ROM image (you cannot use @option{-hdc} and
 using @file{/dev/cdrom} as filename (@pxref{host_drives}).
 ETEXI
 
+DEF("cdrom-allow-fw-upgrade", 0, QEMU_OPTION_cdrom_allow_fw_upgrade,
+    "-cdrom-allow-fw-upgrade     allow the guest to process cdrom firmware upgrade.\n")
+STEXI
+@item -cdrom-allow-fw-upgrade
+Allow Qemu to pass through ATAPI firmware upgrade command.
+ETEXI
+
 DEF("drive", HAS_ARG, QEMU_OPTION_drive,
     "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
     "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
vl.c +5 -0
index fdd4f03..e29d13c 100644
@@ -142,6 +142,7 @@ int main(int argc, char **argv)
 #include "hw/smbios.h"
 #include "hw/xen.h"
 #include "hw/qdev.h"
+#include "hw/atapi-pt.h"
 #include "bt-host.h"
 #include "net.h"
 #include "monitor.h"

@@ -1792,6 +1793,7 @@ static int bt_parse(const char *opt)
 
 #define HD_ALIAS "index=%d,media=disk"
 #define CDROM_ALIAS "index=2,media=cdrom"
+#define CDROM_PT_ALIAS "index=2,media=cdrompt"
 #define FD_ALIAS "index=%d,if=floppy"
 #define PFLASH_ALIAS "if=pflash"
 #define MTD_ALIAS "if=mtd"

@@ -5119,6 +5121,9 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_cdrom:
                 drive_add(optarg, CDROM_ALIAS);
                 break;
+            case QEMU_OPTION_cdrom_allow_fw_upgrade:
+                atapi_pt_allow_fw_upgrade = 1;
+                break;
             case QEMU_OPTION_boot:
                 {
                     static const char * const params[] = {
#10 lore raw
Quoted 1 line · Bique Alexandre

The ATAPI pass through feature.

Again, this should be the subject, and the body should have a short description on how it's done and why it's useful.

Is the firmware upgrade command really that special that we need to filter it and not other harmfull commands? That really needs explanation in the patch description and/or a comment.

Some comments on the patch:

block/raw-posix.c +5 -0
index bdee07f..0510379 100644
@@ -154,6 +154,12 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
     else if (!(bdrv_flags & BDRV_O_CACHE_WB))
         s->open_flags |= O_DSYNC;
 
+    /* If we open a cdrom device, and there is no media inside, we
+     * have to add O_NONBLOCK to open else it will fail */
+    if (bdrv_get_type_hint(bs) == BDRV_TYPE_CDROM &&
+        bdrv_is_sg(bs))
+        s->open_flags |= O_NONBLOCK;

this should be in cdrom_open, that way you won't need the bdrv_get_type_hint check either.

hw/atapi-pt.c new file +2 -0
new file mode 100644
index 0000000..85f6b6a
@@ -0,0 +1,1002 @@
+int atapi_pt_allow_fw_upgrade = 0;
+

This seems to be missing any sort of author/copyright line.

+#define MSF_TO_FRAMES(M, S, F) (((M) * CD_SECS + (S)) * CD_FRAMES + (F))

Would be much nicer as a small (inline) function.

+/* The generic packet command opcodes for CD/DVD Logical Units,
+ * From Table 57 of the SFF8090 Ver. 3 (Mt. Fuji) draft standard. */ +static const struct {
+ unsigned short packet_command;
+ const char * const text;
+} packet_command_texts[] = {

Maybe move all these tables into a separate file?

@@ -1288,6 +1350,14 @@ static inline int ube16_to_cpu(const uint8_t *buf)

     return (buf[0] << 8) | buf[1];

}

+#if CONFIG_ATAPI_PT /* only atapi-pt uses it so let's avoid unused
+ * warning */
+static inline int ube24_to_cpu(const uint8_t *buf)
+{
+ return (buf[0] << 16) | (buf[1] << 8) | buf[2];
+}
+#endif /* CONFIG_ATAPI_PT */

When did gcc start issuing unused warnings for inlines?

@@ -1675,6 +1745,10 @@ static int ide_dvd_read_structure(IDEState *s, int format,

     }

}

+#if CONFIG_ATAPI_PT
+#include "atapi-pt.c"
+#endif

Including .c files is areally horrible style, just make the function you also need from atapi-pt.c non-static.

@@ -2872,6 +2946,16 @@ static void ide_init2(IDEState *ide_state,

             if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
                 s->is_cdrom = 1;

+ if (bdrv_is_sg(s->bs)) {

This check looks reversed to me.

author
#11 lore raw
Quoted 28 lines · On Thursday 06 August 2009 17:23:56 Christoph Hellwig wrote:
Quoted 1 line · Bique Alexandre

The ATAPI pass through feature.

Again, this should be the subject, and the body should have a short description on how it's done and why it's useful.

Is the firmware upgrade command really that special that we need to filter it and not other harmfull commands? That really needs explanation in the patch description and/or a comment.

Some comments on the patch:

block/raw-posix.c +5 -0
index bdee07f..0510379 100644
@@ -154,6 +154,12 @@ static int raw_open_common(BlockDriverState *bs, const
 char *filename, else if (!(bdrv_flags & BDRV_O_CACHE_WB))
         s->open_flags |= O_DSYNC;
 
+    /* If we open a cdrom device, and there is no media inside, we
+     * have to add O_NONBLOCK to open else it will fail */
+    if (bdrv_get_type_hint(bs) == BDRV_TYPE_CDROM &&
+        bdrv_is_sg(bs))
+        s->open_flags |= O_NONBLOCK;
Quoted 2 lines

this should be in cdrom_open, that way you won't need the bdrv_get_type_hint check either.

Thank you for this one :-)

Quoted 10 lines
hw/atapi-pt.c new file +2 -0
new file mode 100644
index 0000000..85f6b6a
@@ -0,0 +1,1002 @@
+int atapi_pt_allow_fw_upgrade = 0;
+

This seems to be missing any sort of author/copyright line.

Done.

Quoted 3 lines

+#define MSF_TO_FRAMES(M, S, F) (((M) * CD_SECS + (S)) * CD_FRAMES + (F))

Would be much nicer as a small (inline) function.

Done.

Quoted 8 lines
hw/atapi-pt.c new file +6 -0
+/* The generic packet command opcodes for CD/DVD Logical Units,
+ * From Table 57 of the SFF8090 Ver. 3 (Mt. Fuji) draft standard. */
+static const struct {
+    unsigned short packet_command;
+    const char * const text;
+} packet_command_texts[] = {

Maybe move all these tables into a separate file?

Alright, moving this in hw/atapi-data.{h,c}

Quoted 12 lines
hw/atapi-pt.c new file +7 -0
@@ -1288,6 +1350,14 @@ static inline int ube16_to_cpu(const uint8_t *buf)
     return (buf[0] << 8) | buf[1];
 }
 
+#if CONFIG_ATAPI_PT /* only atapi-pt uses it so let's avoid unused
+                            * warning */
+static inline int ube24_to_cpu(const uint8_t *buf)
+{
+    return (buf[0] << 16) | (buf[1] << 8) | buf[2];
+}
+#endif /* CONFIG_ATAPI_PT */
Quoted 1 line

When did gcc start issuing unused warnings for inlines?

Because there is the static keyword.

Quoted 10 lines
hw/atapi-pt.c new file +3 -0
@@ -1675,6 +1745,10 @@ static int ide_dvd_read_structure(IDEState *s, int
 format, }
 }
 
+#if CONFIG_ATAPI_PT
+#include "atapi-pt.c"
+#endif

Including .c files is areally horrible style, just make the function you also need from atapi-pt.c non-static.

Alright. Doing.

Quoted 7 lines
hw/atapi-pt.c new file +1 -0
@@ -2872,6 +2946,16 @@ static void ide_init2(IDEState *ide_state,
 
             if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
                 s->is_cdrom = 1;
+                if (bdrv_is_sg(s->bs)) {

This check looks reversed to me.

Oh my god, you're right. Fixed!

Thanks for reviewing, I'll send new patches as soon as possible!

Signature · 1 line
Alexandre Bique

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#13 lore raw
Quoted 7 lines · On Friday 07 August 2009 17:06:36 Juan Quintela wrote:
Quoted 1 line · Bique Alexandre

The ATAPI pass through feature.

+# define CHECK_SAME_VALUE(Val1, Val2)

Can we call this something like: assert_equal() ? or anything else more descriptive?

It's not an assertion because Val1 and Val2 are allowed to be different and it doesn't call abort. It just displays some debut information. Would CHECK_EQUAL be alright for you ?

Quoted 10 lines
hw/atapi-pt.c new file +6 -0
+/* The generic packet command opcodes for CD/DVD Logical Units,
+ * From Table 57 of the SFF8090 Ver. 3 (Mt. Fuji) draft standard. */
+static const struct {
+    unsigned short packet_command;
+    const char * const text;
+} packet_command_texts[] = {

Please, use C99 intializers:

Quoted 8 lines

+ { GPCMD_TEST_UNIT_READY, "Test Unit Ready" },

use this format, same for rest of structs

{
.packet_command = GPCMD_TEST_UNIT_READY,
.text = "Test Unit Ready"
},

I see no reason to do that. It takes more place. We are not going to add a lot of additional fields. And even if we do, this structure should be used only time at only one place and if we decide to add a new field in the beginning or the middle of this structure, it should be worth to do some regexp to fix the declaration.

Quoted 23 lines
hw/atapi-pt.c new file +13 -0
+static void ide_atapi_pt_standard_reply(IDEState *s)
+{
+    uint32_t size = s->atapi_pt.reply_size_init;
+
+    switch (s->atapi_pt.reply_size_len)
+    {
+    case 0:
+        break;
+    case 1:
+        size += s->io_buffer[s->atapi_pt.reply_size_offset];
+        break;
+    case 2:
+        size += ube16_to_cpu(s->io_buffer +

s->atapi_pt.reply_size_offset); + break;

(quoted diff) +2 -0
+    case 3:
+        size += ube24_to_cpu(s->io_buffer +

s->atapi_pt.reply_size_offset); + break;

(quoted diff) +2 -0
+    case 4:
+        size += ube32_to_cpu(s->io_buffer +

s->atapi_pt.reply_size_offset); + break;

hw/atapi-pt.c new file +2 -0
+    default:
+        assert(0);
         ^^^^^^^
Quoted 3 lines

print a nice error message?
die?
something?

I Will do it, but what do you want me to say ?
"We reached a part of the code we should never reach. Please send a bug report to Qemu developers. Thanks." ?

Quoted 24 lines
hw/atapi-pt.c new file +21 -0
+        break;
+    }
 
+static int ide_atapi_pt_read_cd_block_size(const uint8_t *io_buffer)
+{
+    int sector_type = (io_buffer[2] >> 2) & 7;
+    int error_flags = (io_buffer[9] >> 1) & 3;
+    int flags_bits = io_buffer[9] & ~7;
+    int block_size = 0;
+
+    // expected sector type
+    switch (sector_type)
+    {
+    case 0: // Any type
+    case 1: // CD-DA
+        block_size = (flags_bits) ? 2352 : 0;
+        break;
+
+    case 2: // Mode 1
+        switch (flags_bits)
+        {
+        case 0x0: block_size = 0; break;

case 0x40: block_size = 0; break;

Quoted 2 lines

move this one here, same fro the same two with 16 value? group all of them by block_size? Same for the rest of the cases.

I can but I don't want to. Because if you want to double check this, you would prefer to see this sorted so you can check the reference table at the same time you check your switch case.

Quoted 25 lines
hw/atapi-pt.c new file +22 -0
+        case 0x10:
+        case 0x50: block_size = 2048; break;
+        case 0x18:
+        case 0x58: block_size = 2336; break;
+        case 0x20:
+        case 0x60: block_size = 4; break;
+        case 0x30:
+        case 0x70:
+        case 0x78: block_size = 2052; break;
+        case 0x38: block_size = 2340; break;
+        case 0xa0: block_size = 16; break;
+        case 0xb0: block_size = 2064; break;
+        case 0xb8: block_size = 2352; break;
+        case 0xe0: block_size = 16; break;
+        case 0xf0: block_size = 2064; break;
+        case 0xf8: block_size = 2352; break;
+
+        default: return 0; // illegal
+        }
 
 
+#if CONFIG_ATAPI_PT
+#include "atapi-pt.c"
+#endif
Quoted 1 line

Why do we include it here? coulden't yust compile it as a new file?

I did. I am going to send you my new patches where I made some part of ide.c public and introduced 3 new headers.

Thanks for review.

Signature · 1 line
Alexandre Bique

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#15 lore raw
Quoted 38 lines · On Friday 07 August 2009 17:46:00 Juan Quintela wrote:
Quoted 26 lines · Bique Alexandre
Quoted 7 lines · On Friday 07 August 2009 17:06:36 Juan Quintela wrote:
Quoted 1 line · Bique Alexandre

The ATAPI pass through feature.

+# define CHECK_SAME_VALUE(Val1, Val2)

Can we call this something like: assert_equal() ? or anything else more descriptive?

It's not an assertion because Val1 and Val2 are allowed to be different and it doesn't call abort. It just displays some debut information. Would CHECK_EQUAL be alright for you ?

Quoted 13 lines
hw/atapi-pt.c new file +6 -0
+/* The generic packet command opcodes for CD/DVD Logical Units,
+ * From Table 57 of the SFF8090 Ver. 3 (Mt. Fuji) draft standard. */
+static const struct {
+    unsigned short packet_command;
+    const char * const text;
+} packet_command_texts[] = {

Please, use C99 intializers:

+ { GPCMD_TEST_UNIT_READY, "Test Unit Ready" },

use this format, same for rest of structs

Quoted 10 lines
Quoted 4 lines

{
.packet_command = GPCMD_TEST_UNIT_READY,
.text = "Test Unit Ready"
},

I see no reason to do that. It takes more place. We are not going to add a lot of additional fields. And even if we do, this structure should be used only time at only one place and if we decide to add a new field in the beginning or the middle of this structure, it should be worth to do some regexp to fix the declaration.

Quoted 2 lines

Being coherent with everyhing else (new fields use C99 initilizers). Consistence is important IMHO.

C89 features are also part of C99. I agree that C99 initializers is a great feature, but I really feel that it's not the right place to use it :)

Quoted 31 lines
Quoted 22 lines
Quoted 22 lines
hw/atapi-pt.c new file +13 -0
+static void ide_atapi_pt_standard_reply(IDEState *s)
+{
+    uint32_t size = s->atapi_pt.reply_size_init;
+
+    switch (s->atapi_pt.reply_size_len)
+    {
+    case 0:
+        break;
+    case 1:
+        size += s->io_buffer[s->atapi_pt.reply_size_offset];
+        break;
+    case 2:
+        size += ube16_to_cpu(s->io_buffer +

s->atapi_pt.reply_size_offset); + break;

(quoted diff) +2 -0
+    case 3:
+        size += ube24_to_cpu(s->io_buffer +

s->atapi_pt.reply_size_offset); + break;

(quoted diff) +2 -0
+    case 4:
+        size += ube32_to_cpu(s->io_buffer +

s->atapi_pt.reply_size_offset); + break;

hw/atapi-pt.c new file +2 -0
+    default:
+        assert(0);
Quoted 8 lines
Quoted 4 lines
         ^^^^^^^

print a nice error message?
die?
something?

I Will do it, but what do you want me to say ?
"We reached a part of the code we should never reach. Please send a bug report to Qemu developers. Thanks." ?

Quoted 4 lines

The imposible has happened!!! We received a reply with size %d. Please inform <[email protected]>.

Not sure about the mail/web adderss, etc.

Hey fun. For sure I'll add this :)

Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
hw/atapi-pt.c new file +21 -0
+        break;
+    }
 
+static int ide_atapi_pt_read_cd_block_size(const uint8_t *io_buffer)
+{
+    int sector_type = (io_buffer[2] >> 2) & 7;
+    int error_flags = (io_buffer[9] >> 1) & 3;
+    int flags_bits = io_buffer[9] & ~7;
+    int block_size = 0;
+
+    // expected sector type
+    switch (sector_type)
+    {
+    case 0: // Any type
+    case 1: // CD-DA
+        block_size = (flags_bits) ? 2352 : 0;
+        break;
+
+    case 2: // Mode 1
+        switch (flags_bits)
+        {
+        case 0x0: block_size = 0; break;

case 0x40: block_size = 0; break;

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

move this one here, same fro the same two with 16 value? group all of them by block_size? Same for the rest of the cases.

I can but I don't want to. Because if you want to double check this, you would prefer to see this sorted so you can check the reference table at the same time you check your switch case.

Then, were is the reference table pointer? :)

It comes from the "Mt. Fuji Commands for Multimedia Devices SFF8090i v4" page 343.

Quoted 5 lines

One comment indicating it would be nice.

/* The order of this case is the same that table number foo at page X */

Otherwise, it looks very random.

Done.

Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
hw/atapi-pt.c new file +22 -0
+        case 0x10:
+        case 0x50: block_size = 2048; break;
+        case 0x18:
+        case 0x58: block_size = 2336; break;
+        case 0x20:
+        case 0x60: block_size = 4; break;
+        case 0x30:
+        case 0x70:
+        case 0x78: block_size = 2052; break;
+        case 0x38: block_size = 2340; break;
+        case 0xa0: block_size = 16; break;
+        case 0xb0: block_size = 2064; break;
+        case 0xb8: block_size = 2352; break;
+        case 0xe0: block_size = 16; break;
+        case 0xf0: block_size = 2064; break;
+        case 0xf8: block_size = 2352; break;
+
+        default: return 0; // illegal
+        }
 
 
+#if CONFIG_ATAPI_PT
+#include "atapi-pt.c"
+#endif
Quoted 8 lines
Quoted 6 lines
Quoted 1 line

Why do we include it here? coulden't yust compile it as a new file?

I did. I am going to send you my new patches where I made some part of ide.c public and introduced 3 new headers.

Thanks for review.

You are welcome.

Thanks. I send the new patches with git format and git send-mail this time :-).

Signature · 1 line
Alexandre Bique