ide: Use ide_cmd_table function pointers for command execution

PATCH In review Kevin Wolf 2 participants 20 messages Jun 18, 2013 – Jun 19, 2013

Share & tools

Kevin Wolf

author

2013-06-18 08:26 UTC #0 lore raw

cover ide: Use ide_cmd_table function pointers for command execution

The ATAPI code has been using a table of callback functions for each command for quite a while now, this series does the same for ATA commands (which already got the table, but no function pointers, just flags)

The main advantage is that we get a more consistent behaviour of the commands because we get a clear separation between things that are common between all (or most) commands, like setting status register bits or triggering IRQs, and can be handled in one central place, and command specific code that is refactored into its own function. In the past we've had bugs from forgetting to do one of the common things, like setting the BSY flag, which will be avoided with the new infrastructure.

It's also a good opportunity to clean up some of the coding style problems of the IDE code.

Kevin Wolf (17):
ide: Add handler to ide_cmd_table
ide: Convert WIN_DSM to ide_cmd_table handler
ide: Convert WIN_IDENTIFY to ide_cmd_table handler
ide: Convert cmd_nop commands to ide_cmd_table handler
ide: Convert verify commands to ide_cmd_table handler
ide: Convert read/write multiple commands to ide_cmd_table handler
ide: Convert PIO read/write commands to ide_cmd_table handler
ide: Convert DMA read/write commands to ide_cmd_table handler
ide: Convert READ NATIVE MAX ADDRESS to ide_cmd_table handler
ide: Convert CHECK POWER MDOE to ide_cmd_table handler
ide: Convert SET FEATURES to ide_cmd_table handler
ide: Convert FLUSH CACHE to ide_cmd_table handler
ide: Convert SEEK to ide_cmd_table handler
ide: Convert ATAPI commands to ide_cmd_table handler
ide: Convert CF-ATA commands to ide_cmd_table handler
ide: Convert SMART commands to ide_cmd_table handler
ide: Clean up ide_exec_cmd()

 hw/ide/core.c | 1242 +++++++++++++++++++++++++++++++--------------------------
 1 file changed, 669 insertions(+), 573 deletions(-)

Signature · 1 line

1.8.1.4

Kevin Wolf

author

2013-06-18 08:26 UTC #1 lore raw

PATCH 1/17 ide: Add handler to ide_cmd_table

As a preparation for moving all IDE commands into their own function like in the ATAPI code, introduce a 'handler' callback to ide_cmd_table.

Commands using this new infrastructure get some things handled automatically:

* The BSY flag is set before calling the handler (in order to avoid bugs like the one fixed in f68ec837) and reset on completion.

* The (obsolete) DSC flag in the status register is set on completion if the command is flagged with SET_DSC in the command table

* An IRQ is triggered on completion.

* The error register and the ERR flag in the status register are cleared before calling the handler and on completion it is asserted that either none or both of them are set.

No commands are converted at this point.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/ide/core.c | 144 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 86 insertions(+), 58 deletions(-)

hw/ide/core.c +86 ~~-58~~

index 9926d92..cd9de14 100644
@@ -1010,71 +1010,78 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 #define HD_CFA_OK (HD_OK | CFA_OK)
 #define ALL_OK (HD_OK | CD_OK | CFA_OK)
 
+/* Set the Disk Seek Completed status bit during completion */
+#define SET_DSC (1u << 8)
+
 /* See ACS-2 T13/2015-D Table B.2 Command codes */
