Patchwork [v2,07/45] ide: Use a table to declare which drive kinds accept each command

login
register
mail settings
Submitter Markus Armbruster
Date Aug. 3, 2011, 1:07 p.m.
Message ID <1312376904-16115-8-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/108182/
State New
Headers show

Comments

Markus Armbruster - Aug. 3, 2011, 1:07 p.m.
No functional change.

It would be nice to have handler functions in the table, like commit
e1a064f9 did for ATAPI.  Left for another day.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c |  104 +++++++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 79 insertions(+), 25 deletions(-)
Blue Swirl - Aug. 3, 2011, 4:53 p.m.
On Wed, Aug 3, 2011 at 1:07 PM, Markus Armbruster <armbru@redhat.com> wrote:
> No functional change.
>
> It would be nice to have handler functions in the table, like commit
> e1a064f9 did for ATAPI.  Left for another day.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ide/core.c |  104 +++++++++++++++++++++++++++++++++++++++++++--------------
>  1 files changed, 79 insertions(+), 25 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 1c4dc2f..a25c175 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -876,6 +876,77 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>     }
>  }
>
> +#define HD_OK (1u << IDE_HD)
> +#define CD_OK (1u << IDE_CD)
> +#define CFA_OK (1u << IDE_CFATA)
> +#define HD_CFA_OK (HD_OK | CFA_OK)
> +#define ALL_OK (HD_OK | CD_OK | CFA_OK)
> +
> +/* See ACS-2 T13/2015-D Table B.2 Command codes */
> +uint8_t ide_cmd_table[0x100] = {

Missing 'static'.

> +    /* NOP not implemented, mandatory for CD */
> +    [CFA_REQ_EXT_ERROR_CODE]            = CFA_OK,
> +    [WIN_DSM]                           = ALL_OK,
> +    [WIN_DEVICE_RESET]                  = CD_OK,
> +    [WIN_RECAL]                         = ALL_OK,
> +    [WIN_READ]                          = ALL_OK,
> +    [WIN_READ_ONCE]                     = ALL_OK,
> +    [WIN_READ_EXT]                      = ALL_OK,
> +    [WIN_READDMA_EXT]                   = ALL_OK,
> +    [WIN_READ_NATIVE_MAX_EXT]           = ALL_OK,
> +    [WIN_MULTREAD_EXT]                  = ALL_OK,
> +    [WIN_WRITE]                         = ALL_OK,
> +    [WIN_WRITE_ONCE]                    = ALL_OK,
> +    [WIN_WRITE_EXT]                     = ALL_OK,
> +    [WIN_WRITEDMA_EXT]                  = ALL_OK,
> +    [CFA_WRITE_SECT_WO_ERASE]           = ALL_OK,
> +    [WIN_MULTWRITE_EXT]                 = ALL_OK,
> +    [WIN_WRITE_VERIFY]                  = ALL_OK,
> +    [WIN_VERIFY]                        = ALL_OK,
> +    [WIN_VERIFY_ONCE]                   = ALL_OK,
> +    [WIN_VERIFY_EXT]                    = ALL_OK,
> +    [WIN_SEEK]                          = HD_CFA_OK,
> +    [CFA_TRANSLATE_SECTOR]              = CFA_OK,
> +    [WIN_DIAGNOSE]                      = ALL_OK,
> +    [WIN_SPECIFY]                       = ALL_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]                      = ALL_OK,
> +    [WIN_MULTWRITE]                     = ALL_OK,
> +    [WIN_SETMULT]                       = ALL_OK,
> +    [WIN_READDMA]                       = ALL_OK,
> +    [WIN_READDMA_ONCE]                  = ALL_OK,
> +    [WIN_WRITEDMA]                      = ALL_OK,
> +    [WIN_WRITEDMA_ONCE]                 = ALL_OK,
> +    [CFA_WRITE_MULTI_WO_ERASE]          = ALL_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]               = ALL_OK,
> +    [WIN_IDENTIFY]                      = ALL_OK,
> +    [WIN_SETFEATURES]                   = ALL_OK,
> +    [IBM_SENSE_CONDITION]               = CFA_OK,
> +    [CFA_WEAR_LEVEL]                    = CFA_OK,
> +    [WIN_READ_NATIVE_MAX]               = ALL_OK,
> +};
> +
> +static bool ide_cmd_permitted(IDEState *s, uint32_t cmd)
> +{
> +    return cmd <= 0xff && (ide_cmd_table[cmd] & (1u << s->drive_kind));
> +}
>
>  void ide_exec_cmd(IDEBus *bus, uint32_t val)
>  {
> @@ -895,6 +966,10 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>     if ((s->status & (BUSY_STAT|DRQ_STAT)) && val != WIN_DEVICE_RESET)
>         return;
>
> +    if (!ide_cmd_permitted(s, val)) {
> +        goto abort_cmd;
> +    }
> +
>     switch(val) {
>     case WIN_DSM:
>         switch (s->feature) {
> @@ -1115,21 +1190,15 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>         ide_set_irq(s->bus);
>         break;
>     case WIN_SEEK:
> -        if(s->drive_kind == IDE_CD)
> -            goto abort_cmd;
>         /* XXX: Check that seek is within bounds */
>         s->status = READY_STAT | SEEK_STAT;
>         ide_set_irq(s->bus);
>         break;
>         /* ATAPI commands */
>     case WIN_PIDENTIFY:
> -        if (s->drive_kind == IDE_CD) {
> -            ide_atapi_identify(s);
> -            s->status = READY_STAT | SEEK_STAT;
> -            ide_transfer_start(s, s->io_buffer, 512, ide_transfer_stop);
> -        } else {
> -            ide_abort_command(s);
> -        }
> +        ide_atapi_identify(s);
> +        s->status = READY_STAT | SEEK_STAT;
> +        ide_transfer_start(s, s->io_buffer, 512, ide_transfer_stop);
>         ide_set_irq(s->bus);
>         break;
>     case WIN_DIAGNOSE:
> @@ -1146,15 +1215,11 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>         ide_set_irq(s->bus);
>         break;
>     case WIN_DEVICE_RESET:
> -        if (s->drive_kind != IDE_CD)
> -            goto abort_cmd;
>         ide_set_signature(s);
>         s->status = 0x00; /* NOTE: READY is _not_ set */
>         s->error = 0x01;
>         break;
>     case WIN_PACKETCMD:
> -        if (s->drive_kind != IDE_CD)
> -            goto abort_cmd;
>         /* overlapping commands not supported */
>         if (s->feature & 0x02)
>             goto abort_cmd;
> @@ -1166,16 +1231,12 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>         break;
>     /* CF-ATA commands */
>     case CFA_REQ_EXT_ERROR_CODE:
> -        if (s->drive_kind != IDE_CFATA)
> -            goto abort_cmd;
>         s->error = 0x09;    /* miscellaneous error */
>         s->status = READY_STAT | SEEK_STAT;
>         ide_set_irq(s->bus);
>         break;
>     case CFA_ERASE_SECTORS:
>     case CFA_WEAR_LEVEL:
> -        if (s->drive_kind != IDE_CFATA)
> -            goto abort_cmd;
>         if (val == CFA_WEAR_LEVEL)
>             s->nsector = 0;
>         if (val == CFA_ERASE_SECTORS)
> @@ -1185,8 +1246,6 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>         ide_set_irq(s->bus);
>         break;
>     case CFA_TRANSLATE_SECTOR:
> -        if (s->drive_kind != IDE_CFATA)
> -            goto abort_cmd;
>         s->error = 0x00;
>         s->status = READY_STAT | SEEK_STAT;
>         memset(s->io_buffer, 0, 0x200);
> @@ -1205,8 +1264,6 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>         ide_set_irq(s->bus);
>         break;
>     case CFA_ACCESS_METADATA_STORAGE:
> -        if (s->drive_kind != IDE_CFATA)
> -            goto abort_cmd;
>         switch (s->feature) {
>         case 0x02:     /* Inquiry Metadata Storage */
>             ide_cfata_metadata_inquiry(s);
> @@ -1225,8 +1282,6 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>         ide_set_irq(s->bus);
>         break;
>     case IBM_SENSE_CONDITION:
> -        if (s->drive_kind != IDE_CFATA)
> -            goto abort_cmd;
>         switch (s->feature) {
>         case 0x01:  /* sense temperature in device */
>             s->nsector = 0x50;      /* +20 C */
> @@ -1239,8 +1294,6 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>         break;
>
>     case WIN_SMART:
> -       if (s->drive_kind == IDE_CD)
> -               goto abort_cmd;
>        if (s->hcyl != 0xc2 || s->lcyl != 0x4f)
>                goto abort_cmd;
>        if (!s->smart_enabled && s->feature != SMART_ENABLE)
> @@ -1395,6 +1448,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>        }
>        break;
>     default:
> +        /* should not be reachable */
>     abort_cmd:
>         ide_abort_command(s);
>         ide_set_irq(s->bus);
> --
> 1.7.6
>
>
>
Markus Armbruster - Aug. 3, 2011, 5:15 p.m.
Blue Swirl <blauwirbel@gmail.com> writes:

> On Wed, Aug 3, 2011 at 1:07 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> No functional change.
>>
>> It would be nice to have handler functions in the table, like commit
>> e1a064f9 did for ATAPI.  Left for another day.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/ide/core.c |  104 +++++++++++++++++++++++++++++++++++++++++++--------------
>>  1 files changed, 79 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 1c4dc2f..a25c175 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -876,6 +876,77 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>     }
>>  }
>>
>> +#define HD_OK (1u << IDE_HD)
>> +#define CD_OK (1u << IDE_CD)
>> +#define CFA_OK (1u << IDE_CFATA)
>> +#define HD_CFA_OK (HD_OK | CFA_OK)
>> +#define ALL_OK (HD_OK | CD_OK | CFA_OK)
>> +
>> +/* See ACS-2 T13/2015-D Table B.2 Command codes */
>> +uint8_t ide_cmd_table[0x100] = {
>
> Missing 'static'.

Thanks, will fix.

[...]
Kevin Wolf - Sept. 2, 2011, 10:08 a.m.
Am 03.08.2011 18:53, schrieb Blue Swirl:
> On Wed, Aug 3, 2011 at 1:07 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> No functional change.
>>
>> It would be nice to have handler functions in the table, like commit
>> e1a064f9 did for ATAPI.  Left for another day.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/ide/core.c |  104 +++++++++++++++++++++++++++++++++++++++++++--------------
>>  1 files changed, 79 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 1c4dc2f..a25c175 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -876,6 +876,77 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>     }
>>  }
>>
>> +#define HD_OK (1u << IDE_HD)
>> +#define CD_OK (1u << IDE_CD)
>> +#define CFA_OK (1u << IDE_CFATA)
>> +#define HD_CFA_OK (HD_OK | CFA_OK)
>> +#define ALL_OK (HD_OK | CD_OK | CFA_OK)
>> +
>> +/* See ACS-2 T13/2015-D Table B.2 Command codes */
>> +uint8_t ide_cmd_table[0x100] = {
> 
> Missing 'static'.

And const, while we're at it.

>> +    /* NOP not implemented, mandatory for CD */
>> +    [CFA_REQ_EXT_ERROR_CODE]            = CFA_OK,
>> +    [WIN_DSM]                           = ALL_OK,
>> +    [WIN_DEVICE_RESET]                  = CD_OK,
>> +    [WIN_RECAL]                         = ALL_OK,
>> +    [WIN_READ]                          = ALL_OK,
>> +    [WIN_READ_ONCE]                     = ALL_OK,
>> +    [WIN_READ_EXT]                      = ALL_OK,
>> +    [WIN_READDMA_EXT]                   = ALL_OK,
>> +    [WIN_READ_NATIVE_MAX_EXT]           = ALL_OK,
>> +    [WIN_MULTREAD_EXT]                  = ALL_OK,
>> +    [WIN_WRITE]                         = ALL_OK,
>> +    [WIN_WRITE_ONCE]                    = ALL_OK,
>> +    [WIN_WRITE_EXT]                     = ALL_OK,
>> +    [WIN_WRITEDMA_EXT]                  = ALL_OK,
>> +    [CFA_WRITE_SECT_WO_ERASE]           = ALL_OK,
>> +    [WIN_MULTWRITE_EXT]                 = ALL_OK,
>> +    [WIN_WRITE_VERIFY]                  = ALL_OK,
>> +    [WIN_VERIFY]                        = ALL_OK,
>> +    [WIN_VERIFY_ONCE]                   = ALL_OK,
>> +    [WIN_VERIFY_EXT]                    = ALL_OK,
>> +    [WIN_SEEK]                          = HD_CFA_OK,
>> +    [CFA_TRANSLATE_SECTOR]              = CFA_OK,
>> +    [WIN_DIAGNOSE]                      = ALL_OK,
>> +    [WIN_SPECIFY]                       = ALL_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]                      = ALL_OK,
>> +    [WIN_MULTWRITE]                     = ALL_OK,
>> +    [WIN_SETMULT]                       = ALL_OK,
>> +    [WIN_READDMA]                       = ALL_OK,
>> +    [WIN_READDMA_ONCE]                  = ALL_OK,
>> +    [WIN_WRITEDMA]                      = ALL_OK,
>> +    [WIN_WRITEDMA_ONCE]                 = ALL_OK,
>> +    [CFA_WRITE_MULTI_WO_ERASE]          = ALL_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]               = ALL_OK,
>> +    [WIN_IDENTIFY]                      = ALL_OK,
>> +    [WIN_SETFEATURES]                   = ALL_OK,
>> +    [IBM_SENSE_CONDITION]               = CFA_OK,
>> +    [CFA_WEAR_LEVEL]                    = CFA_OK,
>> +    [WIN_READ_NATIVE_MAX]               = ALL_OK,
>> +};
>> +
>> +static bool ide_cmd_permitted(IDEState *s, uint32_t cmd)
>> +{
>> +    return cmd <= 0xff && (ide_cmd_table[cmd] & (1u << s->drive_kind));
>> +}

