Re: [XEN PATCH 12/13] xen/x86: fixed violations of MISRA C:2012 Rule 7.2

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.06.2023 12:35, Simone Ballarin wrote:
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -1211,7 +1211,7 @@ static void __init calibrate_APIC_clock(void)
>       * Setup the APIC counter to maximum. There is no way the lapic
>       * can underflow in the 100ms detection time frame.
>       */
> -    __setup_APIC_LVTT(0xffffffff);
> +    __setup_APIC_LVTT(0xffffffffU);

In cases like this one I wonder whether we wouldn't better switch to ~0.

> --- a/xen/arch/x86/hvm/stdvga.c
> +++ b/xen/arch/x86/hvm/stdvga.c
> @@ -39,46 +39,46 @@
>  
>  #define PAT(x) (x)
>  static const uint32_t mask16[16] = {
> -    PAT(0x00000000),
> -    PAT(0x000000ff),
> -    PAT(0x0000ff00),
> -    PAT(0x0000ffff),
> -    PAT(0x00ff0000),
> -    PAT(0x00ff00ff),
> -    PAT(0x00ffff00),
> -    PAT(0x00ffffff),
> -    PAT(0xff000000),
> -    PAT(0xff0000ff),
> -    PAT(0xff00ff00),
> -    PAT(0xff00ffff),
> -    PAT(0xffff0000),
> -    PAT(0xffff00ff),
> -    PAT(0xffffff00),
> -    PAT(0xffffffff),
> +    PAT(0x00000000U),
> +    PAT(0x000000ffU),
> +    PAT(0x0000ff00U),
> +    PAT(0x0000ffffU),
> +    PAT(0x00ff0000U),
> +    PAT(0x00ff00ffU),
> +    PAT(0x00ffff00U),
> +    PAT(0x00ffffffU),
> +    PAT(0xff000000U),
> +    PAT(0xff0000ffU),
> +    PAT(0xff00ff00U),
> +    PAT(0xff00ffffU),
> +    PAT(0xffff0000U),
> +    PAT(0xffff00ffU),
> +    PAT(0xffffff00U),
> +    PAT(0xffffffffU),
>  };
>  
>  /* force some bits to zero */
>  static const uint8_t sr_mask[8] = {
> -    (uint8_t)~0xfc,
> -    (uint8_t)~0xc2,
> -    (uint8_t)~0xf0,
> -    (uint8_t)~0xc0,
> -    (uint8_t)~0xf1,
> -    (uint8_t)~0xff,
> -    (uint8_t)~0xff,
> -    (uint8_t)~0x00,
> +    (uint8_t)~0xfcU,
> +    (uint8_t)~0xc2U,
> +    (uint8_t)~0xf0U,
> +    (uint8_t)~0xc0U,
> +    (uint8_t)~0xf1U,
> +    (uint8_t)~0xffU,
> +    (uint8_t)~0xffU,
> +    (uint8_t)~0x00U,

What supposed effect have the changes to the initializers of this
array?

>  };
>  
>  static const uint8_t gr_mask[9] = {
> -    (uint8_t)~0xf0, /* 0x00 */
> -    (uint8_t)~0xf0, /* 0x01 */
> -    (uint8_t)~0xf0, /* 0x02 */
> -    (uint8_t)~0xe0, /* 0x03 */
> -    (uint8_t)~0xfc, /* 0x04 */
> -    (uint8_t)~0x84, /* 0x05 */
> -    (uint8_t)~0xf0, /* 0x06 */
> -    (uint8_t)~0xf0, /* 0x07 */
> -    (uint8_t)~0x00, /* 0x08 */
> +    (uint8_t)~0xf0U, /* 0x00 */
> +    (uint8_t)~0xf0U, /* 0x01 */
> +    (uint8_t)~0xf0U, /* 0x02 */
> +    (uint8_t)~0xe0U, /* 0x03 */
> +    (uint8_t)~0xfcU, /* 0x04 */
> +    (uint8_t)~0x84U, /* 0x05 */
> +    (uint8_t)~0xf0U, /* 0x06 */
> +    (uint8_t)~0xf0U, /* 0x07 */
> +    (uint8_t)~0x00U, /* 0x08 */

Same question here. In this latter case, if the array was to be touched,
I'd wonder if this wasn't a good opportunity to drop the not really
helpful (but cluttering) comments.

> --- a/xen/arch/x86/include/asm/guest/hyperv-tlfs.h
> +++ b/xen/arch/x86/include/asm/guest/hyperv-tlfs.h
> @@ -471,30 +471,30 @@ typedef struct _HV_REFERENCE_TSC_PAGE {
>  
>  /* Define hypervisor message types. */
>  enum hv_message_type {
> -     HVMSG_NONE                      = 0x00000000,
> +     HVMSG_NONE                      = 0x00000000U,
>  
>       /* Memory access messages. */
> -     HVMSG_UNMAPPED_GPA              = 0x80000000,
> -     HVMSG_GPA_INTERCEPT             = 0x80000001,
> +     HVMSG_UNMAPPED_GPA              = 0x80000000U,
> +     HVMSG_GPA_INTERCEPT             = 0x80000001U,
>  
>       /* Timer notification messages. */
> -     HVMSG_TIMER_EXPIRED                     = 0x80000010,
> +     HVMSG_TIMER_EXPIRED                     = 0x80000010U,
>  
>       /* Error messages. */
> -     HVMSG_INVALID_VP_REGISTER_VALUE = 0x80000020,
> -     HVMSG_UNRECOVERABLE_EXCEPTION   = 0x80000021,
> -     HVMSG_UNSUPPORTED_FEATURE               = 0x80000022,
> +     HVMSG_INVALID_VP_REGISTER_VALUE = 0x80000020U,
> +     HVMSG_UNRECOVERABLE_EXCEPTION   = 0x80000021U,
> +     HVMSG_UNSUPPORTED_FEATURE               = 0x80000022U,
>  
>       /* Trace buffer complete messages. */
> -     HVMSG_EVENTLOG_BUFFERCOMPLETE   = 0x80000040,
> +     HVMSG_EVENTLOG_BUFFERCOMPLETE   = 0x80000040U,
>  
>       /* Platform-specific processor intercept messages. */
> -     HVMSG_X64_IOPORT_INTERCEPT              = 0x80010000,
> -     HVMSG_X64_MSR_INTERCEPT         = 0x80010001,
> -     HVMSG_X64_CPUID_INTERCEPT               = 0x80010002,
> -     HVMSG_X64_EXCEPTION_INTERCEPT   = 0x80010003,
> -     HVMSG_X64_APIC_EOI                      = 0x80010004,
> -     HVMSG_X64_LEGACY_FP_ERROR               = 0x80010005
> +     HVMSG_X64_IOPORT_INTERCEPT              = 0x80010000U,
> +     HVMSG_X64_MSR_INTERCEPT         = 0x80010001U,
> +     HVMSG_X64_CPUID_INTERCEPT               = 0x80010002U,
> +     HVMSG_X64_EXCEPTION_INTERCEPT   = 0x80010003U,
> +     HVMSG_X64_APIC_EOI                      = 0x80010004U,
> +     HVMSG_X64_LEGACY_FP_ERROR               = 0x80010005U

Iirc this file tries follow the spec to the word. And wasn't this enum
already flagged for another violation? If so, as indicated before already,
dealing with all violations at once would seem desirable.

> --- a/xen/arch/x86/include/asm/hvm/trace.h
> +++ b/xen/arch/x86/include/asm/hvm/trace.h
> @@ -58,7 +58,7 @@
>  #define DO_TRC_HVM_VLAPIC           DEFAULT_HVM_MISC
>  
>  
> -#define TRC_PAR_LONG(par) ((par)&0xFFFFFFFF),((par)>>32)
> +#define TRC_PAR_LONG(par) ((par)&0xFFFFFFFFU),((par)>>32)

Please can you correct style in such cases as you go?

> --- a/xen/arch/x86/include/asm/x86-defns.h
> +++ b/xen/arch/x86/include/asm/x86-defns.h
> @@ -30,17 +30,17 @@
>  /*
>   * Intel CPU flags in CR0
>   */
> -#define X86_CR0_PE              0x00000001 /* Enable Protected Mode    (RW) 
> */
> -#define X86_CR0_MP              0x00000002 /* Monitor Coprocessor      (RW) 
> */
> -#define X86_CR0_EM              0x00000004 /* Require FPU Emulation    (RO) 
> */
> -#define X86_CR0_TS              0x00000008 /* Task Switched            (RW) 
> */
> -#define X86_CR0_ET              0x00000010 /* Extension type           (RO) 
> */
> -#define X86_CR0_NE              0x00000020 /* Numeric Error Reporting  (RW) 
> */
> -#define X86_CR0_WP              0x00010000 /* Supervisor Write Protect (RW) 
> */
> -#define X86_CR0_AM              0x00040000 /* Alignment Checking       (RW) 
> */
> -#define X86_CR0_NW              0x20000000 /* Not Write-Through        (RW) 
> */
> -#define X86_CR0_CD              0x40000000 /* Cache Disable            (RW) 
> */
> -#define X86_CR0_PG              0x80000000 /* Paging                   (RW) 
> */
> +#define X86_CR0_PE              0x00000001U  /* Enable Protected Mode    
> (RW) */
> +#define X86_CR0_MP              0x00000002U  /* Monitor Coprocessor      
> (RW) */
> +#define X86_CR0_EM              0x00000004U  /* Require FPU Emulation    
> (RO) */
> +#define X86_CR0_TS              0x00000008U  /* Task Switched            
> (RW) */
> +#define X86_CR0_ET              0x00000010U  /* Extension type           
> (RO) */
> +#define X86_CR0_NE              0x00000020U  /* Numeric Error Reporting  
> (RW) */
> +#define X86_CR0_WP              0x00010000U  /* Supervisor Write Protect 
> (RW) */
> +#define X86_CR0_AM              0x00040000U  /* Alignment Checking       
> (RW) */
> +#define X86_CR0_NW              0x20000000U  /* Not Write-Through        
> (RW) */
> +#define X86_CR0_CD              0x40000000U  /* Cache Disable            
> (RW) */
> +#define X86_CR0_PG              0x80000000U  /* Paging                   
> (RW) */
>  
>  /*
>   * Intel CPU flags in CR3
> @@ -103,7 +103,7 @@
>  /*
>   * Debug status flags in DR6.
>   */
> -#define X86_DR6_DEFAULT         0xffff0ff0  /* Default %dr6 value. */
> +#define X86_DR6_DEFAULT         0xffff0ff0U  /* Default %dr6 value. */

All of these may want to use UL, as the described registers are 64-bit
on x86-64, and 32-bit only on ix86. (There may be downsides to going
that route though, so in the end I'm not really sure.)

Jan