Il giorno mar 20 giu 2023 alle ore 14:51 Jan Beulich <jbeul...@suse.com> 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 <http://bugseng.com>)