Doesn't cmd < ARRAY_SIZE(ide_cmd_table) better describe what you want?

Kevin
Markus Armbruster - Sept. 2, 2011, 2:43 p.m.
Kevin Wolf <kwolf@redhat.com> writes:

> Am 03.08.2011 18:53, schrieb Blue Swirl:
>> On Wed, Aug 3, 2011 at 1:07 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> No functional change.
>>>
>>> It would be nice to have handler functions in the table, like commit
>>> e1a064f9 did for ATAPI.  Left for another day.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  hw/ide/core.c |  104 +++++++++++++++++++++++++++++++++++++++++++--------------
>>>  1 files changed, 79 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index 1c4dc2f..a25c175 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -876,6 +876,77 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>>     }
>>>  }
>>>
>>> +#define HD_OK (1u << IDE_HD)
>>> +#define CD_OK (1u << IDE_CD)
>>> +#define CFA_OK (1u << IDE_CFATA)
>>> +#define HD_CFA_OK (HD_OK | CFA_OK)
>>> +#define ALL_OK (HD_OK | CD_OK | CFA_OK)
>>> +
>>> +/* See ACS-2 T13/2015-D Table B.2 Command codes */
>>> +uint8_t ide_cmd_table[0x100] = {
>> 
>> Missing 'static'.
>
> And const, while we're at it.

Yes.

>>> +    /* NOP not implemented, mandatory for CD */
>>> +    [CFA_REQ_EXT_ERROR_CODE]            = CFA_OK,
>>> +    [WIN_DSM]                           = ALL_OK,
>>> +    [WIN_DEVICE_RESET]                  = CD_OK,
>>> +    [WIN_RECAL]                         = ALL_OK,
>>> +    [WIN_READ]                          = ALL_OK,
>>> +    [WIN_READ_ONCE]                     = ALL_OK,
>>> +    [WIN_READ_EXT]                      = ALL_OK,
>>> +    [WIN_READDMA_EXT]                   = ALL_OK,
>>> +    [WIN_READ_NATIVE_MAX_EXT]           = ALL_OK,
>>> +    [WIN_MULTREAD_EXT]                  = ALL_OK,
>>> +    [WIN_WRITE]                         = ALL_OK,
>>> +    [WIN_WRITE_ONCE]                    = ALL_OK,
>>> +    [WIN_WRITE_EXT]                     = ALL_OK,
>>> +    [WIN_WRITEDMA_EXT]                  = ALL_OK,
>>> +    [CFA_WRITE_SECT_WO_ERASE]           = ALL_OK,
>>> +    [WIN_MULTWRITE_EXT]                 = ALL_OK,
>>> +    [WIN_WRITE_VERIFY]                  = ALL_OK,
>>> +    [WIN_VERIFY]                        = ALL_OK,
>>> +    [WIN_VERIFY_ONCE]                   = ALL_OK,
>>> +    [WIN_VERIFY_EXT]                    = ALL_OK,
>>> +    [WIN_SEEK]                          = HD_CFA_OK,
>>> +    [CFA_TRANSLATE_SECTOR]              = CFA_OK,
>>> +    [WIN_DIAGNOSE]                      = ALL_OK,
>>> +    [WIN_SPECIFY]                       = ALL_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]                      = ALL_OK,
>>> +    [WIN_MULTWRITE]                     = ALL_OK,
>>> +    [WIN_SETMULT]                       = ALL_OK,
>>> +    [WIN_READDMA]                       = ALL_OK,
>>> +    [WIN_READDMA_ONCE]                  = ALL_OK,
>>> +    [WIN_WRITEDMA]                      = ALL_OK,
>>> +    [WIN_WRITEDMA_ONCE]                 = ALL_OK,
>>> +    [CFA_WRITE_MULTI_WO_ERASE]          = ALL_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]               = ALL_OK,
>>> +    [WIN_IDENTIFY]                      = ALL_OK,
>>> +    [WIN_SETFEATURES]                   = ALL_OK,
>>> +    [IBM_SENSE_CONDITION]               = CFA_OK,
>>> +    [CFA_WEAR_LEVEL]                    = CFA_OK,
>>> +    [WIN_READ_NATIVE_MAX]               = ALL_OK,
>>> +};
>>> +
>>> +static bool ide_cmd_permitted(IDEState *s, uint32_t cmd)
>>> +{
>>> +    return cmd <= 0xff && (ide_cmd_table[cmd] & (1u << s->drive_kind));
>>> +}
>
> Doesn't cmd < ARRAY_SIZE(ide_cmd_table) better describe what you want?

