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

[Simone Ballarin <simone.ballarin@xxxxxxxxxxx>]

Il giorno mar 20 giu 2023 alle ore 14:51 Jan Beulich <jbeulich@xxxxxxxx> ha scritto:

On 20.06.2023 12:34, Simone Ballarin wrote:
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -950,10 +950,10 @@ const struct arch_vpmu_ops *__init core2_vpmu_init(void)
>         fixed_ctrl_mask |=
>             (FIXED_CTR_CTRL_ANYTHREAD_MASK << (FIXED_CTR_CTRL_BITS * i));
> 
> -    fixed_counters_mask = ~((1ull << core2_get_bitwidth_fix_count()) - 1);
> +    fixed_counters_mask = ~((1Ull << core2_get_bitwidth_fix_count()) - 1);

What's the point of this adjustment? And if at all, why keep the l-s
lowercase?

In the patch for Rule 7.3 I will propose to change all 'l' with 'L'.

I will move this type of change to the new patch for 7.3.

 

>      global_ctrl_mask = ~((((1ULL << fixed_pmc_cnt) - 1) << 32) |
>                           ((1ULL << arch_pmc_cnt) - 1));
> -    global_ovf_ctrl_mask = ~(0xC000000000000000 |
> +    global_ovf_ctrl_mask = ~(0xC000000000000000U |

If such constants gain a suffix, I think that ought to be UL or ULL.

Yes, I agree with using 'ULL'.

 

> -#define INTR_INFO_VECTOR_MASK           0xff            /* 7:0 */
> -#define INTR_INFO_INTR_TYPE_MASK        0x700           /* 10:8 */
> -#define INTR_INFO_DELIVER_CODE_MASK     0x800           /* 11 */
> -#define INTR_INFO_NMI_UNBLOCKED_BY_IRET 0x1000          /* 12 */
> -#define INTR_INFO_VALID_MASK            0x80000000      /* 31 */
> -#define INTR_INFO_RESVD_BITS_MASK       0x7ffff000
> +#define INTR_INFO_VECTOR_MASK           0xffU            /* 7:0 */
> +#define INTR_INFO_INTR_TYPE_MASK        0x700U           /* 10:8 */
> +#define INTR_INFO_DELIVER_CODE_MASK     0x800U           /* 11 */
> +#define INTR_INFO_NMI_UNBLOCKED_BY_IRET 0x1000           /* 12 */
> +#define INTR_INFO_VALID_MASK            0x80000000U      /* 31 */
> +#define INTR_INFO_RESVD_BITS_MASK       0x7ffff000U

I think it would be nice if you took the opportunity and added
zero-padding to these constants.

Ok, I can do that.

 

>  #define X86_SEG_AR_SEG_TYPE     0xf        /* 3:0, segment type */
> -#define X86_SEG_AR_DESC_TYPE    (1u << 4)  /* 4, descriptor type */
> +#define X86_SEG_AR_DESC_TYPE    (1U << 4)  /* 4, descriptor type */
>  #define X86_SEG_AR_DPL          0x60       /* 6:5, descriptor privilege level */
> -#define X86_SEG_AR_SEG_PRESENT  (1u << 7)  /* 7, segment present */
> -#define X86_SEG_AR_AVL          (1u << 12) /* 12, available for system software */
> -#define X86_SEG_AR_CS_LM_ACTIVE (1u << 13) /* 13, long mode active (CS only) */
> -#define X86_SEG_AR_DEF_OP_SIZE  (1u << 14) /* 14, default operation size */
> -#define X86_SEG_AR_GRANULARITY  (1u << 15) /* 15, granularity */
> -#define X86_SEG_AR_SEG_UNUSABLE (1u << 16) /* 16, segment unusable */
> +#define X86_SEG_AR_SEG_PRESENT  (1U << 7)  /* 7, segment present */
> +#define X86_SEG_AR_AVL          (1U << 12) /* 12, available for system software */
> +#define X86_SEG_AR_CS_LM_ACTIVE (1U << 13) /* 13, long mode active (CS only) */
> +#define X86_SEG_AR_DEF_OP_SIZE  (1U << 14) /* 14, default operation size */
> +#define X86_SEG_AR_GRANULARITY  (1U << 15) /* 15, granularity */
> +#define X86_SEG_AR_SEG_UNUSABLE (1U << 16) /* 16, segment unusable */

And all of these, when it's exactly the two numbers you don't touch
which might want to gain a U (or u) suffix?

Ok.

Jan

--

Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)