Re: [RFC PATCH 4/5] hw/i386/q35: Wire virtual SMI# lines to ICH9 chipset

[Thomas Huth <thuth@redhat.com>]

On 08/03/2024 09.08, Philippe Mathieu-Daudé wrote:
> On 7/3/24 20:43, Thomas Huth wrote:
>> On 28/02/2024 17.43, Zhao Liu wrote:
>>> Hi Philippe,
>>>
>>>> +/*
>>>> + * Real ICH9 contains a single SMI output line and doesn't broadcast CPUs.
>>>> + * Virtualized ICH9 allows broadcasting upon negatiation with guest, see
>>>> + * commit 5ce45c7a2b.
>>>> + */
>>>> +enum {
>>>> +    ICH9_VIRT_SMI_BROADCAST,
>>>> +    ICH9_VIRT_SMI_CURRENT,
>>>> +#define ICH9_VIRT_SMI_COUNT 2
>>>> +};
>>>> +
>>>
>>> Just quick look here. Shouldn't ICH9_VIRT_SMI_COUNT be defined outside of
>>> enum {}?
>>
>> Or even better, do it without a #define:
>>
>> enum {
>>      ICH9_VIRT_SMI_BROADCAST,
>>      ICH9_VIRT_SMI_CURRENT,
>>      ICH9_VIRT_SMI_COUNT
> 
> This form isn't recommended as it confuses static analyzers,
> considering ICH9_VIRT_SMI_COUNT as part of the enum.

Never heard of that before. We're using it all over the place, e.g.:

typedef enum {
     THROTTLE_BPS_TOTAL,
     THROTTLE_BPS_READ,
     THROTTLE_BPS_WRITE,
     THROTTLE_OPS_TOTAL,
     THROTTLE_OPS_READ,
     THROTTLE_OPS_WRITE,
     BUCKETS_COUNT,
} BucketType;

... and even in our generated QAPI code, e.g.:

typedef enum QCryptoHashAlgorithm {
     QCRYPTO_HASH_ALG_MD5,
     QCRYPTO_HASH_ALG_SHA1,
     QCRYPTO_HASH_ALG_SHA224,
     QCRYPTO_HASH_ALG_SHA256,
     QCRYPTO_HASH_ALG_SHA384,
     QCRYPTO_HASH_ALG_SHA512,
     QCRYPTO_HASH_ALG_RIPEMD160,
     QCRYPTO_HASH_ALG__MAX,
} QCryptoHashAlgorithm;

Where did you see here a problem with static analyzers?

  Thomas