Since I touch this patch anyway, I'll do this your way.

Patch

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 1c4dc2f..a25c175 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -876,6 +876,77 @@  void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
     }
 }
 
+#define HD_OK (1u << IDE_HD)
+#define CD_OK (1u << IDE_CD)
+#define CFA_OK (1u << IDE_CFATA)
+#define HD_CFA_OK (HD_OK | CFA_OK)
+#define ALL_OK (HD_OK | CD_OK | CFA_OK)
+
+/* See ACS-2 T13/2015-D Table B.2 Command codes */
+uint8_t 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]                         = ALL_OK,
+    [WIN_READ]                          = ALL_OK,
+    [WIN_READ_ONCE]                     = ALL_OK,
+    [WIN_READ_EXT]                      = ALL_OK,
+    [WIN_READDMA_EXT]                   = ALL_OK,
+    [WIN_READ_NATIVE_MAX_EXT]           = ALL_OK,
+    [WIN_MULTREAD_EXT]                  = ALL_OK,
+    [WIN_WRITE]                         = ALL_OK,
+    [WIN_WRITE_ONCE]                    = ALL_OK,
+    [WIN_WRITE_EXT]                     = ALL_OK,
+    [WIN_WRITEDMA_EXT]                  = ALL_OK,
+    [CFA_WRITE_SECT_WO_ERASE]           = ALL_OK,
+    [WIN_MULTWRITE_EXT]                 = ALL_OK,
+    [WIN_WRITE_VERIFY]                  = ALL_OK,
+    [WIN_VERIFY]                        = ALL_OK,
+    [WIN_VERIFY_ONCE]                   = ALL_OK,
+    [WIN_VERIFY_EXT]                    = ALL_OK,
+    [WIN_SEEK]                          = HD_CFA_OK,
+    [CFA_TRANSLATE_SECTOR]              = CFA_OK,
+    [WIN_DIAGNOSE]                      = ALL_OK,
+    [WIN_SPECIFY]                       = ALL_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]                      = ALL_OK,
+    [WIN_MULTWRITE]                     = ALL_OK,
+    [WIN_SETMULT]                       = ALL_OK,
+    [WIN_READDMA]                       = ALL_OK,
+    [WIN_READDMA_ONCE]                  = ALL_OK,
+    [WIN_WRITEDMA]                      = ALL_OK,
+    [WIN_WRITEDMA_ONCE]                 = ALL_OK,
+    [CFA_WRITE_MULTI_WO_ERASE]          = ALL_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]               = ALL_OK,
+    [WIN_IDENTIFY]                      = ALL_OK,
+    [WIN_SETFEATURES]                   = ALL_OK,
+    [IBM_SENSE_CONDITION]               = CFA_OK,
+    [CFA_WEAR_LEVEL]                    = CFA_OK,
+    [WIN_READ_NATIVE_MAX]               = ALL_OK,
+};
+
+static bool ide_cmd_permitted(IDEState *s, uint32_t cmd)
+{
+    return cmd <= 0xff && (ide_cmd_table[cmd] & (1u << s->drive_kind));
+}
 
 void ide_exec_cmd(IDEBus *bus, uint32_t val)
 {
@@ -895,6 +966,10 @@  void ide_exec_cmd(IDEBus *bus, uint32_t val)
     if ((s->status & (BUSY_STAT|DRQ_STAT)) && val != WIN_DEVICE_RESET)
         return;
 
+    if (!ide_cmd_permitted(s, val)) {
+        goto abort_cmd;
+    }
+
     switch(val) {
     case WIN_DSM:
         switch (s->feature) {
@@ -1115,21 +1190,15 @@  void ide_exec_cmd(IDEBus *bus, uint32_t val)
         ide_set_irq(s->bus);
         break;
     case WIN_SEEK:
-        if(s->drive_kind == IDE_CD)
-            goto abort_cmd;
         /* XXX: Check that seek is within bounds */
         s->status = READY_STAT | SEEK_STAT;
         ide_set_irq(s->bus);
         break;
         /* ATAPI commands */
     case WIN_PIDENTIFY:
-        if (s->drive_kind == IDE_CD) {
-            ide_atapi_identify(s);
-            s->status = READY_STAT | SEEK_STAT;
-            ide_transfer_start(s, s->io_buffer, 512, ide_transfer_stop);
-        } else {
-            ide_abort_command(s);
-        }
+        ide_atapi_identify(s);
+        s->status = READY_STAT | SEEK_STAT;
+        ide_transfer_start(s, s->io_buffer, 512, ide_transfer_stop);
         ide_set_irq(s->bus);
         break;
     case WIN_DIAGNOSE:
@@ -1146,15 +1215,11 @@  void ide_exec_cmd(IDEBus *bus, uint32_t val)
         ide_set_irq(s->bus);
         break;
     case WIN_DEVICE_RESET:
-        if (s->drive_kind != IDE_CD)
-            goto abort_cmd;
         ide_set_signature(s);
         s->status = 0x00; /* NOTE: READY is _not_ set */
         s->error = 0x01;
         break;
     case WIN_PACKETCMD:
-        if (s->drive_kind != IDE_CD)
-            goto abort_cmd;
         /* overlapping commands not supported */
         if (s->feature & 0x02)
             goto abort_cmd;
@@ -1166,16 +1231,12 @@  void ide_exec_cmd(IDEBus *bus, uint32_t val)
         break;
     /* CF-ATA commands */
     case CFA_REQ_EXT_ERROR_CODE:
-        if (s->drive_kind != IDE_CFATA)
-            goto abort_cmd;
         s->error = 0x09;    /* miscellaneous error */
         s->status = READY_STAT | SEEK_STAT;
         ide_set_irq(s->bus);
         break;
     case CFA_ERASE_SECTORS:
     case CFA_WEAR_LEVEL:
-        if (s->drive_kind != IDE_CFATA)
-            goto abort_cmd;
         if (val == CFA_WEAR_LEVEL)
             s->nsector = 0;
         if (val == CFA_ERASE_SECTORS)
@@ -1185,8 +1246,6 @@  void ide_exec_cmd(IDEBus *bus, uint32_t val)
         ide_set_irq(s->bus);
         break;
     case CFA_TRANSLATE_SECTOR:
-        if (s->drive_kind != IDE_CFATA)
-            goto abort_cmd;
         s->error = 0x00;
         s->status = READY_STAT | SEEK_STAT;
         memset(s->io_buffer, 0, 0x200);
@@ -1205,8 +1264,6 @@  void ide_exec_cmd(IDEBus *bus, uint32_t val)
         ide_set_irq(s->bus);
         break;
     case CFA_ACCESS_METADATA_STORAGE:
-        if (s->drive_kind != IDE_CFATA)
-            goto abort_cmd;
         switch (s->feature) {
         case 0x02:	/* Inquiry Metadata Storage */
             ide_cfata_metadata_inquiry(s);
@@ -1225,8 +1282,6 @@  void ide_exec_cmd(IDEBus *bus, uint32_t val)
         ide_set_irq(s->bus);
         break;
     case IBM_SENSE_CONDITION:
-        if (s->drive_kind != IDE_CFATA)
-            goto abort_cmd;
         switch (s->feature) {
         case 0x01:  /* sense temperature in device */
             s->nsector = 0x50;      /* +20 C */
@@ -1239,8 +1294,6 @@  void ide_exec_cmd(IDEBus *bus, uint32_t val)
         break;
 
     case WIN_SMART:
-	if (s->drive_kind == IDE_CD)
-		goto abort_cmd;
 	if (s->hcyl != 0xc2 || s->lcyl != 0x4f)
 		goto abort_cmd;
 	if (!s->smart_enabled && s->feature != SMART_ENABLE)
@@ -1395,6 +1448,7 @@  void ide_exec_cmd(IDEBus *bus, uint32_t val)
 	}
 	break;
     default:
+        /* should not be reachable */
     abort_cmd:
         ide_abort_command(s);
         ide_set_irq(s->bus);