On 27 Jul 2023, Jan Beulich wrote:
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.
>
> Ok. Sorry for that.
> >* --- 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) ((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) & 0xFFFFFFFF, (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.
>
> Ok.
> >* --- 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 0x00000035*
> >* #define MSR_CTC_THREAD_MASK 0x0000ffff*
> >* -#define MSR_CTC_CORE_MASK 0xffff0000*
> >* +#define MSR_CTC_CORE_MASK 0xffff0000U*
> >
> >* #define MSR_SPEC_CTRL 0x00000048*
> >* #define SPEC_CTRL_IBRS (_AC(1, ULL) << 0)*
> >* @@ -168,7 +168,7 @@*
> >* #define MSR_UARCH_MISC_CTRL 0x00001b01*
> >* #define UARCH_CTRL_DOITM (_AC(1, ULL) << 0)*
> >
> >* -#define MSR_EFER 0xc0000080 /* Extended
> >Feature *
> >* Enable Register */*
> >* +#define MSR_EFER _AC(0xc0000080, 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.
>
> I've used _AC only when strictly required to avoid errors.
In v5 I will use _AC on all constants in the "modern" part of the file.
> As a minor aspect, I also don't really see why you insert a 2nd blank
> ahead of the comment.
>
> To align the comment with the others below.
I see that comment aligning is not done in this file, so I will drop the
change in v5.
> >* #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_STAR 0xc0000081 /* legacy mode *
> >* SYSCALL target */*
> >* -#define MSR_LSTAR 0xc0000082 /* long mode
> >SYSCALL *
> >* target */*
> >* -#define MSR_CSTAR 0xc0000083 /* compat mode *
> >* SYSCALL target */*
> >* -#define MSR_SYSCALL_MASK 0xc0000084 /* EFLAGS mask for
> >*
> >* syscall */*
> >* -#define MSR_FS_BASE 0xc0000100 /* 64bit FS base
> >*/*
> >* -#define MSR_GS_BASE 0xc0000101 /* 64bit GS base
> >*/*
> >* -#define MSR_SHADOW_GS_BASE 0xc0000102 /* SwapGS GS
> >shadow */*
> >* -#define MSR_TSC_AUX 0xc0000103 /* Auxiliary TSC
> >*/*
> >* +#define MSR_STAR 0xc0000081U /* legacy mode *
> >* SYSCALL target */*
> >* +#define MSR_LSTAR 0xc0000082U /* long mode
> >SYSCALL *
> >* target */*
> >* +#define MSR_CSTAR 0xc0000083U /* compat mode *
> >* SYSCALL target */*
> >* +#define MSR_SYSCALL_MASK 0xc0000084U /* EFLAGS mask
> >for *
> >* syscall */*
> >* +#define MSR_FS_BASE 0xc0000100U /* 64bit FS base
> >*/*
> >* +#define MSR_GS_BASE 0xc0000101U /* 64bit GS base
> >*/*
> >* +#define MSR_SHADOW_GS_BASE 0xc0000102U /* SwapGS GS
> >shadow *
> >* */*
> >* +#define MSR_TSC_AUX 0xc0000103U /* 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 0xc0010018*
> >* -#define MSR_K8_IORR_MASK1 0xc0010019*
> >* +#define MSR_K8_IORR_BASE0 0xc0010016U*
> >* +#define MSR_K8_IORR_MASK0 0xc0010017U*
> >* +#define MSR_K8_IORR_BASE1 0xc0010018U*
> >* +#define MSR_K8_IORR_MASK1 0xc0010019U*
> >
> >* -#define MSR_K8_TSEG_BASE 0xc0010112 /* AMD doc:
> >SMMAddr */*
> >* -#define MSR_K8_TSEG_MASK 0xc0010113 /* AMD doc:
> >SMMMask */*
> >* +#define MSR_K8_TSEG_BASE 0xc0010112U /* AMD doc:
> >SMMAddr *
> >* */*
> >* +#define MSR_K8_TSEG_MASK 0xc0010113U /* AMD doc:
> >SMMMask *
> >* */*
> >
> >* -#define MSR_K8_VM_CR 0xc0010114*
> >* +#define MSR_K8_VM_CR 0xc0010114U*
> >* #define VM_CR_INIT_REDIRECTION (_AC(1, ULL) << 1)*
> >* #define VM_CR_SVM_DISABLE (_AC(1, ULL) << 4)*
> >
> >* -#define MSR_VIRT_SPEC_CTRL 0xc001011f /* Layout matches *
> >* MSR_SPEC_CTRL */*
> >* +#define MSR_VIRT_SPEC_CTRL 0xc001011fU /* Layout matches
> >*
> >* MSR_SPEC_CTRL */*
> >
> >* /**
> >* * Legacy MSR constants in need of cleanup. No new MSRs below this
> >comment.*
>
> (As to above remark: This is the separator between "modern" [above]
> and "historic" [below].)
>
> Jan
>
>
--
Simone Ballarin, M.Sc.
Field Application Engineer, BUGSENG (https://bugseng.com
<http://bugseng.com>)
