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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 8/3/24 17:06, Thomas Huth wrote:
> 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;

We tried to remove it:

https://lore.kernel.org/qemu-devel/20230315112811.22355-4-philmd@linaro.org/

But there is a problem with generated empty enums...
https://lore.kernel.org/qemu-devel/87sfdx9w58.fsf@pond.sub.org/