Re: [XEN PATCH v4 4/4] xen/x86: address violations of MISRA C:2012 Rule 7.2
On 26.07.2023 13:03, Simone Ballarin wrote:
> @@ -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,
> };
Hmm, this stray change is _still_ there.
> --- 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)&0x),((par)>>32)
> +#define TRC_PAR_LONG(par) ((uint32_t)par), ((par) >> 32)
You've lost parentheses around "par".
> @@ -93,7 +93,7 @@
> HVMTRACE_ND(evt, 0, 0)
>
> #define HVMTRACE_LONG_1D(evt, d1) \
> - HVMTRACE_2D(evt ## 64, (d1) & 0x, (d1) >> 32)
> + HVMTRACE_2D(evt ## 64, (uint32_t)d1, (d1) >> 32)
Same for "d1" here.
Both of these are, btw., indications that you should have dropped Stefano's
R-b.
> --- a/xen/arch/x86/include/asm/msr-index.h
> +++ b/xen/arch/x86/include/asm/msr-index.h
> @@ -30,7 +30,7 @@
>
> #define MSR_INTEL_CORE_THREAD_COUNT 0x0035
> #define MSR_CTC_THREAD_MASK0x
> -#define MSR_CTC_CORE_MASK 0x
> +#define MSR_CTC_CORE_MASK 0xU
>
> #define MSR_SPEC_CTRL 0x0048
> #define SPEC_CTRL_IBRS (_AC(1, ULL) << 0)
> @@ -168,7 +168,7 @@
> #define MSR_UARCH_MISC_CTRL 0x1b01
> #define UARCH_CTRL_DOITM (_AC(1, ULL) << 0)
>
> -#define MSR_EFER0xc080 /* Extended Feature
> Enable Register */
> +#define MSR_EFER_AC(0xc080, U) /* Extended
> Feature Enable Register */
I understand you use _AC() here because the constant is used in assembly
code. But I don't understand why you use it only here, and not throughout
at least the "modern" portion of the file. I wonder what other x86
maintainers think about this.
As a minor aspect, I also don't really see why you insert a 2nd blank
ahead of the comment.
> #define EFER_SCE (_AC(1, ULL) << 0) /* SYSCALL
> Enable */
> #define EFER_LME (_AC(1, ULL) << 8) /* Long Mode
> Enable */
> #define EFER_LMA (_AC(1, ULL) << 10) /* Long Mode
> Active */
> @@ -181,35 +181,35 @@
> (EFER_SCE | EFER_LME | EFER_LMA | EFER_NXE | EFER_SVME | EFER_FFXSE | \
> EFER_AIBRSE)
>
> -#define MSR_STAR0xc081 /* legacy mode
> SYSCALL target */
> -#define MSR_LSTAR 0xc082 /* long mode SYSCALL
> target */
> -#define MSR_CSTAR 0xc083 /* compat mode
> SYSCALL target */
> -#define MSR_SYSCALL_MASK0xc084 /* EFLAGS mask for
> syscall */
> -#define MSR_FS_BASE 0xc100 /* 64bit FS base */
> -#define MSR_GS_BASE 0xc101 /* 64bit GS base */
> -#define MSR_SHADOW_GS_BASE 0xc102 /* SwapGS GS shadow */
> -#define MSR_TSC_AUX 0xc103 /* Auxiliary TSC */
> +#define MSR_STAR0xc081U /* legacy mode
> SYSCALL target */
> +#define MSR_LSTAR 0xc082U /* long mode SYSCALL
> target */
> +#define MSR_CSTAR 0xc083U /* compat mode
> SYSCALL target */
> +#define MSR_SYSCALL_MASK0xc084U /* EFLAGS mask for
> syscall */
> +#define MSR_FS_BASE 0xc100U /* 64bit FS base */
> +#define MSR_GS_BASE 0xc101U /* 64bit GS base */
> +#define MSR_SHADOW_GS_BASE 0xc102U /* SwapGS GS shadow
> */
> +#define MSR_TSC_AUX 0xc103U /* Auxiliary TSC */
>
> -#define MSR_K8_SYSCFG 0xc0010010
> +#define MSR_K8_SYSCFG 0xc0010010U
> #define SYSCFG_MTRR_FIX_DRAM_EN(_AC(1, ULL) << 18)
> #define SYSCFG_MTRR_FIX_DRAM_MOD_EN(_AC(1, ULL) << 19)
> #define SYSCFG_MTRR_VAR_DRAM_EN(_AC(1, ULL) << 20)
> #define SYSCFG_MTRR_TOM2_EN(_AC(1, ULL) << 21)
> #define SYSCFG_TOM2_FORCE_WB (_AC(1, ULL) << 22)
>
> -#define MSR_K8_IORR_BASE0 0xc0010016
> -#define MSR_K8_IORR_MASK0 0xc0010017
> -#define MSR_K8_IORR_BASE1 0xc00
[XEN PATCH v4 4/4] xen/x86: address violations of MISRA C:2012 Rule 7.2
From: Gianluca Luparini The xen sources contains violations of MISRA C:2012 Rule 7.2 whose headline states: "A 'u' or 'U' suffix shall be applied to all integer constants that are represented in an unsigned type". Add the 'U' suffix to integers literals with unsigned type. Use _AC() for macro costants that are used also in assembly files. For the sake of uniformity, the following changes are made: - add the 'U' suffix to all first macro's arguments in 'mce-apei.c' - add the 'U' suffix to switch cases in 'cpuid.c' - add 'U' suffixes to 'mask16' in 'stdvga.c' - add the 'U' suffix to macros in 'pci.h' - use _AC() for macros near 'X86_CR0_PG' Signed-off-by: Gianluca Luparini Signed-off-by: Simone Ballarin Reviewed-by: Stefano Stabellini --- Changes in v4: - change commit headline - remove 'U' suffix from '0x' in 'cpu-policy.c' - remove some changes in 'msr-index.h' - remove changes in 'irq.c' and 'acpi_mmcfg.c' Changes in v3: - change 'Signed-off-by' ordering - change commit message - add 'UL' in 'extable.c' - fix indentation in 'cpu-policy.c' - remove excessive suffixes in 'mce-apei.c' - add 'UL' in 'x86-defns.h' - remove changes to 'sr_mask' in 'stdvga.c' - remove comments to 'gr_mask' in 'stdvga.c' - move 'viridian.c' and 'hyperv-tlfs.h' in a separate commit Changes in v2: - minor change to commit title - change commit message - remove comments from 'gr_mask' in 'stdvga.c' - correct code style in 'trace.h' - add fix in 'extable.c' - remove changes in 'x86-defns.h', 'msr-index.h' and 'xen-x86_64.h' --- xen/arch/x86/apic.c| 2 +- xen/arch/x86/cpu-policy.c | 18 +-- xen/arch/x86/cpu/mcheck/mce-apei.c | 4 +- xen/arch/x86/cpuid.c | 8 +- xen/arch/x86/efi/efi-boot.h| 6 +- xen/arch/x86/extable.c | 2 +- xen/arch/x86/hvm/hypercall.c | 2 +- xen/arch/x86/hvm/pmtimer.c | 4 +- xen/arch/x86/hvm/stdvga.c | 50 +++ xen/arch/x86/hvm/vlapic.c | 6 +- xen/arch/x86/include/asm/apicdef.h | 2 +- xen/arch/x86/include/asm/config.h | 2 +- xen/arch/x86/include/asm/hpet.h| 2 +- xen/arch/x86/include/asm/hvm/trace.h | 4 +- xen/arch/x86/include/asm/hvm/vioapic.h | 2 +- xen/arch/x86/include/asm/msi.h | 2 +- xen/arch/x86/include/asm/msr-index.h | 180 - xen/arch/x86/include/asm/pci.h | 8 +- xen/arch/x86/include/asm/x86-defns.h | 24 ++-- xen/arch/x86/percpu.c | 2 +- xen/arch/x86/psr.c | 2 +- xen/arch/x86/spec_ctrl.c | 8 +- xen/arch/x86/x86_64/pci.c | 2 +- xen/arch/x86/x86_emulate/x86_emulate.h | 2 +- xen/lib/x86/cpuid.c| 8 +- xen/lib/x86/policy.c | 2 +- 26 files changed, 177 insertions(+), 177 deletions(-) diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c index 41879230ec..1109c0d9cf 100644 --- 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(0x); +__setup_APIC_LVTT(0xU); bus_freq = calibrate_apic_timer(); if ( !bus_freq ) diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c index f40eeb8be8..b0c7e3bd02 100644 --- a/xen/arch/x86/cpu-policy.c +++ b/xen/arch/x86/cpu-policy.c @@ -321,7 +321,7 @@ static void recalculate_misc(struct cpu_policy *p) p->extd.vendor_edx = p->basic.vendor_edx; p->extd.raw_fms = p->basic.raw_fms; -p->extd.raw[0x1].b &= 0xff00; +p->extd.raw[0x1].b &= 0xff00U; p->extd.e1d |= p->basic._1d & CPUID_COMMON_1D_FEATURES; p->extd.raw[0x8].a &= 0x; /* GuestMaxPhysAddr hidden. */ @@ -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, 0x8021); +max_extd_leaf = max(max_extd_leaf, 0x8021U); -p->extd.max_leaf = 0x8000 | min_t(uint32_t, max_extd_leaf & 0x, - ARRAY_SIZE(p->extd.raw) - 1); +p->extd.max_leaf = 0x8000U | min_t(uint32_t, max_extd_leaf & 0x, + ARRAY_SIZE(p->extd.raw) - 1); x86_cpu_featureset_to_policy(boot_cpu_data.x86_capability, p); recalculate_xstate(p); @@ -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= 0x8000 | min(p->extd.max_leaf & 0x, - ((p->x86_vendor & (X86_VENDOR_AMD | -
