[tip:x86/asm] x86/asm/msr: Make wrmsrl_safe() a function

[tip:x86/asm] x86/asm/msr: Make wrmsrl_safe() a function

[tip-bot for Andy Lutomirski]

Commit-ID:  cf991de2f614f454b3cb2a300c06ecdf69f3a70d
Gitweb:     http://git.kernel.org/tip/cf991de2f614f454b3cb2a300c06ecdf69f3a70d
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Thu, 4 Jun 2015 17:13:44 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 5 Jun 2015 09:41:22 +0200

x86/asm/msr: Make wrmsrl_safe() a function

The wrmsrl_safe macro performs invalid shifts if the value
argument is 32 bits.  This makes it unnecessarily awkward to
write code that puts an unsigned long into an MSR.

Convert it to a real inline function.

For inspiration, see:

  7c74d5b7b7b6 ("x86/asm/entry/64: Fix MSR_IA32_SYSENTER_CS MSR value").

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: <linux-kernel@vger.kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
[ Applied small improvements. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/msr.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index de36f22..13bf576 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -205,8 +205,13 @@ do {                                                            \
 
 #endif	/* !CONFIG_PARAVIRT */
 
-#define wrmsrl_safe(msr, val) wrmsr_safe((msr), (u32)(val),		\
-					     (u32)((val) >> 32))
+/*
+ * 64-bit version of wrmsr_safe():
+ */
+static inline int wrmsrl_safe(u32 msr, u64 val)
+{
+	return wrmsr_safe(msr, (u32)val,  (u32)(val >> 32));
+}
 
 #define write_tsc(low, high) wrmsr(MSR_IA32_TSC, (low), (high))
 

Re: [tip:x86/asm] x86/asm/msr: Make wrmsrl_safe() a function

[H. Peter Anvin]

Shouldn't we make it a proper function sine there is going to have to be a function call involved anyway?

On June 5, 2015 1:42:56 AM PDT, tip-bot for Andy Lutomirski <tipbot@zytor.com> wrote:
>Commit-ID:  cf991de2f614f454b3cb2a300c06ecdf69f3a70d
>Gitweb:    
>http://git.kernel.org/tip/cf991de2f614f454b3cb2a300c06ecdf69f3a70d
>Author:     Andy Lutomirski <luto@kernel.org>
>AuthorDate: Thu, 4 Jun 2015 17:13:44 -0700
>Committer:  Ingo Molnar <mingo@kernel.org>
>CommitDate: Fri, 5 Jun 2015 09:41:22 +0200
>
>x86/asm/msr: Make wrmsrl_safe() a function
>
>The wrmsrl_safe macro performs invalid shifts if the value
>argument is 32 bits.  This makes it unnecessarily awkward to
>write code that puts an unsigned long into an MSR.
>
>Convert it to a real inline function.
>
>For inspiration, see:
>
> 7c74d5b7b7b6 ("x86/asm/entry/64: Fix MSR_IA32_SYSENTER_CS MSR value").
>
>Signed-off-by: Andy Lutomirski <luto@kernel.org>
>Cc: <linux-kernel@vger.kernel.org>
>Cc: Andrew Morton <akpm@linux-foundation.org>
>Cc: H. Peter Anvin <hpa@zytor.com>
>Cc: Linus Torvalds <torvalds@linux-foundation.org>
>Cc: Peter Zijlstra <peterz@infradead.org>
>Cc: Thomas Gleixner <tglx@linutronix.de>
>[ Applied small improvements. ]
>Signed-off-by: Ingo Molnar <mingo@kernel.org>
>---
> arch/x86/include/asm/msr.h | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
>diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
>index de36f22..13bf576 100644
>--- a/arch/x86/include/asm/msr.h
>+++ b/arch/x86/include/asm/msr.h
>@@ -205,8 +205,13 @@ do {                                              
>             \
> 
> #endif	/* !CONFIG_PARAVIRT */
> 
>-#define wrmsrl_safe(msr, val) wrmsr_safe((msr), (u32)(val),		\
>-					     (u32)((val) >> 32))
>+/*
>+ * 64-bit version of wrmsr_safe():
>+ */
>+static inline int wrmsrl_safe(u32 msr, u64 val)
>+{
>+	return wrmsr_safe(msr, (u32)val,  (u32)(val >> 32));
>+}
> 
> #define write_tsc(low, high) wrmsr(MSR_IA32_TSC, (low), (high))
> 

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [tip:x86/asm] x86/asm/msr: Make wrmsrl_safe() a function

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> Shouldn't we make it a proper function sine there is going to have to be a 
> function call involved anyway?

Yeah, so what I think should be done instead is to flip around the API:
make wrmsrl_safe() the primary API and derive wrmsr_safe() from that,
because it's the saner API and because we have 3 times more wrmsrl_safe()
users right now!

And I'd make _that_ mapping inline, which would catch crap like:

  ./arch/x86/include/asm/msr.h:   return wrmsr_safe(msr, (u32)val,  (u32)(val >> 32));
  ./arch/x86/xen/enlighten.c:     wrmsr_safe(msr, (u32)pfn, (u32)(pfn >> 32));

and would turn it back into wrmsrl_safe(pfn)/etc. seemlessly.

In addition to that we might even phase out the high/low API altogether, as code 
like this:

            !wrmsr_safe(MSR_EFER,
                        header->pmode_efer_low,
                        header->pmode_efer_high))

should probably use a single u64.

But crappy paravirt indirections get in the way of an easy, trivial restructuring, 
as usual...

Thanks,

	Ingo

Re: [tip:x86/asm] x86/asm/msr: Make wrmsrl_safe() a function

[Borislav Petkov]

On Fri, Jun 05, 2015 at 12:04:42PM +0200, Ingo Molnar wrote:
> Yeah, so what I think should be done instead is to flip around the API:
> make wrmsrl_safe() the primary API and derive wrmsr_safe() from that,
> because it's the saner API and because we have 3 times more wrmsrl_safe()
> users right now!
> 
> And I'd make _that_ mapping inline, which would catch crap like:
> 
>   ./arch/x86/include/asm/msr.h:   return wrmsr_safe(msr, (u32)val,  (u32)(val >> 32));
>   ./arch/x86/xen/enlighten.c:     wrmsr_safe(msr, (u32)pfn, (u32)(pfn >> 32));
> 
> and would turn it back into wrmsrl_safe(pfn)/etc. seemlessly.
> 
> In addition to that we might even phase out the high/low API altogether, as code 
> like this:
> 
>             !wrmsr_safe(MSR_EFER,
>                         header->pmode_efer_low,
>                         header->pmode_efer_high))
> 
> should probably use a single u64.

There's also msr_read()/msr_write() in arch/x86/lib/msr.c.

They're not that straight-forward to use due to that struct msr thing
as the second arg but we could wrap the creation of that thing in a
higher-level caller. The advantage is that you can set both low, high
and the whole u64 values directly and got the error handling of the safe
variants.

> But crappy paravirt indirections get in the way of an easy, trivial
> restructuring, as usual...

Tell me about it. :/

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--