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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.07.2023 17:26, 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);

While making the change less mechanical, we want to consider to switch
to ~0 in this and similar cases.

> @@ -378,9 +378,9 @@ static void __init calculate_host_policy(void)
>       * this information.
>       */
>      if ( cpu_has_lfence_dispatch )
> -        max_extd_leaf = max(max_extd_leaf, 0x80000021);
> +        max_extd_leaf = max(max_extd_leaf, 0x80000021U);
>  
> -    p->extd.max_leaf = 0x80000000 | min_t(uint32_t, max_extd_leaf & 0xffff,
> +    p->extd.max_leaf = 0x80000000U | min_t(uint32_t, max_extd_leaf & 0xffffU,
>                                            ARRAY_SIZE(p->extd.raw) - 1);

Adjustments like this or ...

> @@ -768,7 +768,7 @@ void recalculate_cpuid_policy(struct domain *d)
>  
>      p->basic.max_leaf   = min(p->basic.max_leaf,   max->basic.max_leaf);
>      p->feat.max_subleaf = min(p->feat.max_subleaf, max->feat.max_subleaf);
> -    p->extd.max_leaf    = 0x80000000 | min(p->extd.max_leaf & 0xffff,
> +    p->extd.max_leaf    = 0x80000000U | min(p->extd.max_leaf & 0xffff,
>                                             ((p->x86_vendor & (X86_VENDOR_AMD 
> |
>                                                                
> X86_VENDOR_HYGON))
>                                              ? CPUID_GUEST_NR_EXTD_AMD

... this also need to adjust indentation of the following lines.

> --- a/xen/arch/x86/cpu/mcheck/mce-apei.c
> +++ b/xen/arch/x86/cpu/mcheck/mce-apei.c
> @@ -37,11 +37,11 @@
>  #include "mce.h"
>  
>  #define CPER_CREATOR_MCE                                             \
> -     UUID_LE(0x75a574e3, 0x5052, 0x4b29, 0x8a, 0x8e, 0xbe, 0x2c,     \
> -             0x64, 0x90, 0xb8, 0x9d)
> +     UUID_LE(0x75a574e3U, 0x5052U, 0x4b29U, 0x8aU, 0x8eU, 0xbeU, 0x2cU,      
> \
> +             0x64U, 0x90U, 0xb8U, 0x9dU)
>  #define CPER_SECTION_TYPE_MCE                                                
> \
> -     UUID_LE(0xfe08ffbe, 0x95e4, 0x4be7, 0xbc, 0x73, 0x40, 0x96,     \
> -             0x04, 0x4a, 0x38, 0xfc)
> +     UUID_LE(0xfe08ffbeU, 0x95e4U, 0x4be7U, 0xbcU, 0x73U, 0x40U, 0x96U,      
> \
> +             0x04U, 0x4aU, 0x38U, 0xfcU)

See the earlier (EFI) comment regarding excessive use of suffixes here.

> --- 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),
>  };

While I agree here, ...

>  /* 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,
>  };
>  
>  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,
> +    (uint8_t)~0xf0U,
> +    (uint8_t)~0xf0U,
> +    (uint8_t)~0xe0U,
> +    (uint8_t)~0xfcU,
> +    (uint8_t)~0x84U,
> +    (uint8_t)~0xf0U,
> +    (uint8_t)~0xf0U,
> +    (uint8_t)~0x00U,
>  };

I continue to question these changes. They don't fix anything, do they?

> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -291,7 +291,7 @@ static void enable_hypercall_page(struct domain *d)
>       * calling convention) to differentiate Xen and Viridian hypercalls.
>       */
>      *(u8  *)(p + 0) = 0x0d; /* orl $0x80000000, %eax */
> -    *(u32 *)(p + 1) = 0x80000000;
> +    *(u32 *)(p + 1) = 0x80000000U;
>      *(u8  *)(p + 5) = 0x0f; /* vmcall/vmmcall */
>      *(u8  *)(p + 6) = 0x01;
>      *(u8  *)(p + 7) = (cpu_has_vmx ? 0xc1 : 0xd9);

Please can this and ...

> --- 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
>  };

... this together be made a separate Viridian-specific change? (I
continue to be uncertain about touching the header file; the
Viridian maintainers will need to judge.)

> --- a/xen/arch/x86/include/asm/x86-defns.h
> +++ b/xen/arch/x86/include/asm/x86-defns.h
> @@ -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. */

Considering the respective register is pointer-/long-size, wouldn't
this better use UL? But of course we'd want to check that this then
doesn't affect code in do_debug() in an undesirable way.

Jan