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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.07.2023 12:32, Simone Ballarin wrote:
> @@ -378,10 +378,10 @@ 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,
> -                                          ARRAY_SIZE(p->extd.raw) - 1);
> +    p->extd.max_leaf = 0x80000000U | min_t(uint32_t, max_extd_leaf & 0xffffU,
> +                                           ARRAY_SIZE(p->extd.raw) - 1);

Why the 2nd of the U additions? I ask in particular because ...

> @@ -768,11 +768,11 @@ 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->x86_vendor & (X86_VENDOR_AMD 
> |
> -                                                              
> X86_VENDOR_HYGON))
> -                                            ? CPUID_GUEST_NR_EXTD_AMD
> -                                            : CPUID_GUEST_NR_EXTD_INTEL) - 
> 1);
> +    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
> +                                             : CPUID_GUEST_NR_EXTD_INTEL) - 
> 1);

... you don't do the same here (or else you would have to further
switch to e.g. using 1u, to please min()'s type checking).

Just to mention it (I think that U wants dropping for now), in the
earlier case if you switched to UL, you/we would be able to switch
from min_t() to the type-safe min().

> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -383,7 +383,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, 
> uint32_t data)
>  
>      if ( !vector )
>      {
> -        int pirq = ((addr >> 32) & 0xffffff00) | dest;
> +        int pirq = ((addr >> 32) & 0xffffff00U) | dest;

This is fishy, not just because of the use of plain int, but even more
so ...

>          if ( pirq > 0 )
>          {

... with this following. It leaves unclear what negative values would
mean. I think pirq wants to be unsigned int (pirq_info() expects it
this way, albeit far from all of its invocations adhere to this rule),
but the situation isn't helped by XEN_DMOP_inject_msi not having any
comment whatsoever on the meaning of the upper half of the uint64_t
addr field that's being passed in.

I'm inclined to omit this hunk for now. I'll look around some more,
and if I can come to a sensible conclusion I may then submit a patch
just for this.

> --- a/xen/arch/x86/hvm/stdvga.c
> +++ b/xen/arch/x86/hvm/stdvga.c
> @@ -39,22 +39,22 @@
>  
>  #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 */
> @@ -70,15 +70,15 @@ static const uint8_t sr_mask[8] = {
>  };
>  
>  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)~0xf0,
> +    (uint8_t)~0xf0,
> +    (uint8_t)~0xf0,
> +    (uint8_t)~0xe0,
> +    (uint8_t)~0xfc,
> +    (uint8_t)~0x84,
> +    (uint8_t)~0xf0,
> +    (uint8_t)~0xf0,
> +    (uint8_t)~0x00,
>  };

The changelog for v3 says you did drop the changes to this array.

> --- 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)

Perhaps again better to switch to a uint32_t cast on the lhs of the comma.

> @@ -93,7 +93,7 @@
>      HVMTRACE_ND(evt, 0, 0)
>  
>  #define HVMTRACE_LONG_1D(evt, d1)                  \
> -                   HVMTRACE_2D(evt ## 64, (d1) & 0xFFFFFFFF, (d1) >> 32)
> +                   HVMTRACE_2D(evt ## 64, (d1) & 0xFFFFFFFFU, (d1) >> 32)

Same here then.

>  /* K6 MSRs */
> -#define MSR_K6_EFER                  0xc0000080
> -#define MSR_K6_STAR                  0xc0000081
> -#define MSR_K6_WHCR                  0xc0000082
> -#define MSR_K6_UWCCR                 0xc0000085
> -#define MSR_K6_EPMR                  0xc0000086
> -#define MSR_K6_PSOR                  0xc0000087
> -#define MSR_K6_PFIR                  0xc0000088
> +#define MSR_K6_EFER                  0xc0000080U
> +#define MSR_K6_STAR                  0xc0000081U
> +#define MSR_K6_WHCR                  0xc0000082U
> +#define MSR_K6_UWCCR                 0xc0000085U
> +#define MSR_K6_EPMR                  0xc0000086U
> +#define MSR_K6_PSOR                  0xc0000087U
> +#define MSR_K6_PFIR                  0xc0000088U

This entire block can be dropped rather than adjusted; there are no uses of
these constants. I expect they're a remnant of 32-bit Xen, which has been
gone for a decade.

> @@ -459,10 +459,10 @@
>  #define MSR_VIA_BCR2                 0x00001147
>  
>  /* Transmeta defined MSRs */
> -#define MSR_TMTA_LONGRUN_CTRL                0x80868010
> -#define MSR_TMTA_LONGRUN_FLAGS               0x80868011
> -#define MSR_TMTA_LRTI_READOUT                0x80868018
> -#define MSR_TMTA_LRTI_VOLT_MHZ               0x8086801a
> +#define MSR_TMTA_LONGRUN_CTRL                0x80868010U
> +#define MSR_TMTA_LONGRUN_FLAGS               0x80868011U
> +#define MSR_TMTA_LRTI_READOUT                0x80868018U
> +#define MSR_TMTA_LRTI_VOLT_MHZ               0x8086801aU

Same here.

> --- a/xen/arch/x86/x86_64/acpi_mmcfg.c
> +++ b/xen/arch/x86/x86_64/acpi_mmcfg.c
> @@ -50,7 +50,7 @@ static int __init acpi_mcfg_check_entry(struct 
> acpi_table_mcfg *mcfg,
>  {
>      int year;
>  
> -    if (cfg->address < 0xFFFFFFFF)
> +    if (cfg->address < 0xFFFFFFFFU)
>          return 0;

This check is bogus and would better be corrected. Such a correction
would presumably deal with the Misra violation at the same time.

Jan