Re: [Qemu-devel] [PATCH v2 07/45] ide: Use a table to declare which drive kinds accept each command

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 03.08.2011 18:53, schrieb Blue Swirl:
>> On Wed, Aug 3, 2011 at 1:07 PM, Markus Armbruster <address@hidden> 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 <address@hidden>
>>> ---
>>>  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.