__wrmsr() is the lowest level primitive MSR write API, and its direct
use is NOT preferred. Use its wrapper function native_wrmsrl() instead.
No functional change intended.
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---
arch/x86/events/amd/brs.c | 2 +-
arch/x86/include/asm/apic.h | 2 +-
arch/x86/include/asm/msr.h | 6 ++++--
arch/x86/kernel/cpu/mce/core.c | 2 +-
arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 6 +++---
5 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/arch/x86/events/amd/brs.c b/arch/x86/events/amd/brs.c
index ec3427463382..4a47f3c108de 100644
--- a/arch/x86/events/amd/brs.c
+++ b/arch/x86/events/amd/brs.c
@@ -44,7 +44,7 @@ static inline unsigned int brs_to(int idx)
static __always_inline void set_debug_extn_cfg(u64 val)
{
/* bits[4:3] must always be set to 11b */
- __wrmsr(MSR_AMD_DBG_EXTN_CFG, val | 3ULL << 3, val >> 32);
+ native_wrmsrl(MSR_AMD_DBG_EXTN_CFG, val | 3ULL << 3);
}
static __always_inline u64 get_debug_extn_cfg(void)
diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index c903d358405d..3345a819c859 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -214,7 +214,7 @@ static inline void native_apic_msr_write(u32 reg, u32 v)
static inline void native_apic_msr_eoi(void)
{
- __wrmsr(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0);
+ native_wrmsrl(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK);
}
static inline u32 native_apic_msr_read(u32 reg)
diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 9397a319d165..27ea8793705d 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -144,10 +144,12 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
static inline void notrace
native_write_msr(unsigned int msr, u32 low, u32 high)
{
- __wrmsr(msr, low, high);
+ u64 val = (u64)high << 32 | low;
+
+ native_wrmsrl(msr, val);
if (tracepoint_enabled(write_msr))
- do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
+ do_trace_write_msr(msr, val, 0);
}
/* Can be uninlined because referenced by paravirt */
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 1f14c3308b6b..0eaeaba12df2 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1306,7 +1306,7 @@ static noinstr bool mce_check_crashing_cpu(void)
}
if (mcgstatus & MCG_STATUS_RIPV) {
- __wrmsr(MSR_IA32_MCG_STATUS, 0, 0);
+ native_wrmsrl(MSR_IA32_MCG_STATUS, 0);
return true;
}
}
diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index 01fa7890b43f..55536120c8d1 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -481,7 +481,7 @@ int resctrl_arch_pseudo_lock_fn(void *_plr)
* cache.
*/
saved_msr = __rdmsr(MSR_MISC_FEATURE_CONTROL);
- __wrmsr(MSR_MISC_FEATURE_CONTROL, prefetch_disable_bits, 0x0);
+ native_wrmsrl(MSR_MISC_FEATURE_CONTROL, prefetch_disable_bits);
closid_p = this_cpu_read(pqr_state.cur_closid);
rmid_p = this_cpu_read(pqr_state.cur_rmid);
mem_r = plr->kmem;
@@ -493,7 +493,7 @@ int resctrl_arch_pseudo_lock_fn(void *_plr)
* pseudo-locked followed by reading of kernel memory to load it
* into the cache.
*/
- __wrmsr(MSR_IA32_PQR_ASSOC, rmid_p, plr->closid);
+ native_wrmsrl(MSR_IA32_PQR_ASSOC, (u64)plr->closid << 32 | rmid_p);
/*
* Cache was flushed earlier. Now access kernel memory to read it
@@ -530,7 +530,7 @@ int resctrl_arch_pseudo_lock_fn(void *_plr)
* Critical section end: restore closid with capacity bitmask that
* does not overlap with pseudo-locked region.
*/
- __wrmsr(MSR_IA32_PQR_ASSOC, rmid_p, closid_p);
+ native_wrmsrl(MSR_IA32_PQR_ASSOC, (u64)closid_p << 32 | rmid_p);
/* Re-enable the hardware prefetcher(s) */
wrmsrl(MSR_MISC_FEATURE_CONTROL, saved_msr);
--
2.49.0On 31/03/2025 9:22 am, Xin Li (Intel) wrote: > __wrmsr() is the lowest level primitive MSR write API, and its direct > use is NOT preferred. Use its wrapper function native_wrmsrl() instead. > > No functional change intended. > > Signed-off-by: Xin Li (Intel) <xin@zytor.com> The critical piece of information you're missing from the commit message is that the MSR_IMM instructions take a single u64. Therefore to use them, you've got to arrange for all callers to provide a single u64, rather than a split u32 pair. ~Andrew
On 3/31/2025 2:45 PM, Andrew Cooper wrote:
> On 31/03/2025 9:22 am, Xin Li (Intel) wrote:
>> __wrmsr() is the lowest level primitive MSR write API, and its direct
>> use is NOT preferred. Use its wrapper function native_wrmsrl() instead.
>>
>> No functional change intended.
>>
>> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
>
> The critical piece of information you're missing from the commit message
> is that the MSR_IMM instructions take a single u64.
>
> Therefore to use them, you've got to arrange for all callers to provide
> a single u64, rather than a split u32 pair.
You definitely caught me on how I was thinking it ;)
Sometimes it is nice to see a change log with a thinking process.
Thanks!
Xin
On March 31, 2025 2:45:43 PM PDT, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >On 31/03/2025 9:22 am, Xin Li (Intel) wrote: >> __wrmsr() is the lowest level primitive MSR write API, and its direct >> use is NOT preferred. Use its wrapper function native_wrmsrl() instead. >> >> No functional change intended. >> >> Signed-off-by: Xin Li (Intel) <xin@zytor.com> > >The critical piece of information you're missing from the commit message >is that the MSR_IMM instructions take a single u64. > >Therefore to use them, you've got to arrange for all callers to provide >a single u64, rather than a split u32 pair. > >~Andrew That being said, there is nothing wrong with having a two-word convenience wrapper.
On 3/31/2025 10:13 PM, H. Peter Anvin wrote: > On March 31, 2025 2:45:43 PM PDT, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> On 31/03/2025 9:22 am, Xin Li (Intel) wrote: >>> __wrmsr() is the lowest level primitive MSR write API, and its direct >>> use is NOT preferred. Use its wrapper function native_wrmsrl() instead. >>> >>> No functional change intended. >>> >>> Signed-off-by: Xin Li (Intel) <xin@zytor.com> >> >> The critical piece of information you're missing from the commit message >> is that the MSR_IMM instructions take a single u64. >> >> Therefore to use them, you've got to arrange for all callers to provide >> a single u64, rather than a split u32 pair. >> >> ~Andrew > > That being said, there is nothing wrong with having a two-word convenience wrapper. > Yes, I ended up keeping the two-word convenience wrapper in this patch set, and the wrapper calls a lower level API that takes a u64 argument. And yes, as Ingo said, some of the conversion is NOT an improvement.
* Xin Li (Intel) <xin@zytor.com> wrote: > - __wrmsr (MSR_AMD_DBG_EXTN_CFG, val | 3ULL << 3, val >> 32); > + native_wrmsrl(MSR_AMD_DBG_EXTN_CFG, val | 3ULL << 3); This is an improvement. > - __wrmsr (MSR_IA32_PQR_ASSOC, rmid_p, plr->closid); > + native_wrmsrl(MSR_IA32_PQR_ASSOC, (u64)plr->closid << 32 | rmid_p); > - __wrmsr (MSR_IA32_PQR_ASSOC, rmid_p, closid_p); > + native_wrmsrl(MSR_IA32_PQR_ASSOC, (u64)closid_p << 32 | rmid_p); This is not an improvement. Please provide a native_wrmsrl() API variant where natural [rmid_p, closid_p] high/lo parameters can be used, without the shift-uglification... Thanks, Ingo
On March 31, 2025 3:17:30 AM PDT, Ingo Molnar <mingo@kernel.org> wrote: > >* Xin Li (Intel) <xin@zytor.com> wrote: > >> - __wrmsr (MSR_AMD_DBG_EXTN_CFG, val | 3ULL << 3, val >> 32); >> + native_wrmsrl(MSR_AMD_DBG_EXTN_CFG, val | 3ULL << 3); > >This is an improvement. > >> - __wrmsr (MSR_IA32_PQR_ASSOC, rmid_p, plr->closid); >> + native_wrmsrl(MSR_IA32_PQR_ASSOC, (u64)plr->closid << 32 | rmid_p); > >> - __wrmsr (MSR_IA32_PQR_ASSOC, rmid_p, closid_p); >> + native_wrmsrl(MSR_IA32_PQR_ASSOC, (u64)closid_p << 32 | rmid_p); > >This is not an improvement. > >Please provide a native_wrmsrl() API variant where natural [rmid_p, closid_p] >high/lo parameters can be used, without the shift-uglification... > >Thanks, > > Ingo Directing this question primarily to Ingo, who is more than anyone else the namespace consistency guardian: On the subject of msr function naming ... *msrl() has always been misleading. The -l suffix usually means 32 bits; sometimes it means the C type "long" (which in the kernel is used instead of size_t/uintptr_t, which might end up being "fun" when 128-bit architectures appear some time this century), but for a fixed 64-but type we normally use -q. Should we rename the *msrl() functions to *msrq() as part of this overhaul?
* H. Peter Anvin <hpa@zytor.com> wrote:
> On March 31, 2025 3:17:30 AM PDT, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >* Xin Li (Intel) <xin@zytor.com> wrote:
> >
> >> - __wrmsr (MSR_AMD_DBG_EXTN_CFG, val | 3ULL << 3, val >> 32);
> >> + native_wrmsrl(MSR_AMD_DBG_EXTN_CFG, val | 3ULL << 3);
> >
> >This is an improvement.
> >
> >> - __wrmsr (MSR_IA32_PQR_ASSOC, rmid_p, plr->closid);
> >> + native_wrmsrl(MSR_IA32_PQR_ASSOC, (u64)plr->closid << 32 | rmid_p);
> >
> >> - __wrmsr (MSR_IA32_PQR_ASSOC, rmid_p, closid_p);
> >> + native_wrmsrl(MSR_IA32_PQR_ASSOC, (u64)closid_p << 32 | rmid_p);
> >
> >This is not an improvement.
> >
> >Please provide a native_wrmsrl() API variant where natural [rmid_p, closid_p]
> >high/lo parameters can be used, without the shift-uglification...
> >
> >Thanks,
> >
> > Ingo
>
> Directing this question primarily to Ingo, who is more than anyone
> else the namespace consistency guardian:
>
> On the subject of msr function naming ... *msrl() has always been
> misleading. The -l suffix usually means 32 bits; sometimes it means
> the C type "long" (which in the kernel is used instead of
> size_t/uintptr_t, which might end up being "fun" when 128-bit
> architectures appear some time this century), but for a fixed 64-but
> type we normally use -q.
Yeah, agreed - that's been bothering me for a while too. :-)
> Should we rename the *msrl() functions to *msrq() as part of this
> overhaul?
Yeah, that's a good idea, and because talk is cheap I just implemented
this in the tip:WIP.x86/msr branch with a couple of other cleanups in
this area (see the shortlog & diffstat below), but the churn is high:
144 files changed, 1034 insertions(+), 1034 deletions(-)
So this can only be done if regenerated and sent to Linus right before
an -rc1 I think:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip WIP.x86/msr
Thanks,
Ingo
=======================>
Ingo Molnar (18):
x86/msr: Standardize on u64 in <asm/msr.h>
x86/msr: Standardize on u64 in <asm/msr-index.h>
x86/msr: Use u64 in rdmsrl_amd_safe() and wrmsrl_amd_safe()
x86/msr: Use u64 in rdmsrl_safe() and paravirt_read_pmc()
x86/msr: Rename 'rdmsrl()' to 'rdmsrq()'
x86/msr: Rename 'wrmsrl()' to 'wrmsrq()'
x86/msr: Rename 'rdmsrl_safe()' to 'rdmsrq_safe()'
x86/msr: Rename 'wrmsrl_safe()' to 'wrmsrq_safe()'
x86/msr: Rename 'rdmsrl_safe_on_cpu()' to 'rdmsrq_safe_on_cpu()'
x86/msr: Rename 'wrmsrl_safe_on_cpu()' to 'wrmsrq_safe_on_cpu()'
x86/msr: Rename 'rdmsrl_on_cpu()' to 'rdmsrq_on_cpu()'
x86/msr: Rename 'wrmsrl_on_cpu()' to 'wrmsrq_on_cpu()'
x86/msr: Rename 'mce_rdmsrl()' to 'mce_rdmsrq()'
x86/msr: Rename 'mce_wrmsrl()' to 'mce_wrmsrq()'
x86/msr: Rename 'rdmsrl_amd_safe()' to 'rdmsrq_amd_safe()'
x86/msr: Rename 'wrmsrl_amd_safe()' to 'wrmsrq_amd_safe()'
x86/msr: Rename 'native_wrmsrl()' to 'native_wrmsrq()'
x86/msr: Rename 'wrmsrl_cstar()' to 'wrmsrq_cstar()'
arch/x86/coco/sev/core.c | 2 +-
arch/x86/events/amd/brs.c | 8 +-
arch/x86/events/amd/core.c | 12 +--
arch/x86/events/amd/ibs.c | 26 ++---
arch/x86/events/amd/lbr.c | 20 ++--
arch/x86/events/amd/power.c | 10 +-
arch/x86/events/amd/uncore.c | 12 +--
arch/x86/events/core.c | 42 ++++----
arch/x86/events/intel/core.c | 66 ++++++-------
arch/x86/events/intel/cstate.c | 2 +-
arch/x86/events/intel/ds.c | 10 +-
arch/x86/events/intel/knc.c | 16 +--
arch/x86/events/intel/lbr.c | 44 ++++-----
arch/x86/events/intel/p4.c | 24 ++---
arch/x86/events/intel/p6.c | 12 +--
arch/x86/events/intel/pt.c | 32 +++---
arch/x86/events/intel/uncore.c | 2 +-
arch/x86/events/intel/uncore_discovery.c | 10 +-
arch/x86/events/intel/uncore_nhmex.c | 70 ++++++-------
arch/x86/events/intel/uncore_snb.c | 42 ++++----
arch/x86/events/intel/uncore_snbep.c | 50 +++++-----
arch/x86/events/msr.c | 2 +-
arch/x86/events/perf_event.h | 26 ++---
arch/x86/events/probe.c | 2 +-
arch/x86/events/rapl.c | 8 +-
arch/x86/events/zhaoxin/core.c | 16 +--
arch/x86/hyperv/hv_apic.c | 4 +-
arch/x86/hyperv/hv_init.c | 66 ++++++-------
arch/x86/hyperv/hv_spinlock.c | 6 +-
arch/x86/hyperv/ivm.c | 2 +-
arch/x86/include/asm/apic.h | 8 +-
arch/x86/include/asm/debugreg.h | 4 +-
arch/x86/include/asm/fsgsbase.h | 4 +-
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/include/asm/microcode.h | 2 +-
arch/x86/include/asm/msr-index.h | 12 +--
arch/x86/include/asm/msr.h | 50 +++++-----
arch/x86/include/asm/paravirt.h | 8 +-
arch/x86/include/asm/spec-ctrl.h | 2 +-
arch/x86/kernel/acpi/cppc.c | 8 +-
arch/x86/kernel/amd_nb.c | 2 +-
arch/x86/kernel/apic/apic.c | 16 +--
arch/x86/kernel/apic/apic_numachip.c | 6 +-
arch/x86/kernel/cet.c | 2 +-
arch/x86/kernel/cpu/amd.c | 28 +++---
arch/x86/kernel/cpu/aperfmperf.c | 28 +++---
arch/x86/kernel/cpu/bugs.c | 24 ++---
arch/x86/kernel/cpu/bus_lock.c | 18 ++--
arch/x86/kernel/cpu/common.c | 68 ++++++-------
arch/x86/kernel/cpu/feat_ctl.c | 4 +-
arch/x86/kernel/cpu/hygon.c | 6 +-
arch/x86/kernel/cpu/intel.c | 10 +-
arch/x86/kernel/cpu/intel_epb.c | 12 +--
arch/x86/kernel/cpu/mce/amd.c | 22 ++---
arch/x86/kernel/cpu/mce/core.c | 58 +++++------
arch/x86/kernel/cpu/mce/inject.c | 32 +++---
arch/x86/kernel/cpu/mce/intel.c | 32 +++---
arch/x86/kernel/cpu/mce/internal.h | 2 +-
arch/x86/kernel/cpu/microcode/amd.c | 2 +-
arch/x86/kernel/cpu/microcode/intel.c | 2 +-
arch/x86/kernel/cpu/mshyperv.c | 12 +--
arch/x86/kernel/cpu/resctrl/core.c | 10 +-
arch/x86/kernel/cpu/resctrl/monitor.c | 2 +-
arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 2 +-
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 6 +-
arch/x86/kernel/cpu/sgx/main.c | 2 +-
arch/x86/kernel/cpu/topology.c | 2 +-
arch/x86/kernel/cpu/topology_amd.c | 4 +-
arch/x86/kernel/cpu/tsx.c | 20 ++--
arch/x86/kernel/cpu/umwait.c | 2 +-
arch/x86/kernel/fpu/core.c | 2 +-
arch/x86/kernel/fpu/xstate.c | 10 +-
arch/x86/kernel/fpu/xstate.h | 2 +-
arch/x86/kernel/fred.c | 20 ++--
arch/x86/kernel/hpet.c | 2 +-
arch/x86/kernel/kvm.c | 28 +++---
arch/x86/kernel/kvmclock.c | 4 +-
arch/x86/kernel/mmconf-fam10h_64.c | 8 +-
arch/x86/kernel/process.c | 16 +--
arch/x86/kernel/process_64.c | 20 ++--
arch/x86/kernel/reboot_fixups_32.c | 2 +-
arch/x86/kernel/shstk.c | 18 ++--
arch/x86/kernel/traps.c | 10 +-
arch/x86/kernel/tsc.c | 2 +-
arch/x86/kernel/tsc_sync.c | 14 +--
arch/x86/kvm/svm/avic.c | 2 +-
arch/x86/kvm/svm/sev.c | 2 +-
arch/x86/kvm/svm/svm.c | 16 +--
arch/x86/kvm/vmx/nested.c | 4 +-
arch/x86/kvm/vmx/pmu_intel.c | 4 +-
arch/x86/kvm/vmx/sgx.c | 8 +-
arch/x86/kvm/vmx/vmx.c | 66 ++++++-------
arch/x86/kvm/x86.c | 38 ++++----
arch/x86/lib/insn-eval.c | 6 +-
arch/x86/lib/msr-smp.c | 16 +--
arch/x86/lib/msr.c | 4 +-
arch/x86/mm/pat/memtype.c | 4 +-
arch/x86/mm/tlb.c | 2 +-
arch/x86/pci/amd_bus.c | 10 +-
arch/x86/platform/olpc/olpc-xo1-rtc.c | 6 +-
arch/x86/platform/olpc/olpc-xo1-sci.c | 2 +-
arch/x86/power/cpu.c | 26 ++---
arch/x86/realmode/init.c | 2 +-
arch/x86/virt/svm/sev.c | 20 ++--
arch/x86/xen/suspend.c | 6 +-
drivers/acpi/acpi_extlog.c | 2 +-
drivers/acpi/acpi_lpit.c | 2 +-
drivers/cpufreq/acpi-cpufreq.c | 8 +-
drivers/cpufreq/amd-pstate-ut.c | 6 +-
drivers/cpufreq/amd-pstate.c | 22 ++---
drivers/cpufreq/amd_freq_sensitivity.c | 2 +-
drivers/cpufreq/e_powersaver.c | 6 +-
drivers/cpufreq/intel_pstate.c | 108 ++++++++++-----------
drivers/cpufreq/longhaul.c | 24 ++---
drivers/cpufreq/powernow-k7.c | 14 +--
drivers/crypto/ccp/sev-dev.c | 2 +-
drivers/edac/amd64_edac.c | 6 +-
drivers/gpu/drm/i915/selftests/librapl.c | 4 +-
drivers/hwmon/fam15h_power.c | 6 +-
drivers/idle/intel_idle.c | 34 +++----
drivers/mtd/nand/raw/cs553x_nand.c | 6 +-
drivers/platform/x86/intel/ifs/core.c | 4 +-
drivers/platform/x86/intel/ifs/load.c | 20 ++--
drivers/platform/x86/intel/ifs/runtest.c | 16 +--
drivers/platform/x86/intel/pmc/cnp.c | 6 +-
drivers/platform/x86/intel/pmc/core.c | 8 +-
.../x86/intel/speed_select_if/isst_if_common.c | 18 ++--
.../x86/intel/speed_select_if/isst_if_mbox_msr.c | 14 +--
.../x86/intel/speed_select_if/isst_tpmi_core.c | 2 +-
drivers/platform/x86/intel/tpmi_power_domains.c | 4 +-
drivers/platform/x86/intel/turbo_max_3.c | 4 +-
.../x86/intel/uncore-frequency/uncore-frequency.c | 10 +-
drivers/platform/x86/intel_ips.c | 36 +++----
drivers/powercap/intel_rapl_msr.c | 6 +-
.../int340x_thermal/processor_thermal_device.c | 2 +-
drivers/thermal/intel/intel_hfi.c | 14 +--
drivers/thermal/intel/intel_powerclamp.c | 4 +-
drivers/thermal/intel/intel_tcc_cooling.c | 4 +-
drivers/thermal/intel/therm_throt.c | 10 +-
drivers/video/fbdev/geode/gxfb_core.c | 2 +-
drivers/video/fbdev/geode/lxfb_ops.c | 22 ++---
drivers/video/fbdev/geode/suspend_gx.c | 10 +-
drivers/video/fbdev/geode/video_gx.c | 16 +--
include/hyperv/hvgdk_mini.h | 2 +-
144 files changed, 1034 insertions(+), 1034 deletions(-)
On 4/1/2025 12:52 AM, Ingo Molnar wrote:
>> Should we rename the *msrl() functions to *msrq() as part of this
>> overhaul?
> Yeah, that's a good idea, and because talk is cheap I just implemented
> this in the tip:WIP.x86/msr branch with a couple of other cleanups in
> this area (see the shortlog & diffstat below), but the churn is high:
>
> 144 files changed, 1034 insertions(+), 1034 deletions(-)
Hi Ingo,
I noticed that you keep the type of MSR index in these patches as
"unsigned int".
I'm thinking would it be better to standardize it as "u32"?
Because:
1) MSR index is placed in ECX to execute MSR instructions, and the
high-order 32 bits of RCX are ignored on 64-bit.
2) MSR index is encoded as a 32-bit immediate in the new immediate form
MSR instructions.
Thanks!
Xin
On April 2, 2025 10:09:21 PM PDT, Xin Li <xin@zytor.com> wrote: >On 4/1/2025 12:52 AM, Ingo Molnar wrote: >>> Should we rename the *msrl() functions to *msrq() as part of this >>> overhaul? >> Yeah, that's a good idea, and because talk is cheap I just implemented >> this in the tip:WIP.x86/msr branch with a couple of other cleanups in >> this area (see the shortlog & diffstat below), but the churn is high: >> >> 144 files changed, 1034 insertions(+), 1034 deletions(-) > >Hi Ingo, > >I noticed that you keep the type of MSR index in these patches as >"unsigned int". > >I'm thinking would it be better to standardize it as "u32"? > >Because: >1) MSR index is placed in ECX to execute MSR instructions, and the > high-order 32 bits of RCX are ignored on 64-bit. >2) MSR index is encoded as a 32-bit immediate in the new immediate form > MSR instructions. > >Thanks! > Xin "unsigned int" and "u32" are synonymous, but the latter is more explicit and would be better.
* Xin Li <xin@zytor.com> wrote:
> On 4/1/2025 12:52 AM, Ingo Molnar wrote:
> > > Should we rename the *msrl() functions to *msrq() as part of this
> > > overhaul?
> > Yeah, that's a good idea, and because talk is cheap I just implemented
> > this in the tip:WIP.x86/msr branch with a couple of other cleanups in
> > this area (see the shortlog & diffstat below), but the churn is high:
> >
> > 144 files changed, 1034 insertions(+), 1034 deletions(-)
>
> Hi Ingo,
>
> I noticed that you keep the type of MSR index in these patches as
> "unsigned int".
>
> I'm thinking would it be better to standardize it as "u32"?
>
> Because:
> 1) MSR index is placed in ECX to execute MSR instructions, and the
> high-order 32 bits of RCX are ignored on 64-bit.
> 2) MSR index is encoded as a 32-bit immediate in the new immediate form
> MSR instructions.
Makes sense - something like the attached patch?
Thanks,
Ingo
=====================>
From: Ingo Molnar <mingo@kernel.org>
Date: Wed, 9 Apr 2025 21:12:39 +0200
Subject: [PATCH] x86/msr: Standardize on 'u32' MSR indices in <asm/msr.h>
This is the customary type used for hardware ABIs.
Suggested-by: Xin Li <xin@zytor.com>
Suggested-by: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
arch/x86/include/asm/msr.h | 29 ++++++++++++++---------------
arch/x86/lib/msr.c | 4 ++--
2 files changed, 16 insertions(+), 17 deletions(-)
diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 4ee9ae734c08..20deb58308e5 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -63,12 +63,12 @@ struct saved_msrs {
DECLARE_TRACEPOINT(read_msr);
DECLARE_TRACEPOINT(write_msr);
DECLARE_TRACEPOINT(rdpmc);
-extern void do_trace_write_msr(unsigned int msr, u64 val, int failed);
-extern void do_trace_read_msr(unsigned int msr, u64 val, int failed);
+extern void do_trace_write_msr(u32 msr, u64 val, int failed);
+extern void do_trace_read_msr(u32 msr, u64 val, int failed);
extern void do_trace_rdpmc(u32 msr, u64 val, int failed);
#else
-static inline void do_trace_write_msr(unsigned int msr, u64 val, int failed) {}
-static inline void do_trace_read_msr(unsigned int msr, u64 val, int failed) {}
+static inline void do_trace_write_msr(u32 msr, u64 val, int failed) {}
+static inline void do_trace_read_msr(u32 msr, u64 val, int failed) {}
static inline void do_trace_rdpmc(u32 msr, u64 val, int failed) {}
#endif
@@ -79,7 +79,7 @@ static inline void do_trace_rdpmc(u32 msr, u64 val, int failed) {}
* think of extending them - you will be slapped with a stinking trout or a frozen
* shark will reach you, wherever you are! You've been warned.
*/
-static __always_inline u64 __rdmsr(unsigned int msr)
+static __always_inline u64 __rdmsr(u32 msr)
{
DECLARE_ARGS(val, low, high);
@@ -91,7 +91,7 @@ static __always_inline u64 __rdmsr(unsigned int msr)
return EAX_EDX_VAL(val, low, high);
}
-static __always_inline void __wrmsr(unsigned int msr, u32 low, u32 high)
+static __always_inline void __wrmsr(u32 msr, u32 low, u32 high)
{
asm volatile("1: wrmsr\n"
"2:\n"
@@ -113,7 +113,7 @@ do { \
__wrmsr((msr), (u32)((u64)(val)), \
(u32)((u64)(val) >> 32))
-static inline u64 native_read_msr(unsigned int msr)
+static inline u64 native_read_msr(u32 msr)
{
u64 val;
@@ -125,8 +125,7 @@ static inline u64 native_read_msr(unsigned int msr)
return val;
}
-static inline u64 native_read_msr_safe(unsigned int msr,
- int *err)
+static inline u64 native_read_msr_safe(u32 msr, int *err)
{
DECLARE_ARGS(val, low, high);
@@ -142,7 +141,7 @@ static inline u64 native_read_msr_safe(unsigned int msr,
/* Can be uninlined because referenced by paravirt */
static inline void notrace
-native_write_msr(unsigned int msr, u32 low, u32 high)
+native_write_msr(u32 msr, u32 low, u32 high)
{
__wrmsr(msr, low, high);
@@ -152,7 +151,7 @@ native_write_msr(unsigned int msr, u32 low, u32 high)
/* Can be uninlined because referenced by paravirt */
static inline int notrace
-native_write_msr_safe(unsigned int msr, u32 low, u32 high)
+native_write_msr_safe(u32 msr, u32 low, u32 high)
{
int err;
@@ -251,7 +250,7 @@ do { \
(void)((high) = (u32)(__val >> 32)); \
} while (0)
-static inline void wrmsr(unsigned int msr, u32 low, u32 high)
+static inline void wrmsr(u32 msr, u32 low, u32 high)
{
native_write_msr(msr, low, high);
}
@@ -259,13 +258,13 @@ static inline void wrmsr(unsigned int msr, u32 low, u32 high)
#define rdmsrq(msr, val) \
((val) = native_read_msr((msr)))
-static inline void wrmsrq(unsigned int msr, u64 val)
+static inline void wrmsrq(u32 msr, u64 val)
{
native_write_msr(msr, (u32)(val & 0xffffffffULL), (u32)(val >> 32));
}
/* wrmsr with exception handling */
-static inline int wrmsr_safe(unsigned int msr, u32 low, u32 high)
+static inline int wrmsr_safe(u32 msr, u32 low, u32 high)
{
return native_write_msr_safe(msr, low, high);
}
@@ -280,7 +279,7 @@ static inline int wrmsr_safe(unsigned int msr, u32 low, u32 high)
__err; \
})
-static inline int rdmsrq_safe(unsigned int msr, u64 *p)
+static inline int rdmsrq_safe(u32 msr, u64 *p)
{
int err;
diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
index e18925899f13..4ef7c6dcbea6 100644
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -122,14 +122,14 @@ int msr_clear_bit(u32 msr, u8 bit)
EXPORT_SYMBOL_GPL(msr_clear_bit);
#ifdef CONFIG_TRACEPOINTS
-void do_trace_write_msr(unsigned int msr, u64 val, int failed)
+void do_trace_write_msr(u32 msr, u64 val, int failed)
{
trace_write_msr(msr, val, failed);
}
EXPORT_SYMBOL(do_trace_write_msr);
EXPORT_TRACEPOINT_SYMBOL(write_msr);
-void do_trace_read_msr(unsigned int msr, u64 val, int failed)
+void do_trace_read_msr(u32 msr, u64 val, int failed)
{
trace_read_msr(msr, val, failed);
}On 4/1/2025 12:52 AM, Ingo Molnar wrote:
>
> * H. Peter Anvin <hpa@zytor.com> wrote:
>
>> On March 31, 2025 3:17:30 AM PDT, Ingo Molnar <mingo@kernel.org> wrote:
>>>
>>> * Xin Li (Intel) <xin@zytor.com> wrote:
>>>
>>>> - __wrmsr (MSR_AMD_DBG_EXTN_CFG, val | 3ULL << 3, val >> 32);
>>>> + native_wrmsrl(MSR_AMD_DBG_EXTN_CFG, val | 3ULL << 3);
>>>
>>> This is an improvement.
>>>
>>>> - __wrmsr (MSR_IA32_PQR_ASSOC, rmid_p, plr->closid);
>>>> + native_wrmsrl(MSR_IA32_PQR_ASSOC, (u64)plr->closid << 32 | rmid_p);
>>>
>>>> - __wrmsr (MSR_IA32_PQR_ASSOC, rmid_p, closid_p);
>>>> + native_wrmsrl(MSR_IA32_PQR_ASSOC, (u64)closid_p << 32 | rmid_p);
>>>
>>> This is not an improvement.
>>>
>>> Please provide a native_wrmsrl() API variant where natural [rmid_p, closid_p]
>>> high/lo parameters can be used, without the shift-uglification...
>>>
>>> Thanks,
>>>
>>> Ingo
>>
>> Directing this question primarily to Ingo, who is more than anyone
>> else the namespace consistency guardian:
>>
>> On the subject of msr function naming ... *msrl() has always been
>> misleading. The -l suffix usually means 32 bits; sometimes it means
>> the C type "long" (which in the kernel is used instead of
>> size_t/uintptr_t, which might end up being "fun" when 128-bit
>> architectures appear some time this century), but for a fixed 64-but
>> type we normally use -q.
>
> Yeah, agreed - that's been bothering me for a while too. :-)
>
>> Should we rename the *msrl() functions to *msrq() as part of this
>> overhaul?
>
> Yeah, that's a good idea, and because talk is cheap I just implemented
> this in the tip:WIP.x86/msr branch with a couple of other cleanups in
> this area (see the shortlog & diffstat below), but the churn is high:
>
> 144 files changed, 1034 insertions(+), 1034 deletions(-)
>
> So this can only be done if regenerated and sent to Linus right before
> an -rc1 I think:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip WIP.x86/msr
Hi Ingo,
Is this branch public?
I wanted to rebase on it and then incooperate your review comments, but
couldn't find the branch.
Thanks!
Xin
>
> Thanks,
>
> Ingo
>
> =======================>
> Ingo Molnar (18):
> x86/msr: Standardize on u64 in <asm/msr.h>
> x86/msr: Standardize on u64 in <asm/msr-index.h>
> x86/msr: Use u64 in rdmsrl_amd_safe() and wrmsrl_amd_safe()
> x86/msr: Use u64 in rdmsrl_safe() and paravirt_read_pmc()
> x86/msr: Rename 'rdmsrl()' to 'rdmsrq()'
> x86/msr: Rename 'wrmsrl()' to 'wrmsrq()'
> x86/msr: Rename 'rdmsrl_safe()' to 'rdmsrq_safe()'
> x86/msr: Rename 'wrmsrl_safe()' to 'wrmsrq_safe()'
> x86/msr: Rename 'rdmsrl_safe_on_cpu()' to 'rdmsrq_safe_on_cpu()'
> x86/msr: Rename 'wrmsrl_safe_on_cpu()' to 'wrmsrq_safe_on_cpu()'
> x86/msr: Rename 'rdmsrl_on_cpu()' to 'rdmsrq_on_cpu()'
> x86/msr: Rename 'wrmsrl_on_cpu()' to 'wrmsrq_on_cpu()'
> x86/msr: Rename 'mce_rdmsrl()' to 'mce_rdmsrq()'
> x86/msr: Rename 'mce_wrmsrl()' to 'mce_wrmsrq()'
> x86/msr: Rename 'rdmsrl_amd_safe()' to 'rdmsrq_amd_safe()'
> x86/msr: Rename 'wrmsrl_amd_safe()' to 'wrmsrq_amd_safe()'
> x86/msr: Rename 'native_wrmsrl()' to 'native_wrmsrq()'
> x86/msr: Rename 'wrmsrl_cstar()' to 'wrmsrq_cstar()'
>
> arch/x86/coco/sev/core.c | 2 +-
> arch/x86/events/amd/brs.c | 8 +-
> arch/x86/events/amd/core.c | 12 +--
> arch/x86/events/amd/ibs.c | 26 ++---
> arch/x86/events/amd/lbr.c | 20 ++--
> arch/x86/events/amd/power.c | 10 +-
> arch/x86/events/amd/uncore.c | 12 +--
> arch/x86/events/core.c | 42 ++++----
> arch/x86/events/intel/core.c | 66 ++++++-------
> arch/x86/events/intel/cstate.c | 2 +-
> arch/x86/events/intel/ds.c | 10 +-
> arch/x86/events/intel/knc.c | 16 +--
> arch/x86/events/intel/lbr.c | 44 ++++-----
> arch/x86/events/intel/p4.c | 24 ++---
> arch/x86/events/intel/p6.c | 12 +--
> arch/x86/events/intel/pt.c | 32 +++---
> arch/x86/events/intel/uncore.c | 2 +-
> arch/x86/events/intel/uncore_discovery.c | 10 +-
> arch/x86/events/intel/uncore_nhmex.c | 70 ++++++-------
> arch/x86/events/intel/uncore_snb.c | 42 ++++----
> arch/x86/events/intel/uncore_snbep.c | 50 +++++-----
> arch/x86/events/msr.c | 2 +-
> arch/x86/events/perf_event.h | 26 ++---
> arch/x86/events/probe.c | 2 +-
> arch/x86/events/rapl.c | 8 +-
> arch/x86/events/zhaoxin/core.c | 16 +--
> arch/x86/hyperv/hv_apic.c | 4 +-
> arch/x86/hyperv/hv_init.c | 66 ++++++-------
> arch/x86/hyperv/hv_spinlock.c | 6 +-
> arch/x86/hyperv/ivm.c | 2 +-
> arch/x86/include/asm/apic.h | 8 +-
> arch/x86/include/asm/debugreg.h | 4 +-
> arch/x86/include/asm/fsgsbase.h | 4 +-
> arch/x86/include/asm/kvm_host.h | 2 +-
> arch/x86/include/asm/microcode.h | 2 +-
> arch/x86/include/asm/msr-index.h | 12 +--
> arch/x86/include/asm/msr.h | 50 +++++-----
> arch/x86/include/asm/paravirt.h | 8 +-
> arch/x86/include/asm/spec-ctrl.h | 2 +-
> arch/x86/kernel/acpi/cppc.c | 8 +-
> arch/x86/kernel/amd_nb.c | 2 +-
> arch/x86/kernel/apic/apic.c | 16 +--
> arch/x86/kernel/apic/apic_numachip.c | 6 +-
> arch/x86/kernel/cet.c | 2 +-
> arch/x86/kernel/cpu/amd.c | 28 +++---
> arch/x86/kernel/cpu/aperfmperf.c | 28 +++---
> arch/x86/kernel/cpu/bugs.c | 24 ++---
> arch/x86/kernel/cpu/bus_lock.c | 18 ++--
> arch/x86/kernel/cpu/common.c | 68 ++++++-------
> arch/x86/kernel/cpu/feat_ctl.c | 4 +-
> arch/x86/kernel/cpu/hygon.c | 6 +-
> arch/x86/kernel/cpu/intel.c | 10 +-
> arch/x86/kernel/cpu/intel_epb.c | 12 +--
> arch/x86/kernel/cpu/mce/amd.c | 22 ++---
> arch/x86/kernel/cpu/mce/core.c | 58 +++++------
> arch/x86/kernel/cpu/mce/inject.c | 32 +++---
> arch/x86/kernel/cpu/mce/intel.c | 32 +++---
> arch/x86/kernel/cpu/mce/internal.h | 2 +-
> arch/x86/kernel/cpu/microcode/amd.c | 2 +-
> arch/x86/kernel/cpu/microcode/intel.c | 2 +-
> arch/x86/kernel/cpu/mshyperv.c | 12 +--
> arch/x86/kernel/cpu/resctrl/core.c | 10 +-
> arch/x86/kernel/cpu/resctrl/monitor.c | 2 +-
> arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 2 +-
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 6 +-
> arch/x86/kernel/cpu/sgx/main.c | 2 +-
> arch/x86/kernel/cpu/topology.c | 2 +-
> arch/x86/kernel/cpu/topology_amd.c | 4 +-
> arch/x86/kernel/cpu/tsx.c | 20 ++--
> arch/x86/kernel/cpu/umwait.c | 2 +-
> arch/x86/kernel/fpu/core.c | 2 +-
> arch/x86/kernel/fpu/xstate.c | 10 +-
> arch/x86/kernel/fpu/xstate.h | 2 +-
> arch/x86/kernel/fred.c | 20 ++--
> arch/x86/kernel/hpet.c | 2 +-
> arch/x86/kernel/kvm.c | 28 +++---
> arch/x86/kernel/kvmclock.c | 4 +-
> arch/x86/kernel/mmconf-fam10h_64.c | 8 +-
> arch/x86/kernel/process.c | 16 +--
> arch/x86/kernel/process_64.c | 20 ++--
> arch/x86/kernel/reboot_fixups_32.c | 2 +-
> arch/x86/kernel/shstk.c | 18 ++--
> arch/x86/kernel/traps.c | 10 +-
> arch/x86/kernel/tsc.c | 2 +-
> arch/x86/kernel/tsc_sync.c | 14 +--
> arch/x86/kvm/svm/avic.c | 2 +-
> arch/x86/kvm/svm/sev.c | 2 +-
> arch/x86/kvm/svm/svm.c | 16 +--
> arch/x86/kvm/vmx/nested.c | 4 +-
> arch/x86/kvm/vmx/pmu_intel.c | 4 +-
> arch/x86/kvm/vmx/sgx.c | 8 +-
> arch/x86/kvm/vmx/vmx.c | 66 ++++++-------
> arch/x86/kvm/x86.c | 38 ++++----
> arch/x86/lib/insn-eval.c | 6 +-
> arch/x86/lib/msr-smp.c | 16 +--
> arch/x86/lib/msr.c | 4 +-
> arch/x86/mm/pat/memtype.c | 4 +-
> arch/x86/mm/tlb.c | 2 +-
> arch/x86/pci/amd_bus.c | 10 +-
> arch/x86/platform/olpc/olpc-xo1-rtc.c | 6 +-
> arch/x86/platform/olpc/olpc-xo1-sci.c | 2 +-
> arch/x86/power/cpu.c | 26 ++---
> arch/x86/realmode/init.c | 2 +-
> arch/x86/virt/svm/sev.c | 20 ++--
> arch/x86/xen/suspend.c | 6 +-
> drivers/acpi/acpi_extlog.c | 2 +-
> drivers/acpi/acpi_lpit.c | 2 +-
> drivers/cpufreq/acpi-cpufreq.c | 8 +-
> drivers/cpufreq/amd-pstate-ut.c | 6 +-
> drivers/cpufreq/amd-pstate.c | 22 ++---
> drivers/cpufreq/amd_freq_sensitivity.c | 2 +-
> drivers/cpufreq/e_powersaver.c | 6 +-
> drivers/cpufreq/intel_pstate.c | 108 ++++++++++-----------
> drivers/cpufreq/longhaul.c | 24 ++---
> drivers/cpufreq/powernow-k7.c | 14 +--
> drivers/crypto/ccp/sev-dev.c | 2 +-
> drivers/edac/amd64_edac.c | 6 +-
> drivers/gpu/drm/i915/selftests/librapl.c | 4 +-
> drivers/hwmon/fam15h_power.c | 6 +-
> drivers/idle/intel_idle.c | 34 +++----
> drivers/mtd/nand/raw/cs553x_nand.c | 6 +-
> drivers/platform/x86/intel/ifs/core.c | 4 +-
> drivers/platform/x86/intel/ifs/load.c | 20 ++--
> drivers/platform/x86/intel/ifs/runtest.c | 16 +--
> drivers/platform/x86/intel/pmc/cnp.c | 6 +-
> drivers/platform/x86/intel/pmc/core.c | 8 +-
> .../x86/intel/speed_select_if/isst_if_common.c | 18 ++--
> .../x86/intel/speed_select_if/isst_if_mbox_msr.c | 14 +--
> .../x86/intel/speed_select_if/isst_tpmi_core.c | 2 +-
> drivers/platform/x86/intel/tpmi_power_domains.c | 4 +-
> drivers/platform/x86/intel/turbo_max_3.c | 4 +-
> .../x86/intel/uncore-frequency/uncore-frequency.c | 10 +-
> drivers/platform/x86/intel_ips.c | 36 +++----
> drivers/powercap/intel_rapl_msr.c | 6 +-
> .../int340x_thermal/processor_thermal_device.c | 2 +-
> drivers/thermal/intel/intel_hfi.c | 14 +--
> drivers/thermal/intel/intel_powerclamp.c | 4 +-
> drivers/thermal/intel/intel_tcc_cooling.c | 4 +-
> drivers/thermal/intel/therm_throt.c | 10 +-
> drivers/video/fbdev/geode/gxfb_core.c | 2 +-
> drivers/video/fbdev/geode/lxfb_ops.c | 22 ++---
> drivers/video/fbdev/geode/suspend_gx.c | 10 +-
> drivers/video/fbdev/geode/video_gx.c | 16 +--
> include/hyperv/hvgdk_mini.h | 2 +-
> 144 files changed, 1034 insertions(+), 1034 deletions(-)
* Xin Li <xin@zytor.com> wrote: > Hi Ingo, > > Is this branch public? > > I wanted to rebase on it and then incooperate your review comments, but > couldn't find the branch. Yeah, I moved it over to: git://git.kernel.org/pub/scm/linux/kernel/git/mingo/tip.git WIP.x86/msr Thanks, Ingo
On 4/1/2025 9:10 PM, Ingo Molnar wrote:
> Yeah, I moved it over to:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mingo/tip.git WIP.x86/msr
>
Hi Ingo,
Are you going to merge it into tip in this development cycle for the
v6.16 merge window?
Thanks!
Xin
On 4/1/2025 9:10 PM, Ingo Molnar wrote:
> Yeah, I moved it over to:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mingo/tip.git WIP.x86/msr
On it now.
Thanks!
Xin
On 3/31/2025 1:32 PM, H. Peter Anvin wrote:
> On March 31, 2025 3:17:30 AM PDT, Ingo Molnar <mingo@kernel.org> wrote:
>>
>> * Xin Li (Intel) <xin@zytor.com> wrote:
>>
>>> - __wrmsr (MSR_AMD_DBG_EXTN_CFG, val | 3ULL << 3, val >> 32);
>>> + native_wrmsrl(MSR_AMD_DBG_EXTN_CFG, val | 3ULL << 3);
>>
>> This is an improvement.
>>
>>> - __wrmsr (MSR_IA32_PQR_ASSOC, rmid_p, plr->closid);
>>> + native_wrmsrl(MSR_IA32_PQR_ASSOC, (u64)plr->closid << 32 | rmid_p);
>>
>>> - __wrmsr (MSR_IA32_PQR_ASSOC, rmid_p, closid_p);
>>> + native_wrmsrl(MSR_IA32_PQR_ASSOC, (u64)closid_p << 32 | rmid_p);
>>
>> This is not an improvement.
>>
>> Please provide a native_wrmsrl() API variant where natural [rmid_p, closid_p]
>> high/lo parameters can be used, without the shift-uglification...
>>
>> Thanks,
>>
>> Ingo
>
> Directing this question primarily to Ingo, who is more than anyone else the namespace consistency guardian:
>
> On the subject of msr function naming ... *msrl() has always been misleading. The -l suffix usually means 32 bits; sometimes it means the C type "long" (which in the kernel is used instead of size_t/uintptr_t, which might end up being "fun" when 128-bit architectures appear some time this century), but for a fixed 64-but type we normally use -q.
>
> Should we rename the *msrl() functions to *msrq() as part of this overhaul?
>
Per "struct msr" defined in arch/x86/include/asm/shared/msr.h:
struct msr {
union {
struct {
u32 l;
u32 h;
};
u64 q;
};
};
Probably *msrq() is what we want?
On 3/31/25 22:53, Xin Li wrote:
> Per "struct msr" defined in arch/x86/include/asm/shared/msr.h:
>
> struct msr {
> union {
> struct {
> u32 l;
> u32 h;
> };
> u64 q;
> };
> };
>
> Probably *msrq() is what we want?
What would folks think about "wrmsr64()"? It's writing a 64-bit value to
an MSR and there are a lot of functions in the kernel that are named
with the argument width in bits.
On April 2, 2025 8:41:07 AM PDT, Dave Hansen <dave.hansen@intel.com> wrote:
>On 3/31/25 22:53, Xin Li wrote:
>> Per "struct msr" defined in arch/x86/include/asm/shared/msr.h:
>>
>> struct msr {
>> union {
>> struct {
>> u32 l;
>> u32 h;
>> };
>> u64 q;
>> };
>> };
>>
>> Probably *msrq() is what we want?
>
>What would folks think about "wrmsr64()"? It's writing a 64-bit value to
>an MSR and there are a lot of functions in the kernel that are named
>with the argument width in bits.
Personally, I hate the extra verbosity, mostly visual, since numerals are nearly as prominent as capital letters they tend to attract the eye. There is a reason why they aren't used this way in assembly languages.
* H. Peter Anvin <hpa@zytor.com> wrote:
> On April 2, 2025 8:41:07 AM PDT, Dave Hansen <dave.hansen@intel.com> wrote:
> >On 3/31/25 22:53, Xin Li wrote:
> >> Per "struct msr" defined in arch/x86/include/asm/shared/msr.h:
> >>
> >> struct msr {
> >> union {
> >> struct {
> >> u32 l;
> >> u32 h;
> >> };
> >> u64 q;
> >> };
> >> };
> >>
> >> Probably *msrq() is what we want?
> >
> > What would folks think about "wrmsr64()"? It's writing a 64-bit
> > value to an MSR and there are a lot of functions in the kernel that
> > are named with the argument width in bits.
>
> Personally, I hate the extra verbosity, mostly visual, since numerals
> are nearly as prominent as capital letters they tend to attract the
> eye. There is a reason why they aren't used this way in assembly
> languages.
So what's the consensus here? Both work for me, but I have to pick one. :-)
Thanks,
Ingo
On 4/9/25 12:53, Ingo Molnar wrote: >>> What would folks think about "wrmsr64()"? It's writing a 64-bit >>> value to an MSR and there are a lot of functions in the kernel that >>> are named with the argument width in bits. >> Personally, I hate the extra verbosity, mostly visual, since numerals >> are nearly as prominent as capital letters they tend to attract the >> eye. There is a reason why they aren't used this way in assembly >> languages. > So what's the consensus here? Both work for me, but I have to pick one. 🙂 I don't feel strongly about it. You're not going to hurt my feelings if you pick the "q" one, so go for "q" unless you have a real preference.
* Dave Hansen <dave.hansen@intel.com> wrote: > On 4/9/25 12:53, Ingo Molnar wrote: > >>> What would folks think about "wrmsr64()"? It's writing a 64-bit > >>> value to an MSR and there are a lot of functions in the kernel that > >>> are named with the argument width in bits. > >> Personally, I hate the extra verbosity, mostly visual, since numerals > >> are nearly as prominent as capital letters they tend to attract the > >> eye. There is a reason why they aren't used this way in assembly > >> languages. > > So what's the consensus here? Both work for me, but I have to pick one. 🙂 > > I don't feel strongly about it. You're not going to hurt my feelings if > you pick the "q" one, so go for "q" unless you have a real preference. Ok, since hpa seems to hate the wrmsr64()/rdmsr64() names due to the numeric verbosity, I'll go with wrmsrq()/rdmsrq(). Thanks, Ingo
© 2016 - 2025 Red Hat, Inc.