-static const uint8_t ide_cmd_table[0x100] = {
+static const struct {
+    /* Returns true if the completion code should be run */
+    bool (*handler)(IDEState *s, uint8_t cmd);
+    int flags;
+} ide_cmd_table[0x100] = {
     /* NOP not implemented, mandatory for CD */
-    [CFA_REQ_EXT_ERROR_CODE]            = CFA_OK,
-    [WIN_DSM]                           = ALL_OK,
-    [WIN_DEVICE_RESET]                  = CD_OK,
-    [WIN_RECAL]                         = HD_CFA_OK,
-    [WIN_READ]                          = ALL_OK,
-    [WIN_READ_ONCE]                     = ALL_OK,
-    [WIN_READ_EXT]                      = HD_CFA_OK,
-    [WIN_READDMA_EXT]                   = HD_CFA_OK,
-    [WIN_READ_NATIVE_MAX_EXT]           = HD_CFA_OK,
-    [WIN_MULTREAD_EXT]                  = HD_CFA_OK,
-    [WIN_WRITE]                         = HD_CFA_OK,
-    [WIN_WRITE_ONCE]                    = HD_CFA_OK,
-    [WIN_WRITE_EXT]                     = HD_CFA_OK,
-    [WIN_WRITEDMA_EXT]                  = HD_CFA_OK,
-    [CFA_WRITE_SECT_WO_ERASE]           = CFA_OK,
-    [WIN_MULTWRITE_EXT]                 = HD_CFA_OK,
-    [WIN_WRITE_VERIFY]                  = HD_CFA_OK,
-    [WIN_VERIFY]                        = HD_CFA_OK,
-    [WIN_VERIFY_ONCE]                   = HD_CFA_OK,
-    [WIN_VERIFY_EXT]                    = HD_CFA_OK,
-    [WIN_SEEK]                          = HD_CFA_OK,
-    [CFA_TRANSLATE_SECTOR]              = CFA_OK,
-    [WIN_DIAGNOSE]                      = ALL_OK,
-    [WIN_SPECIFY]                       = HD_CFA_OK,
-    [WIN_STANDBYNOW2]                   = ALL_OK,
-    [WIN_IDLEIMMEDIATE2]                = ALL_OK,
-    [WIN_STANDBY2]                      = ALL_OK,
-    [WIN_SETIDLE2]                      = ALL_OK,
-    [WIN_CHECKPOWERMODE2]               = ALL_OK,
-    [WIN_SLEEPNOW2]                     = ALL_OK,
-    [WIN_PACKETCMD]                     = CD_OK,
-    [WIN_PIDENTIFY]                     = CD_OK,
-    [WIN_SMART]                         = HD_CFA_OK,
-    [CFA_ACCESS_METADATA_STORAGE]       = CFA_OK,
-    [CFA_ERASE_SECTORS]                 = CFA_OK,
-    [WIN_MULTREAD]                      = HD_CFA_OK,
-    [WIN_MULTWRITE]                     = HD_CFA_OK,
-    [WIN_SETMULT]                       = HD_CFA_OK,
-    [WIN_READDMA]                       = HD_CFA_OK,
-    [WIN_READDMA_ONCE]                  = HD_CFA_OK,
-    [WIN_WRITEDMA]                      = HD_CFA_OK,
-    [WIN_WRITEDMA_ONCE]                 = HD_CFA_OK,
-    [CFA_WRITE_MULTI_WO_ERASE]          = CFA_OK,
-    [WIN_STANDBYNOW1]                   = ALL_OK,
-    [WIN_IDLEIMMEDIATE]                 = ALL_OK,
-    [WIN_STANDBY]                       = ALL_OK,
-    [WIN_SETIDLE1]                      = ALL_OK,
-    [WIN_CHECKPOWERMODE1]               = ALL_OK,
-    [WIN_SLEEPNOW1]                     = ALL_OK,
-    [WIN_FLUSH_CACHE]                   = ALL_OK,
-    [WIN_FLUSH_CACHE_EXT]               = HD_CFA_OK,
-    [WIN_IDENTIFY]                      = ALL_OK,
-    [WIN_SETFEATURES]                   = ALL_OK,
-    [IBM_SENSE_CONDITION]               = CFA_OK,
-    [CFA_WEAR_LEVEL]                    = HD_CFA_OK,
-    [WIN_READ_NATIVE_MAX]               = ALL_OK,
+    [CFA_REQ_EXT_ERROR_CODE]      = { NULL, CFA_OK },
+    [WIN_DSM]                     = { NULL, ALL_OK },
+    [WIN_DEVICE_RESET]            = { NULL, CD_OK },
+    [WIN_RECAL]                   = { NULL, HD_CFA_OK },
+    [WIN_READ]                    = { NULL, ALL_OK },
+    [WIN_READ_ONCE]               = { NULL, ALL_OK },
+    [WIN_READ_EXT]                = { NULL, HD_CFA_OK },
+    [WIN_READDMA_EXT]             = { NULL, HD_CFA_OK },
+    [WIN_READ_NATIVE_MAX_EXT]     = { NULL, HD_CFA_OK },
+    [WIN_MULTREAD_EXT]            = { NULL, HD_CFA_OK },
+    [WIN_WRITE]                   = { NULL, HD_CFA_OK },
+    [WIN_WRITE_ONCE]              = { NULL, HD_CFA_OK },
+    [WIN_WRITE_EXT]               = { NULL, HD_CFA_OK },
+    [WIN_WRITEDMA_EXT]            = { NULL, HD_CFA_OK },
+    [CFA_WRITE_SECT_WO_ERASE]     = { NULL, CFA_OK },
+    [WIN_MULTWRITE_EXT]           = { NULL, HD_CFA_OK },
+    [WIN_WRITE_VERIFY]            = { NULL, HD_CFA_OK },
+    [WIN_VERIFY]                  = { NULL, HD_CFA_OK },
+    [WIN_VERIFY_ONCE]             = { NULL, HD_CFA_OK },
+    [WIN_VERIFY_EXT]              = { NULL, HD_CFA_OK },
+    [WIN_SEEK]                    = { NULL, HD_CFA_OK },
+    [CFA_TRANSLATE_SECTOR]        = { NULL, CFA_OK },
+    [WIN_DIAGNOSE]                = { NULL, ALL_OK },
+    [WIN_SPECIFY]                 = { NULL, HD_CFA_OK },
+    [WIN_STANDBYNOW2]             = { NULL, ALL_OK },
+    [WIN_IDLEIMMEDIATE2]          = { NULL, ALL_OK },
+    [WIN_STANDBY2]                = { NULL, ALL_OK },
+    [WIN_SETIDLE2]                = { NULL, ALL_OK },
+    [WIN_CHECKPOWERMODE2]         = { NULL, ALL_OK },
+    [WIN_SLEEPNOW2]               = { NULL, ALL_OK },
+    [WIN_PACKETCMD]               = { NULL, CD_OK },
+    [WIN_PIDENTIFY]               = { NULL, CD_OK },
+    [WIN_SMART]                   = { NULL, HD_CFA_OK },
+    [CFA_ACCESS_METADATA_STORAGE] = { NULL, CFA_OK },
+    [CFA_ERASE_SECTORS]           = { NULL, CFA_OK },
+    [WIN_MULTREAD]                = { NULL, HD_CFA_OK },
+    [WIN_MULTWRITE]               = { NULL, HD_CFA_OK },
+    [WIN_SETMULT]                 = { NULL, HD_CFA_OK },
+    [WIN_READDMA]                 = { NULL, HD_CFA_OK },
+    [WIN_READDMA_ONCE]            = { NULL, HD_CFA_OK },
+    [WIN_WRITEDMA]                = { NULL, HD_CFA_OK },
+    [WIN_WRITEDMA_ONCE]           = { NULL, HD_CFA_OK },
+    [CFA_WRITE_MULTI_WO_ERASE]    = { NULL, CFA_OK },
+    [WIN_STANDBYNOW1]             = { NULL, ALL_OK },
+    [WIN_IDLEIMMEDIATE]           = { NULL, ALL_OK },
+    [WIN_STANDBY]                 = { NULL, ALL_OK },
+    [WIN_SETIDLE1]                = { NULL, ALL_OK },
+    [WIN_CHECKPOWERMODE1]         = { NULL, ALL_OK },
+    [WIN_SLEEPNOW1]               = { NULL, ALL_OK },
+    [WIN_FLUSH_CACHE]             = { NULL, ALL_OK },
+    [WIN_FLUSH_CACHE_EXT]         = { NULL, HD_CFA_OK },
+    [WIN_IDENTIFY]                = { NULL, ALL_OK },
+    [WIN_SETFEATURES]             = { NULL, ALL_OK },
+    [IBM_SENSE_CONDITION]         = { NULL, CFA_OK },
+    [CFA_WEAR_LEVEL]              = { NULL, HD_CFA_OK },
+    [WIN_READ_NATIVE_MAX]         = { NULL, ALL_OK },
 };
 
 static bool ide_cmd_permitted(IDEState *s, uint32_t cmd)
 {
     return cmd < ARRAY_SIZE(ide_cmd_table)
-        && (ide_cmd_table[cmd] & (1u << s->drive_kind));
+        && (ide_cmd_table[cmd].flags & (1u << s->drive_kind));
 }
 
 void ide_exec_cmd(IDEBus *bus, uint32_t val)

@@ -1100,6 +1107,27 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
         goto abort_cmd;
     }
 
