Re: [XEN PATCH v5 4/4] xen/x86: address violations of MISRA C:2012 Rule 7.2

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.08.2023 13:03, Simone Ballarin wrote:
> --- a/xen/arch/x86/hvm/stdvga.c
> +++ b/xen/arch/x86/hvm/stdvga.c
> @@ -39,34 +39,35 @@
>  
>  #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)~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 */
>  };

I'm sorry to say this quite bluntly, but this is what absolutely should
not happen when doing supposedly mechanical changes: Initially I was
merely puzzled by the comments that are appearing here all of the
sudden, but then I noticed that values also change. (This also
definitely invalidates Stefano's R-b; quite likely it shouldn't have
been kept in the first place.)

May I remind you of something that was said earlier on: As soon as a
change is controversial, it's likely better to split out. And the
changes to this file were previously commented upon. This is even more
so that by its mere size this patch likely would better have been split
at some reasonable boundaries (hvm, include [there msr-index.h would
probably have been good to deal with all on its own], lib, and the rest
would come to mind). That way you also wouldn't need to carry (and
repeatedly re-submit) such large a chunk of work, because parts likely
would have gone in already.

> --- 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              _AC(0x00000001, U) /* Enable Protected Mode  
>   (RW) */
> +#define X86_CR0_MP              _AC(0x00000002, U) /* Monitor Coprocessor    
>   (RW) */
> +#define X86_CR0_EM              _AC(0x00000004, U) /* Require FPU Emulation  
>   (RO) */
> +#define X86_CR0_TS              _AC(0x00000008, U) /* Task Switched          
>   (RW) */
> +#define X86_CR0_ET              _AC(0x00000010, U) /* Extension type         
>   (RO) */
> +#define X86_CR0_NE              _AC(0x00000020, U) /* Numeric Error 
> Reporting  (RW) */
> +#define X86_CR0_WP              _AC(0x00010000, U) /* Supervisor Write 
> Protect (RW) */
> +#define X86_CR0_AM              _AC(0x00040000, U) /* Alignment Checking     
>   (RW) */
> +#define X86_CR0_NW              _AC(0x20000000, U) /* Not Write-Through      
>   (RW) */
> +#define X86_CR0_CD              _AC(0x40000000, U) /* Cache Disable          
>   (RW) */
> +#define X86_CR0_PG              _AC(0x80000000, U) /* Paging                 
>   (RW) */

CR0 being a 64-bit register, I consider this risky. Imo either UL needs
to be used as suffix, or the change needs limiting to just PG (and even
then perhaps better using UL).

Jan