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

[Thomas Huth]

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