+    if (ide_cmd_table[val].handler != NULL) {
+        bool complete;
+
+        s->status = READY_STAT | BUSY_STAT;
+        s->error = 0;
+
+        complete = ide_cmd_table[val].handler(s, val);
+        if (complete) {
+            s->status &= ~BUSY_STAT;
+            assert(!!s->error == !!(s->status & ERR_STAT));
+
+            if ((ide_cmd_table[val].flags & SET_DSC) && !s->error) {
+                s->status |= SEEK_STAT;
+            }
+
+            ide_set_irq(s->bus);
+        }
+
+        return;
+    }
+
     switch(val) {
     case WIN_DSM:
         switch (s->feature) {

Signature · 1 line

1.8.1.4

Stefan Hajnoczi

2013-06-19 11:46 UTC #2 lore raw

Re: [Qemu-devel] [PATCH 01/17] ide: Add handler to ide_cmd_table

Quoted 30 lines · Kevin Wolf

As a preparation for moving all IDE commands into their own function like in the ATAPI code, introduce a 'handler' callback to ide_cmd_table.
Commands using this new infrastructure get some things handled automatically:
* The BSY flag is set before calling the handler (in order to avoid bugs like the one fixed in f68ec837) and reset on completion.
* The (obsolete) DSC flag in the status register is set on completion if the command is flagged with SET_DSC in the command table
* An IRQ is triggered on completion.
* The error register and the ERR flag in the status register are cleared before calling the handler and on completion it is asserted that either none or both of them are set.
No commands are converted at this point.
Signed-off-by Kevin Wolf <[email protected]>
---
 hw/ide/core.c | 144 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 86 insertions(+), 58 deletions(-)
hw/ide/core.c +0 -0
index 9926d92..cd9de14 100644

hw/ide/core.c void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val) · line 1014

Quoted 5 lines

hw/ide/core.c +2 -0

@@ -1010,71 +1010,78 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 #define HD_CFA_OK (HD_OK | CFA_OK)
 #define ALL_OK (HD_OK | CD_OK | CFA_OK)
 
+/* Set the Disk Seek Completed status bit during completion */
+#define SET_DSC (1u << 8)

If DriveKind changes there could be unnoticed collisions and in the meantime we have empty flags bits...

Creating a single IDEHandlerFlags enum would still depend on IDEDriveKind constants so I don't see a nicer solution than what you've done. We can live with this.

Kevin Wolf

author

2013-06-18 08:26 UTC #3 lore raw

PATCH 2/17 ide: Convert WIN_DSM to ide_cmd_table handler