On Thu, 2007-03-22 at 10:09 +0200, Avi Kivity wrote:
> Right, scratch that. Was confused by rdmsr_safe().
Heh... me too 8)
> > The behaviour change (don't oops when an invalid rdmsr is used) was
> > there with CONFIG_PARAVIRT=y, the cleanup just made !CONFIG_PARAVIRT the
> > same. Is it important?
>
> I think so. A function should either never fail, or indicate that it
> has failed (panic, error return, debug message). With the kinder,
> gentler rdmsr() one can code the wrong msr value and not notice that
> something is wrong.
This is persuasive. I think restoring native_read_msr &
native_write_msr make sense, and Jeremy's BUG_ON suggestion will cover
the CONFIG_PARAVIRT case.
Something like this (I'll roll back into the main patch, retest, and
resend tomorrow).
Thanks!
Rusty.
diff -r 3ddb2898e72a include/asm-i386/msr.h
--- a/include/asm-i386/msr.h Thu Mar 22 19:31:11 2007 +1100
+++ b/include/asm-i386/msr.h Thu Mar 22 19:42:30 2007 +1100
@@ -3,7 +3,16 @@
#include <asm/errno.h>
-static inline unsigned long long native_read_msr(unsigned int msr, int *err)
+static inline unsigned long long native_read_msr(unsigned int msr)
+{
+ unsigned long long val;
+
+ asm volatile("rdmsr" : "=A" (val) : "c" (msr));
+ return val;
+}
+
+static inline unsigned long long native_read_msr_safe(unsigned int msr,
+ int *err)
{
unsigned long long val;
@@ -22,7 +31,13 @@ static inline unsigned long long native_
return val;
}
-static inline int native_write_msr(unsigned int msr, unsigned long long val)
+static inline void native_write_msr(unsigned int msr, unsigned long long val)
+{
+ asm volatile("wrmsr" : : "c" (msr), "A"(val));
+}
+
+static inline int native_write_msr_safe(unsigned int msr,
+ unsigned long long val)
{
int err;
asm volatile("2: wrmsr ; xorl %0,%0\n"
@@ -66,21 +81,17 @@ static inline unsigned long long native_
#define rdmsr(msr,val1,val2) \
do { \
- int __err; \
- unsigned long long __val = native_read_msr(msr, &__err);
\
+ unsigned long long __val = native_read_msr(msr); \
val1 = __val; \
val2 = __val >> 32; \
} while(0)
#define wrmsr(msr,val1,val2) \
- do { \
- native_write_msr(msr, ((unsigned long long)val2 << 32) | val1);
\
- } while(0)
+ native_write_msr(msr, ((unsigned long long)val2 << 32) | val1)
#define rdmsrl(msr,val) \
do { \
- int __err; \
- (val) = native_read_msr(msr, &__err); \
+ (val) = native_read_msr(msr); \
} while(0)
static inline void wrmsrl (unsigned long msr, unsigned long long val)
@@ -93,13 +104,13 @@ static inline void wrmsrl (unsigned long
/* wrmsr with exception handling */
#define wrmsr_safe(msr,val1,val2)
\
- (native_write_msr(msr, ((unsigned long long)val2 << 32) | val1))
+ (native_write_msr_safe(msr, ((unsigned long long)val2 << 32) | val1))
/* rdmsr with exception handling */
#define rdmsr_safe(msr,p1,p2) \
({ \
int __err; \
- unsigned long long __val = native_read_msr(msr, &__err);\
+ unsigned long long __val = native_read_msr_safe(msr, &__err);\
(*p1) = __val; \
(*p2) = __val >> 32; \
__err; \
diff -r 3ddb2898e72a include/asm-i386/paravirt.h
--- a/include/asm-i386/paravirt.h Thu Mar 22 19:31:11 2007 +1100
+++ b/include/asm-i386/paravirt.h Thu Mar 22 19:44:03 2007 +1100
@@ -237,19 +237,26 @@ static inline void halt(void)
u64 _l = paravirt_ops.read_msr(msr,&_err); \
val1 = (u32)_l; \
val2 = _l >> 32; \
+ BUG_ON(_err); \
} while(0)
#define wrmsr(msr,val1,val2) do { \
u64 _l = ((u64)(val2) << 32) | (val1); \
- paravirt_ops.write_msr((msr), _l); \
+ int _err = paravirt_ops.write_msr((msr), _l); \
+ BUG_ON(_err); \
} while(0)
#define rdmsrl(msr,val) do { \
int _err; \
val = paravirt_ops.read_msr((msr),&_err); \
-} while(0)
-
-#define wrmsrl(msr,val) (paravirt_ops.write_msr((msr),(val)))
+ BUG_ON(_err); \
+} while(0)
+
+#define wrmsrl(msr,val) do { \
+ int _err = paravirt_ops.write_msr((msr),(val)); \
+ BUG_ON(_err); \
+} while(0)
+
#define wrmsr_safe(msr,a,b) ({ \
u64 _l = ((u64)(b) << 32) | (a); \
paravirt_ops.write_msr((msr),_l); \
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
