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

[Bernhard Beschow]

Am 8. März 2024 08:10:24 UTC schrieb Laszlo Ersek <lersek@redhat.com>:
>On 3/8/24 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.
>
>Side comment: I didn't know about this (so thanks for the info), but
>that's really a shame for those static analyzers. It's an ancient and
>valid pattern. :/

Another pattern would be:

    enum {
        ICH9_VIRT_SMI_BROADCAST,
        ICH9_VIRT_SMI_CURRENT,
        ICH9_VIRT_SMI_LAST = ICH9_VIRT_SMI_CURRENT
    };

which should also work with GCC's `-Wswitch-enum`.

Best regards,
Bernhard

>
>> 
>>> };
>>>
>>>   Thomas
>>>
>> 
>