On 15.08.2025 22:41, Andrew Cooper wrote:
> We want a consistent MSR API, and these want to be named rdmsr() and wrmsr(),
> but not with their current APIs. The current rdmsr() flavours writing to
> their parameters by name makes code that reads like invalid C, and is
> unergonomic to use in lots of cases.
>
> Change the API, and update the callers all in one go. Where appropriate,
> update the write side to wrmsrns() as per the recommendation.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <[email protected]>
> ---
> CC: Jan Beulich <[email protected]>
> CC: Roger Pau Monné <[email protected]>
>
> I do have a more creative solution if this patch is considered to be too
> large.
> https://gitlab.com/xen-project/hardware/xen-staging/-/commit/e13cf25d06d08481e2c138daa1fd902cf36d757b
I'm not concerned by the size of this patch.
> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -23,17 +23,17 @@ static uint32_t __ro_after_init mcu_opt_ctrl_val;
>
> void update_mcu_opt_ctrl(void)
> {
> - uint32_t mask = mcu_opt_ctrl_mask, lo, hi;
> + uint32_t mask = mcu_opt_ctrl_mask, val;
>
> if ( !mask )
> return;
>
> - rdmsr(MSR_MCU_OPT_CTRL, lo, hi);
> + val = rdmsr(MSR_MCU_OPT_CTRL);
>
> - lo &= ~mask;
> - lo |= mcu_opt_ctrl_val;
> + val &= ~mask;
> + val |= mcu_opt_ctrl_val;
>
> - wrmsr(MSR_MCU_OPT_CTRL, lo, hi);
> + wrmsrns(MSR_MCU_OPT_CTRL, val);
> }
I don't consider it a good idea to suddenly clear the upper half of this
MSR, and ...
> @@ -51,17 +51,17 @@ static uint32_t __ro_after_init pb_opt_ctrl_val;
>
> void update_pb_opt_ctrl(void)
> {
> - uint32_t mask = pb_opt_ctrl_mask, lo, hi;
> + uint32_t mask = pb_opt_ctrl_mask, val;
>
> if ( !mask )
> return;
>
> - rdmsr(MSR_PB_OPT_CTRL, lo, hi);
> + val = rdmsr(MSR_PB_OPT_CTRL);
>
> - lo &= ~mask;
> - lo |= pb_opt_ctrl_val;
> + val &= ~mask;
> + val |= pb_opt_ctrl_val;
>
> - wrmsr(MSR_PB_OPT_CTRL, lo, hi);
> + wrmsrns(MSR_PB_OPT_CTRL, val);
> }
... this one.
> @@ -456,15 +456,15 @@ static void __init probe_mwait_errata(void)
> */
> static void Intel_errata_workarounds(struct cpuinfo_x86 *c)
> {
> - unsigned long lo, hi;
> + uint64_t val;
>
> if ((c->x86 == 15) && (c->x86_model == 1) && (c->x86_mask == 1)) {
> - rdmsr (MSR_IA32_MISC_ENABLE, lo, hi);
> - if ((lo & (1<<9)) == 0) {
> + val = rdmsr(MSR_IA32_MISC_ENABLE);
> + if ((val & (1 << 9)) == 0) {
> printk (KERN_INFO "CPU: C0 stepping P4 Xeon
> detected.\n");
> printk (KERN_INFO "CPU: Disabling hardware prefetching
> (Errata 037)\n");
> - lo |= (1<<9); /* Disable hw prefetching */
> - wrmsr (MSR_IA32_MISC_ENABLE, lo, hi);
> + val |= (1 << 9); /* Disable hw prefetching */
> + wrmsrns(MSR_IA32_MISC_ENABLE, val);
> }
> }
Move val into the more narrow scope at the same time?
> @@ -699,7 +715,7 @@ void cf_check vmx_cpu_dead(unsigned int cpu)
>
> static int _vmx_cpu_up(bool bsp)
> {
> - u32 eax, edx;
> + u32 eax;
Like you do elsewhere, switch to uint32_t at the same time?
> --- a/xen/arch/x86/tsx.c
> +++ b/xen/arch/x86/tsx.c
> @@ -42,6 +42,8 @@ void tsx_init(void)
> {
> static bool __read_mostly once;
>
> + uint64_t val;
> +
> /*
No real need for yet another newline, I would say.
Jan
