thr3ads.net - Xen devel - [PATCH 0/4] x86, CPU, AMD: Cleanup AMD-specific MSR-rw users [Jun 2012]

If this information is useful, please help other people find it:
Share via:

Borislav Petkov

2012-Jun-01 14:52 UTC

[PATCH 0/4] x86, CPU, AMD: Cleanup AMD-specific MSR-rw users

From: Borislav Petkov <borislav.petkov@amd.com>

Ok,

the following patchset should take care of all the {rd,wr}msrl_amd_safe
headaches we had this week. After it is applied, the AMD-specific
variants become private to amd.c and issue a warning when used on
anything else beside K8.

This also contains the two patches from Andre which cleanup the PV-side
of things.

Thanks.

Borislav Petkov

2012-Jun-01 14:52 UTC

head link

[PATCH 1/4] x86, pvops: Remove hooks for {rd, wr}msr_safe_regs

From: Andre Przywara <andre.przywara@amd.com>

There were paravirt_ops hooks for the full register set variant of
{rd,wr}msr_safe which are actually not used by anyone anymore. Remove
them to make the code cleaner and avoid silent breakages when the pvops
members were uninitialized. This has been boot-tested natively and under
Xen with PVOPS enabled and disabled on one machine.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>
---
 arch/x86/include/asm/msr.h            |   67 ++++++++++++++-------------------
 arch/x86/include/asm/paravirt.h       |   39 -------------------
 arch/x86/include/asm/paravirt_types.h |    2 -
 arch/x86/kernel/paravirt.c            |    2 -
 arch/x86/lib/msr-reg-export.c         |    4 +-
 arch/x86/lib/msr-reg.S                |   10 ++---
 arch/x86/xen/enlighten.c              |    2 -
 7 files changed, 35 insertions(+), 91 deletions(-)

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 084ef95274cd..81860cc012d1 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -115,8 +115,8 @@ notrace static inline int native_write_msr_safe(unsigned int
msr,
 
 extern unsigned long long native_read_tsc(void);
 
-extern int native_rdmsr_safe_regs(u32 regs[8]);
-extern int native_wrmsr_safe_regs(u32 regs[8]);
+extern int rdmsr_safe_regs(u32 regs[8]);
+extern int wrmsr_safe_regs(u32 regs[8]);
 
 static __always_inline unsigned long long __native_read_tsc(void)
 {
@@ -187,43 +187,6 @@ static inline int rdmsrl_safe(unsigned msr, unsigned long
long *p)
 	return err;
 }
 
-static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p)
-{
-	u32 gprs[8] = { 0 };
-	int err;
-
-	gprs[1] = msr;
-	gprs[7] = 0x9c5a203a;
-
-	err = native_rdmsr_safe_regs(gprs);
-
-	*p = gprs[0] | ((u64)gprs[2] << 32);
-
-	return err;
-}
-
-static inline int wrmsrl_amd_safe(unsigned msr, unsigned long long val)
-{
-	u32 gprs[8] = { 0 };
-
-	gprs[0] = (u32)val;
-	gprs[1] = msr;
-	gprs[2] = val >> 32;
-	gprs[7] = 0x9c5a203a;
-
-	return native_wrmsr_safe_regs(gprs);
-}
-
-static inline int rdmsr_safe_regs(u32 regs[8])
-{
-	return native_rdmsr_safe_regs(regs);
-}
-
-static inline int wrmsr_safe_regs(u32 regs[8])
-{
-	return native_wrmsr_safe_regs(regs);
-}
-
 #define rdtscl(low)						\
 	((low) = (u32)__native_read_tsc())
 
@@ -248,6 +211,32 @@ do {                                                       
\
 
 #endif	/* !CONFIG_PARAVIRT */
 
+static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p)
+{
+	u32 gprs[8] = { 0 };
+	int err;
+
+	gprs[1] = msr;
+	gprs[7] = 0x9c5a203a;
+
+	err = rdmsr_safe_regs(gprs);
+
+	*p = gprs[0] | ((u64)gprs[2] << 32);
+
+	return err;
+}
+
+static inline int wrmsrl_amd_safe(unsigned msr, unsigned long long val)
+{
+	u32 gprs[8] = { 0 };
+
+	gprs[0] = (u32)val;
+	gprs[1] = msr;
+	gprs[2] = val >> 32;
+	gprs[7] = 0x9c5a203a;
+
+	return wrmsr_safe_regs(gprs);
+}
 
 #define checking_wrmsrl(msr, val) wrmsr_safe((msr), (u32)(val),		\
 					     (u32)((val) >> 32))
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 6cbbabf52707..ebb0cdb60a89 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -128,21 +128,11 @@ static inline u64 paravirt_read_msr(unsigned msr, int
*err)
 	return PVOP_CALL2(u64, pv_cpu_ops.read_msr, msr, err);
 }
 
-static inline int paravirt_rdmsr_regs(u32 *regs)
-{
-	return PVOP_CALL1(int, pv_cpu_ops.rdmsr_regs, regs);
-}
-
 static inline int paravirt_write_msr(unsigned msr, unsigned low, unsigned high)
 {
 	return PVOP_CALL3(int, pv_cpu_ops.write_msr, msr, low, high);
 }
 
-static inline int paravirt_wrmsr_regs(u32 *regs)
-{
-	return PVOP_CALL1(int, pv_cpu_ops.wrmsr_regs, regs);
-}
-
 /* These should all do BUG_ON(_err), but our headers are too tangled. */
 #define rdmsr(msr, val1, val2)			\
 do {						\
@@ -176,9 +166,6 @@ do {						\
 	_err;					\
 })
 
-#define rdmsr_safe_regs(regs)	paravirt_rdmsr_regs(regs)
-#define wrmsr_safe_regs(regs)	paravirt_wrmsr_regs(regs)
-
 static inline int rdmsrl_safe(unsigned msr, unsigned long long *p)
 {
 	int err;
@@ -186,32 +173,6 @@ static inline int rdmsrl_safe(unsigned msr, unsigned long
long *p)
 	*p = paravirt_read_msr(msr, &err);
 	return err;
 }
-static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p)
-{
-	u32 gprs[8] = { 0 };
-	int err;
-
-	gprs[1] = msr;
-	gprs[7] = 0x9c5a203a;
-
-	err = paravirt_rdmsr_regs(gprs);
-
-	*p = gprs[0] | ((u64)gprs[2] << 32);
-
-	return err;
-}
-
-static inline int wrmsrl_amd_safe(unsigned msr, unsigned long long val)
-{
-	u32 gprs[8] = { 0 };
-
-	gprs[0] = (u32)val;
-	gprs[1] = msr;
-	gprs[2] = val >> 32;
-	gprs[7] = 0x9c5a203a;
-
-	return paravirt_wrmsr_regs(gprs);
-}
 
 static inline u64 paravirt_read_tsc(void)
 {
diff --git a/arch/x86/include/asm/paravirt_types.h
b/arch/x86/include/asm/paravirt_types.h
index 8e8b9a4987ee..8613cbb7ba41 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -153,9 +153,7 @@ struct pv_cpu_ops {
 	/* MSR, PMC and TSR operations.
 	   err = 0/-EFAULT.  wrmsr returns 0/-EFAULT. */
 	u64 (*read_msr)(unsigned int msr, int *err);
-	int (*rdmsr_regs)(u32 *regs);
 	int (*write_msr)(unsigned int msr, unsigned low, unsigned high);
-	int (*wrmsr_regs)(u32 *regs);
 
 	u64 (*read_tsc)(void);
 	u64 (*read_pmc)(int counter);
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 9ce885996fd7..17fff18a1031 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -352,9 +352,7 @@ struct pv_cpu_ops pv_cpu_ops = {
 #endif
 	.wbinvd = native_wbinvd,
 	.read_msr = native_read_msr_safe,
-	.rdmsr_regs = native_rdmsr_safe_regs,
 	.write_msr = native_write_msr_safe,
-	.wrmsr_regs = native_wrmsr_safe_regs,
 	.read_tsc = native_read_tsc,
 	.read_pmc = native_read_pmc,
 	.read_tscp = native_read_tscp,
diff --git a/arch/x86/lib/msr-reg-export.c b/arch/x86/lib/msr-reg-export.c
index a311cc59b65d..8d6ef78b5d01 100644
--- a/arch/x86/lib/msr-reg-export.c
+++ b/arch/x86/lib/msr-reg-export.c
@@ -1,5 +1,5 @@
 #include <linux/module.h>
 #include <asm/msr.h>
 
-EXPORT_SYMBOL(native_rdmsr_safe_regs);
-EXPORT_SYMBOL(native_wrmsr_safe_regs);
+EXPORT_SYMBOL(rdmsr_safe_regs);
+EXPORT_SYMBOL(wrmsr_safe_regs);
diff --git a/arch/x86/lib/msr-reg.S b/arch/x86/lib/msr-reg.S
index 69fa10623f21..f6d13eefad10 100644
--- a/arch/x86/lib/msr-reg.S
+++ b/arch/x86/lib/msr-reg.S
@@ -6,13 +6,13 @@
 
 #ifdef CONFIG_X86_64
 /*
- * int native_{rdmsr,wrmsr}_safe_regs(u32 gprs[8]);
+ * int {rdmsr,wrmsr}_safe_regs(u32 gprs[8]);
  *
  * reg layout: u32 gprs[eax, ecx, edx, ebx, esp, ebp, esi, edi]
  *
  */
 .macro op_safe_regs op
-ENTRY(native_\op\()_safe_regs)
+ENTRY(\op\()_safe_regs)
 	CFI_STARTPROC
 	pushq_cfi %rbx
 	pushq_cfi %rbp
@@ -45,13 +45,13 @@ ENTRY(native_\op\()_safe_regs)
 
 	_ASM_EXTABLE(1b, 3b)
 	CFI_ENDPROC
-ENDPROC(native_\op\()_safe_regs)
+ENDPROC(\op\()_safe_regs)
 .endm
 
 #else /* X86_32 */
 
 .macro op_safe_regs op
-ENTRY(native_\op\()_safe_regs)
+ENTRY(\op\()_safe_regs)
 	CFI_STARTPROC
 	pushl_cfi %ebx
 	pushl_cfi %ebp
@@ -92,7 +92,7 @@ ENTRY(native_\op\()_safe_regs)
 
 	_ASM_EXTABLE(1b, 3b)
 	CFI_ENDPROC
-ENDPROC(native_\op\()_safe_regs)
+ENDPROC(\op\()_safe_regs)
 .endm
 
 #endif
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index e74df9548a02..60f1131eb94f 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1116,9 +1116,7 @@ static const struct pv_cpu_ops xen_cpu_ops __initconst = {
 	.wbinvd = native_wbinvd,
 
 	.read_msr = native_read_msr_safe,
-	.rdmsr_regs = native_rdmsr_safe_regs,
 	.write_msr = xen_write_msr_safe,
-	.wrmsr_regs = native_wrmsr_safe_regs,
 
 	.read_tsc = native_read_tsc,
 	.read_pmc = native_read_pmc,
-- 
1.7.9.3.362.g71319

Borislav Petkov

2012-Jun-01 14:52 UTC

head link

[PATCH 2/4] x86, CPU: Fix show_msr MSR accessing function

From: Borislav Petkov <borislav.petkov@amd.com>

There''s no real reason why, when showing the MSRs on a CPU at boottime,
we should be using the AMD-specific variant. Simply use the generic safe
one which handles #GPs just fine.

Cc: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/common.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 82f29e70d058..232fba2d54c9 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -947,7 +947,7 @@ static void __cpuinit __print_cpu_msr(void)
 		index_max = msr_range_array[i].max;
 
 		for (index = index_min; index < index_max; index++) {
-			if (rdmsrl_amd_safe(index, &val))
+			if (rdmsrl_safe(index, &val))
 				continue;
 			printk(KERN_INFO " MSR%08x: %016llx\n", index, val);
 		}
-- 
1.7.9.3.362.g71319

Borislav Petkov

2012-Jun-01 14:52 UTC

head link

[PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

From: Andre Przywara <andre.przywara@amd.com>

f7f286a910221 ("x86/amd: Re-enable CPU topology extensions in case BIOS
has disabled it") wrongfully added code which used the AMD-specific
{rd,wr}msr variants for no real reason.

This caused boot panics on xen which wasn''t initializing the
{rd,wr}msr_safe_regs pv_ops members properly.

This, in turn, caused a heated discussion leading to us reviewing all
uses of the AMD-specific variants and removing them where unneeded
(almost everywhere except an obscure K8 BIOS fix, see 6b0f43ddfa358).

Finally, this patch switches to the standard {rd,wr}msr*_safe* variants
which should''ve been used in the first place anyway and avoided
unneeded
excitation with xen.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>
Cc: Andreas Herrmann <andreas.herrmann3@amd.com>
Cc: stable@vger.kernel.org # 3.4+
Link:
<http://lkml.kernel.org/r/1338383402-3838-1-git-send-email-andre.przywara@amd.com>
[Boris: correct and expand commit message]
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/amd.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 146bb6218eec..80ccd99542e6 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -586,9 +586,9 @@ static void __cpuinit init_amd(struct cpuinfo_x86 *c)
 	    !cpu_has(c, X86_FEATURE_TOPOEXT)) {
 		u64 val;
 
-		if (!rdmsrl_amd_safe(0xc0011005, &val)) {
+		if (!rdmsrl_safe(0xc0011005, &val)) {
 			val |= 1ULL << 54;
-			wrmsrl_amd_safe(0xc0011005, val);
+			checking_wrmsrl(0xc0011005, val);
 			rdmsrl(0xc0011005, val);
 			if (val & (1ULL << 54)) {
 				set_cpu_cap(c, X86_FEATURE_TOPOEXT);
-- 
1.7.9.3.362.g71319

Borislav Petkov

2012-Jun-01 14:52 UTC

head link

[PATCH 4/4] x86, CPU, AMD: Deprecate AMD-specific MSR variants

From: Borislav Petkov <borislav.petkov@amd.com>

Now that all users of {rd,wr}msr_amd_safe have been fixed, deprecate its
use by making them private to amd.c and adding warnings when used on
anything else beside K8.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/include/asm/msr.h |   27 ---------------------------
 arch/x86/kernel/cpu/amd.c  |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 81860cc012d1..cb33b5f00267 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -211,33 +211,6 @@ do {                                                       
\
 
 #endif	/* !CONFIG_PARAVIRT */
 
-static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p)
-{
-	u32 gprs[8] = { 0 };
-	int err;
-
-	gprs[1] = msr;
-	gprs[7] = 0x9c5a203a;
-
-	err = rdmsr_safe_regs(gprs);
-
-	*p = gprs[0] | ((u64)gprs[2] << 32);
-
-	return err;
-}
-
-static inline int wrmsrl_amd_safe(unsigned msr, unsigned long long val)
-{
-	u32 gprs[8] = { 0 };
-
-	gprs[0] = (u32)val;
-	gprs[1] = msr;
-	gprs[2] = val >> 32;
-	gprs[7] = 0x9c5a203a;
-
-	return wrmsr_safe_regs(gprs);
-}
-
 #define checking_wrmsrl(msr, val) wrmsr_safe((msr), (u32)(val),		\
 					     (u32)((val) >> 32))
 
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 80ccd99542e6..c928eb26ada6 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -19,6 +19,39 @@
 
 #include "cpu.h"
 
+static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p)
+{
+	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
+	u32 gprs[8] = { 0 };
+	int err;
+
+	WARN_ONCE((c->x86 != 0xf), "%s should only be used on K8!\n",
__func__);
+
+	gprs[1] = msr;
+	gprs[7] = 0x9c5a203a;
+
+	err = rdmsr_safe_regs(gprs);
+
+	*p = gprs[0] | ((u64)gprs[2] << 32);
+
+	return err;
+}
+
+static inline int wrmsrl_amd_safe(unsigned msr, unsigned long long val)
+{
+	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
+	u32 gprs[8] = { 0 };
+
+	WARN_ONCE((c->x86 != 0xf), "%s should only be used on K8!\n",
__func__);
+
+	gprs[0] = (u32)val;
+	gprs[1] = msr;
+	gprs[2] = val >> 32;
+	gprs[7] = 0x9c5a203a;
+
+	return wrmsr_safe_regs(gprs);
+}
+
 #ifdef CONFIG_X86_32
 /*
  *	B step AMD K6 before B 9730xxxx have hardware bugs that can cause
-- 
1.7.9.3.362.g71319

H. Peter Anvin

2012-Jun-01 14:53 UTC

head link

Re: [PATCH 0/4] x86, CPU, AMD: Cleanup AMD-specific MSR-rw users

On 06/01/2012 07:52 AM, Borislav Petkov wrote:> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> Ok,
> 
> the following patchset should take care of all the {rd,wr}msrl_amd_safe
> headaches we had this week. After it is applied, the AMD-specific
> variants become private to amd.c and issue a warning when used on
> anything else beside K8.
> 
> This also contains the two patches from Andre which cleanup the PV-side
> of things.
> 
Looks good.  This can be for 3.6, right (no immediate urgency)?

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don''t speak on their behalf.

Borislav Petkov

2012-Jun-01 14:57 UTC

head link

Re: [PATCH 0/4] x86, CPU, AMD: Cleanup AMD-specific MSR-rw users

On Fri, Jun 01, 2012 at 07:53:26AM -0700, H. Peter Anvin
wrote:> Looks good.  This can be for 3.6, right (no immediate urgency)?
None on arch/x86/ - simply correctness fixes.

And I''m assuming since you took Konrad''s patch, there are no
pending
issues for xen either.

So yeah, this can wait for 3.6, IMHO.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

H. Peter Anvin

2012-Jun-01 15:06 UTC

head link

Re: [PATCH 0/4] x86, CPU, AMD: Cleanup AMD-specific MSR-rw users

On 06/01/2012 07:57 AM, Borislav Petkov wrote:> On Fri, Jun 01, 2012 at 07:53:26AM -0700, H. Peter Anvin wrote:
>> Looks good.  This can be for 3.6, right (no immediate urgency)?
> 
> None on arch/x86/ - simply correctness fixes.
> 
> And I''m assuming since you took Konrad''s patch, there are
no pending
> issues for xen either.
> 
> So yeah, this can wait for 3.6, IMHO.
> 
Great!  Will queue up shortly.

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don''t speak on their behalf.

Yinghai Lu

2012-Jun-01 18:22 UTC

head link

Re: [PATCH 2/4] x86, CPU: Fix show_msr MSR accessing function

On Fri, Jun 1, 2012 at 7:52 AM, Borislav Petkov <bp@amd64.org>
wrote:> From: Borislav Petkov <borislav.petkov@amd.com>
>
> There''s no real reason why, when showing the MSRs on a CPU at
boottime,
> we should be using the AMD-specific variant. Simply use the generic safe
> one which handles #GPs just fine.
>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  arch/x86/kernel/cpu/common.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 82f29e70d058..232fba2d54c9 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -947,7 +947,7 @@ static void __cpuinit __print_cpu_msr(void)
>                index_max = msr_range_array[i].max;
>
>                for (index = index_min; index < index_max; index++) {
> -                       if (rdmsrl_amd_safe(index, &val))
> +                       if (rdmsrl_safe(index, &val))
>                                continue;
>                        printk(KERN_INFO " MSR%08x: %016llx\n",
index, val);
>                }
> --
can you double check if the range will need special passcode ?



        { 0x00000000, 0x00000418},
        { 0xc0000000, 0xc000040b},
        { 0xc0010000, 0xc0010142},
        { 0xc0011000, 0xc001103b},

passcode should be
       gprs[7] = 0x9c5a203a;

Thanks

Yinghai

Borislav Petkov

2012-Jun-01 22:50 UTC

head link

Re: [PATCH 2/4] x86, CPU: Fix show_msr MSR accessing function

On Fri, Jun 01, 2012 at 11:22:03AM -0700, Yinghai Lu
wrote:> can you double check if the range will need special passcode ?
> 
>         { 0x00000000, 0x00000418},
>         { 0xc0000000, 0xc000040b},
>         { 0xc0010000, 0xc0010142},
>         { 0xc0011000, 0xc001103b},
> 
> passcode should be
>        gprs[7] = 0x9c5a203a;
This currently reads the MSRs it can read without the passcode. There''s
no reason I''m aware of to access the other MSRs.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Konrad Rzeszutek Wilk

2012-Jun-06 20:06 UTC

head link

Re: [PATCH 1/4] x86, pvops: Remove hooks for {rd,wr}msr_safe_regs

On Fri, Jun 01, 2012 at 04:52:35PM +0200, Borislav Petkov
wrote:> From: Andre Przywara <andre.przywara@amd.com>
> 
> There were paravirt_ops hooks for the full register set variant of
> {rd,wr}msr_safe which are actually not used by anyone anymore. Remove
> them to make the code cleaner and avoid silent breakages when the pvops
> members were uninitialized. This has been boot-tested natively and under
> Xen with PVOPS enabled and disabled on one machine.
> 
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
Acked-by: Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com>> ---
>  arch/x86/include/asm/msr.h            |   67
++++++++++++++-------------------
>  arch/x86/include/asm/paravirt.h       |   39 -------------------
>  arch/x86/include/asm/paravirt_types.h |    2 -
>  arch/x86/kernel/paravirt.c            |    2 -
>  arch/x86/lib/msr-reg-export.c         |    4 +-
>  arch/x86/lib/msr-reg.S                |   10 ++---
>  arch/x86/xen/enlighten.c              |    2 -
>  7 files changed, 35 insertions(+), 91 deletions(-)
> 
> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> index 084ef95274cd..81860cc012d1 100644
> --- a/arch/x86/include/asm/msr.h
> +++ b/arch/x86/include/asm/msr.h
> @@ -115,8 +115,8 @@ notrace static inline int
native_write_msr_safe(unsigned int msr,
>  
>  extern unsigned long long native_read_tsc(void);
>  
> -extern int native_rdmsr_safe_regs(u32 regs[8]);
> -extern int native_wrmsr_safe_regs(u32 regs[8]);
> +extern int rdmsr_safe_regs(u32 regs[8]);
> +extern int wrmsr_safe_regs(u32 regs[8]);
>  
>  static __always_inline unsigned long long __native_read_tsc(void)
>  {
> @@ -187,43 +187,6 @@ static inline int rdmsrl_safe(unsigned msr, unsigned
long long *p)
>  	return err;
>  }
>  
> -static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p)
> -{
> -	u32 gprs[8] = { 0 };
> -	int err;
> -
> -	gprs[1] = msr;
> -	gprs[7] = 0x9c5a203a;
> -
> -	err = native_rdmsr_safe_regs(gprs);
> -
> -	*p = gprs[0] | ((u64)gprs[2] << 32);
> -
> -	return err;
> -}
> -
> -static inline int wrmsrl_amd_safe(unsigned msr, unsigned long long val)
> -{
> -	u32 gprs[8] = { 0 };
> -
> -	gprs[0] = (u32)val;
> -	gprs[1] = msr;
> -	gprs[2] = val >> 32;
> -	gprs[7] = 0x9c5a203a;
> -
> -	return native_wrmsr_safe_regs(gprs);
> -}
> -
> -static inline int rdmsr_safe_regs(u32 regs[8])
> -{
> -	return native_rdmsr_safe_regs(regs);
> -}
> -
> -static inline int wrmsr_safe_regs(u32 regs[8])
> -{
> -	return native_wrmsr_safe_regs(regs);
> -}
> -
>  #define rdtscl(low)						\
>  	((low) = (u32)__native_read_tsc())
>  
> @@ -248,6 +211,32 @@ do {                                                  
\
>  
>  #endif	/* !CONFIG_PARAVIRT */
>  
> +static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p)
> +{
> +	u32 gprs[8] = { 0 };
> +	int err;
> +
> +	gprs[1] = msr;
> +	gprs[7] = 0x9c5a203a;
> +
> +	err = rdmsr_safe_regs(gprs);
> +
> +	*p = gprs[0] | ((u64)gprs[2] << 32);
> +
> +	return err;
> +}
> +
> +static inline int wrmsrl_amd_safe(unsigned msr, unsigned long long val)
> +{
> +	u32 gprs[8] = { 0 };
> +
> +	gprs[0] = (u32)val;
> +	gprs[1] = msr;
> +	gprs[2] = val >> 32;
> +	gprs[7] = 0x9c5a203a;
> +
> +	return wrmsr_safe_regs(gprs);
> +}
>  
>  #define checking_wrmsrl(msr, val) wrmsr_safe((msr), (u32)(val),		\
>  					     (u32)((val) >> 32))
> diff --git a/arch/x86/include/asm/paravirt.h
b/arch/x86/include/asm/paravirt.h
> index 6cbbabf52707..ebb0cdb60a89 100644
> --- a/arch/x86/include/asm/paravirt.h
> +++ b/arch/x86/include/asm/paravirt.h
> @@ -128,21 +128,11 @@ static inline u64 paravirt_read_msr(unsigned msr, int
*err)
>  	return PVOP_CALL2(u64, pv_cpu_ops.read_msr, msr, err);
>  }
>  
> -static inline int paravirt_rdmsr_regs(u32 *regs)
> -{
> -	return PVOP_CALL1(int, pv_cpu_ops.rdmsr_regs, regs);
> -}
> -
>  static inline int paravirt_write_msr(unsigned msr, unsigned low, unsigned
high)
>  {
>  	return PVOP_CALL3(int, pv_cpu_ops.write_msr, msr, low, high);
>  }
>  
> -static inline int paravirt_wrmsr_regs(u32 *regs)
> -{
> -	return PVOP_CALL1(int, pv_cpu_ops.wrmsr_regs, regs);
> -}
> -
>  /* These should all do BUG_ON(_err), but our headers are too tangled. */
>  #define rdmsr(msr, val1, val2)			\
>  do {						\
> @@ -176,9 +166,6 @@ do {						\
>  	_err;					\
>  })
>  
> -#define rdmsr_safe_regs(regs)	paravirt_rdmsr_regs(regs)
> -#define wrmsr_safe_regs(regs)	paravirt_wrmsr_regs(regs)
> -
>  static inline int rdmsrl_safe(unsigned msr, unsigned long long *p)
>  {
>  	int err;
> @@ -186,32 +173,6 @@ static inline int rdmsrl_safe(unsigned msr, unsigned
long long *p)
>  	*p = paravirt_read_msr(msr, &err);
>  	return err;
>  }
> -static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p)
> -{
> -	u32 gprs[8] = { 0 };
> -	int err;
> -
> -	gprs[1] = msr;
> -	gprs[7] = 0x9c5a203a;
> -
> -	err = paravirt_rdmsr_regs(gprs);
> -
> -	*p = gprs[0] | ((u64)gprs[2] << 32);
> -
> -	return err;
> -}
> -
> -static inline int wrmsrl_amd_safe(unsigned msr, unsigned long long val)
> -{
> -	u32 gprs[8] = { 0 };
> -
> -	gprs[0] = (u32)val;
> -	gprs[1] = msr;
> -	gprs[2] = val >> 32;
> -	gprs[7] = 0x9c5a203a;
> -
> -	return paravirt_wrmsr_regs(gprs);
> -}
>  
>  static inline u64 paravirt_read_tsc(void)
>  {
> diff --git a/arch/x86/include/asm/paravirt_types.h
b/arch/x86/include/asm/paravirt_types.h
> index 8e8b9a4987ee..8613cbb7ba41 100644
> --- a/arch/x86/include/asm/paravirt_types.h
> +++ b/arch/x86/include/asm/paravirt_types.h
> @@ -153,9 +153,7 @@ struct pv_cpu_ops {
>  	/* MSR, PMC and TSR operations.
>  	   err = 0/-EFAULT.  wrmsr returns 0/-EFAULT. */
>  	u64 (*read_msr)(unsigned int msr, int *err);
> -	int (*rdmsr_regs)(u32 *regs);
>  	int (*write_msr)(unsigned int msr, unsigned low, unsigned high);
> -	int (*wrmsr_regs)(u32 *regs);
>  
>  	u64 (*read_tsc)(void);
>  	u64 (*read_pmc)(int counter);
> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
> index 9ce885996fd7..17fff18a1031 100644
> --- a/arch/x86/kernel/paravirt.c
> +++ b/arch/x86/kernel/paravirt.c
> @@ -352,9 +352,7 @@ struct pv_cpu_ops pv_cpu_ops = {
>  #endif
>  	.wbinvd = native_wbinvd,
>  	.read_msr = native_read_msr_safe,
> -	.rdmsr_regs = native_rdmsr_safe_regs,
>  	.write_msr = native_write_msr_safe,
> -	.wrmsr_regs = native_wrmsr_safe_regs,
>  	.read_tsc = native_read_tsc,
>  	.read_pmc = native_read_pmc,
>  	.read_tscp = native_read_tscp,
> diff --git a/arch/x86/lib/msr-reg-export.c b/arch/x86/lib/msr-reg-export.c
> index a311cc59b65d..8d6ef78b5d01 100644
> --- a/arch/x86/lib/msr-reg-export.c
> +++ b/arch/x86/lib/msr-reg-export.c
> @@ -1,5 +1,5 @@
>  #include <linux/module.h>
>  #include <asm/msr.h>
>  
> -EXPORT_SYMBOL(native_rdmsr_safe_regs);
> -EXPORT_SYMBOL(native_wrmsr_safe_regs);
> +EXPORT_SYMBOL(rdmsr_safe_regs);
> +EXPORT_SYMBOL(wrmsr_safe_regs);
> diff --git a/arch/x86/lib/msr-reg.S b/arch/x86/lib/msr-reg.S
> index 69fa10623f21..f6d13eefad10 100644
> --- a/arch/x86/lib/msr-reg.S
> +++ b/arch/x86/lib/msr-reg.S
> @@ -6,13 +6,13 @@
>  
>  #ifdef CONFIG_X86_64
>  /*
> - * int native_{rdmsr,wrmsr}_safe_regs(u32 gprs[8]);
> + * int {rdmsr,wrmsr}_safe_regs(u32 gprs[8]);
>   *
>   * reg layout: u32 gprs[eax, ecx, edx, ebx, esp, ebp, esi, edi]
>   *
>   */
>  .macro op_safe_regs op
> -ENTRY(native_\op\()_safe_regs)
> +ENTRY(\op\()_safe_regs)
>  	CFI_STARTPROC
>  	pushq_cfi %rbx
>  	pushq_cfi %rbp
> @@ -45,13 +45,13 @@ ENTRY(native_\op\()_safe_regs)
>  
>  	_ASM_EXTABLE(1b, 3b)
>  	CFI_ENDPROC
> -ENDPROC(native_\op\()_safe_regs)
> +ENDPROC(\op\()_safe_regs)
>  .endm
>  
>  #else /* X86_32 */
>  
>  .macro op_safe_regs op
> -ENTRY(native_\op\()_safe_regs)
> +ENTRY(\op\()_safe_regs)
>  	CFI_STARTPROC
>  	pushl_cfi %ebx
>  	pushl_cfi %ebp
> @@ -92,7 +92,7 @@ ENTRY(native_\op\()_safe_regs)
>  
>  	_ASM_EXTABLE(1b, 3b)
>  	CFI_ENDPROC
> -ENDPROC(native_\op\()_safe_regs)
> +ENDPROC(\op\()_safe_regs)
>  .endm
>  
>  #endif
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index e74df9548a02..60f1131eb94f 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -1116,9 +1116,7 @@ static const struct pv_cpu_ops xen_cpu_ops
__initconst = {
>  	.wbinvd = native_wbinvd,
>  
>  	.read_msr = native_read_msr_safe,
> -	.rdmsr_regs = native_rdmsr_safe_regs,
>  	.write_msr = xen_write_msr_safe,
> -	.wrmsr_regs = native_wrmsr_safe_regs,
>  
>  	.read_tsc = native_read_tsc,
>  	.read_pmc = native_read_pmc,
> -- 
> 1.7.9.3.362.g71319

Konrad Rzeszutek Wilk

2012-Jun-06 20:07 UTC

head link

Re: [PATCH 4/4] x86, CPU, AMD: Deprecate AMD-specific MSR variants

On Fri, Jun 01, 2012 at 04:52:38PM +0200, Borislav Petkov
wrote:> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> Now that all users of {rd,wr}msr_amd_safe have been fixed, deprecate its
> use by making them private to amd.c and adding warnings when used on
> anything else beside K8.
> 
Acked-by: Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com>> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  arch/x86/include/asm/msr.h |   27 ---------------------------
>  arch/x86/kernel/cpu/amd.c  |   33 +++++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> index 81860cc012d1..cb33b5f00267 100644
> --- a/arch/x86/include/asm/msr.h
> +++ b/arch/x86/include/asm/msr.h
> @@ -211,33 +211,6 @@ do {                                                  
\
>  
>  #endif	/* !CONFIG_PARAVIRT */
>  
> -static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p)
> -{
> -	u32 gprs[8] = { 0 };
> -	int err;
> -
> -	gprs[1] = msr;
> -	gprs[7] = 0x9c5a203a;
> -
> -	err = rdmsr_safe_regs(gprs);
> -
> -	*p = gprs[0] | ((u64)gprs[2] << 32);
> -
> -	return err;
> -}
> -
> -static inline int wrmsrl_amd_safe(unsigned msr, unsigned long long val)
> -{
> -	u32 gprs[8] = { 0 };
> -
> -	gprs[0] = (u32)val;
> -	gprs[1] = msr;
> -	gprs[2] = val >> 32;
> -	gprs[7] = 0x9c5a203a;
> -
> -	return wrmsr_safe_regs(gprs);
> -}
> -
>  #define checking_wrmsrl(msr, val) wrmsr_safe((msr), (u32)(val),		\
>  					     (u32)((val) >> 32))
>  
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 80ccd99542e6..c928eb26ada6 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -19,6 +19,39 @@
>  
>  #include "cpu.h"
>  
> +static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p)
> +{
> +	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
> +	u32 gprs[8] = { 0 };
> +	int err;
> +
> +	WARN_ONCE((c->x86 != 0xf), "%s should only be used on
K8!\n", __func__);
> +
> +	gprs[1] = msr;
> +	gprs[7] = 0x9c5a203a;
> +
> +	err = rdmsr_safe_regs(gprs);
> +
> +	*p = gprs[0] | ((u64)gprs[2] << 32);
> +
> +	return err;
> +}
> +
> +static inline int wrmsrl_amd_safe(unsigned msr, unsigned long long val)
> +{
> +	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
> +	u32 gprs[8] = { 0 };
> +
> +	WARN_ONCE((c->x86 != 0xf), "%s should only be used on
K8!\n", __func__);
> +
> +	gprs[0] = (u32)val;
> +	gprs[1] = msr;
> +	gprs[2] = val >> 32;
> +	gprs[7] = 0x9c5a203a;
> +
> +	return wrmsr_safe_regs(gprs);
> +}
> +
>  #ifdef CONFIG_X86_32
>  /*
>   *	B step AMD K6 before B 9730xxxx have hardware bugs that can cause
> -- 
> 1.7.9.3.362.g71319

Konrad Rzeszutek Wilk

2012-Jun-06 20:07 UTC

head link

Re: [PATCH 2/4] x86, CPU: Fix show_msr MSR accessing function

On Fri, Jun 01, 2012 at 04:52:36PM +0200, Borislav Petkov
wrote:> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> There''s no real reason why, when showing the MSRs on a CPU at
boottime,
> we should be using the AMD-specific variant. Simply use the generic safe
> one which handles #GPs just fine.
Acked-by: Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com>> 
> Cc: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  arch/x86/kernel/cpu/common.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 82f29e70d058..232fba2d54c9 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -947,7 +947,7 @@ static void __cpuinit __print_cpu_msr(void)
>  		index_max = msr_range_array[i].max;
>  
>  		for (index = index_min; index < index_max; index++) {
> -			if (rdmsrl_amd_safe(index, &val))
> +			if (rdmsrl_safe(index, &val))
>  				continue;
>  			printk(KERN_INFO " MSR%08x: %016llx\n", index, val);
>  		}
> -- 
> 1.7.9.3.362.g71319

Konrad Rzeszutek Wilk

2012-Jun-06 20:07 UTC

head link

Re: [PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

On Fri, Jun 01, 2012 at 04:52:37PM +0200, Borislav Petkov
wrote:> From: Andre Przywara <andre.przywara@amd.com>
> 
> f7f286a910221 ("x86/amd: Re-enable CPU topology extensions in case
BIOS
> has disabled it") wrongfully added code which used the AMD-specific
> {rd,wr}msr variants for no real reason.
> 
> This caused boot panics on xen which wasn''t initializing the
> {rd,wr}msr_safe_regs pv_ops members properly.
> 
> This, in turn, caused a heated discussion leading to us reviewing all
> uses of the AMD-specific variants and removing them where unneeded
> (almost everywhere except an obscure K8 BIOS fix, see 6b0f43ddfa358).
> 
> Finally, this patch switches to the standard {rd,wr}msr*_safe* variants
> which should''ve been used in the first place anyway and avoided
unneeded
> excitation with xen.
> 
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
> Cc: Andreas Herrmann <andreas.herrmann3@amd.com>
Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: stable@vger.kernel.org # 3.4+
> Link:
<http://lkml.kernel.org/r/1338383402-3838-1-git-send-email-andre.przywara@amd.com>
> [Boris: correct and expand commit message]
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  arch/x86/kernel/cpu/amd.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 146bb6218eec..80ccd99542e6 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -586,9 +586,9 @@ static void __cpuinit init_amd(struct cpuinfo_x86 *c)
>  	    !cpu_has(c, X86_FEATURE_TOPOEXT)) {
>  		u64 val;
>  
> -		if (!rdmsrl_amd_safe(0xc0011005, &val)) {
> +		if (!rdmsrl_safe(0xc0011005, &val)) {
>  			val |= 1ULL << 54;
> -			wrmsrl_amd_safe(0xc0011005, val);
> +			checking_wrmsrl(0xc0011005, val);
>  			rdmsrl(0xc0011005, val);
>  			if (val & (1ULL << 54)) {
>  				set_cpu_cap(c, X86_FEATURE_TOPOEXT);
> -- 
> 1.7.9.3.362.g71319

H. Peter Anvin

2012-Jun-06 22:00 UTC

head link

Re: [PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

On 06/01/2012 07:52 AM, Borislav Petkov wrote:> From: Andre Przywara <andre.przywara@amd.com>
> 
> f7f286a910221 ("x86/amd: Re-enable CPU topology extensions in case
BIOS
> has disabled it") wrongfully added code which used the AMD-specific
> {rd,wr}msr variants for no real reason.
> 
> This caused boot panics on xen which wasn''t initializing the
> {rd,wr}msr_safe_regs pv_ops members properly.
> 
> This, in turn, caused a heated discussion leading to us reviewing all
> uses of the AMD-specific variants and removing them where unneeded
> (almost everywhere except an obscure K8 BIOS fix, see 6b0f43ddfa358).
> 
> Finally, this patch switches to the standard {rd,wr}msr*_safe* variants
> which should''ve been used in the first place anyway and avoided
unneeded
> excitation with xen.
> 
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
> Cc: Andreas Herrmann <andreas.herrmann3@amd.com>
> Cc: stable@vger.kernel.org # 3.4+
> Link:
<http://lkml.kernel.org/r/1338383402-3838-1-git-send-email-andre.przywara@amd.com>
> [Boris: correct and expand commit message]
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
Why -stable?  I though we had agreed that we didn''t have an active
problem (unclean hack, yes, but not an active problem) in 3.4/3.5 as it
currently sits?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don''t speak on their behalf.

Borislav Petkov

2012-Jun-07 07:21 UTC

head link

Re: [PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

On Wed, Jun 06, 2012 at 03:00:14PM -0700, H. Peter Anvin
wrote:> On 06/01/2012 07:52 AM, Borislav Petkov wrote:
> > From: Andre Przywara <andre.przywara@amd.com>
> > 
> > f7f286a910221 ("x86/amd: Re-enable CPU topology extensions in
case BIOS
> > has disabled it") wrongfully added code which used the
AMD-specific
> > {rd,wr}msr variants for no real reason.
> > 
> > This caused boot panics on xen which wasn''t initializing the
> > {rd,wr}msr_safe_regs pv_ops members properly.
> > 
> > This, in turn, caused a heated discussion leading to us reviewing all
> > uses of the AMD-specific variants and removing them where unneeded
> > (almost everywhere except an obscure K8 BIOS fix, see 6b0f43ddfa358).
> > 
> > Finally, this patch switches to the standard {rd,wr}msr*_safe*
variants
> > which should''ve been used in the first place anyway and
avoided unneeded
> > excitation with xen.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@amd.com>
> > Cc: Andreas Herrmann <andreas.herrmann3@amd.com>
> > Cc: stable@vger.kernel.org # 3.4+
> > Link:
<http://lkml.kernel.org/r/1338383402-3838-1-git-send-email-andre.przywara@amd.com>
> > [Boris: correct and expand commit message]
> > Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> 
> Why -stable?  I though we had agreed that we didn''t have an active
> problem (unclean hack, yes, but not an active problem) in 3.4/3.5 as it
> currently sits?
Yes, AFAICT, we need at least one fix for 3.4 where the original patch
f7f286a910221 broke xen.

So either this one or 1ab46fd319bc should be backported to stable, if
I''m not mistaken. If the second, I''ll drop the stable tag from
this one
and resend.

Konrad, what do you want wrt xen paravirt nullptr breakage for
3.4-stable?

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Andre Przywara

2012-Jun-07 07:49 UTC

head link

Re: [PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

On 06/07/2012 09:21 AM, Borislav Petkov wrote:> On Wed, Jun 06, 2012 at 03:00:14PM -0700, H. Peter Anvin wrote:
>> On 06/01/2012 07:52 AM, Borislav Petkov wrote:
>>> From: Andre Przywara<andre.przywara@amd.com>
>>>
>>> f7f286a910221 ("x86/amd: Re-enable CPU topology extensions in
case BIOS
>>> has disabled it") wrongfully added code which used the
AMD-specific
>>> {rd,wr}msr variants for no real reason.
>>>
>>> This caused boot panics on xen which wasn''t initializing
the
>>> {rd,wr}msr_safe_regs pv_ops members properly.
>>>
>>> This, in turn, caused a heated discussion leading to us reviewing
all
>>> uses of the AMD-specific variants and removing them where unneeded
>>> (almost everywhere except an obscure K8 BIOS fix, see
6b0f43ddfa358).
>>>
>>> Finally, this patch switches to the standard {rd,wr}msr*_safe*
variants
>>> which should''ve been used in the first place anyway and
avoided unneeded
>>> excitation with xen.
>>>
>>> Signed-off-by: Andre Przywara<andre.przywara@amd.com>
>>> Cc: Andreas Herrmann<andreas.herrmann3@amd.com>
>>> Cc: stable@vger.kernel.org # 3.4+
>>>
Link:<http://lkml.kernel.org/r/1338383402-3838-1-git-send-email-andre.przywara@amd.com>
>>> [Boris: correct and expand commit message]
>>> Signed-off-by: Borislav Petkov<borislav.petkov@amd.com>
>>
>> Why -stable?  I though we had agreed that we didn''t have an
active
>> problem (unclean hack, yes, but not an active problem) in 3.4/3.5 as it
>> currently sits?
This is probably a leftover from the original patch, before Konrad''s 
patch got committed.
> Yes, AFAICT, we need at least one fix for 3.4 where the original patch
> f7f286a910221 broke xen.
>
> So either this one or 1ab46fd319bc should be backported to stable, if
> I''m not mistaken. If the second, I''ll drop the stable tag
from this one
> and resend.
>
> Konrad, what do you want wrt xen paravirt nullptr breakage for
> 3.4-stable?
Greg just sent out the review mail for 1ab46fd319bc, so we can drop the 
stable tag from this one.

Regards,
Andre.

-------- Original Message --------
Subject: [ 21/82] x86, amd, xen: Avoid NULL pointer paravirt references
Date: Thu, 7 Jun 2012 13:03:57 +0900
From: Greg KH <gregkh@linuxfoundation.org>
To: <linux-kernel@vger.kernel.org>, <stable@vger.kernel.org>
CC: <torvalds@linux-foundation.org>, <akpm@linux-foundation.org>, 
<alan@lxorguk.ukuu.org.uk>, Andre Przywara <andre.przywara@amd.com>,
"H.
Peter Anvin" <hpa@zytor.com>

3.4-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Konrad Rzeszutek Wilk <konrad@darnok.org>

commit 1ab46fd319bcf1fcd9fb6311727d532b580e4eba upstream.

Stub out MSR methods that aren''t actually needed.  This fixes a crash
as Xen Dom0 on AMD Trinity systems.  A bigger patch should be added to
remove the paravirt machinery completely for the methods which
apparently have no users!
.....



-- 
Andre Przywara
AMD-OSRC (Dresden)
Tel: x29712

Greg KH

2012-Jun-07 08:08 UTC

head link

Re: [PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

On Thu, Jun 07, 2012 at 09:49:01AM +0200, Andre Przywara
wrote:> On 06/07/2012 09:21 AM, Borislav Petkov wrote:
> >On Wed, Jun 06, 2012 at 03:00:14PM -0700, H. Peter Anvin wrote:
> >>On 06/01/2012 07:52 AM, Borislav Petkov wrote:
> >>>From: Andre Przywara<andre.przywara@amd.com>
> >>>
> >>>f7f286a910221 ("x86/amd: Re-enable CPU topology extensions
in case BIOS
> >>>has disabled it") wrongfully added code which used the
AMD-specific
> >>>{rd,wr}msr variants for no real reason.
> >>>
> >>>This caused boot panics on xen which wasn''t
initializing the
> >>>{rd,wr}msr_safe_regs pv_ops members properly.
> >>>
> >>>This, in turn, caused a heated discussion leading to us
reviewing all
> >>>uses of the AMD-specific variants and removing them where
unneeded
> >>>(almost everywhere except an obscure K8 BIOS fix, see
6b0f43ddfa358).
> >>>
> >>>Finally, this patch switches to the standard {rd,wr}msr*_safe*
variants
> >>>which should''ve been used in the first place anyway
and avoided unneeded
> >>>excitation with xen.
> >>>
> >>>Signed-off-by: Andre Przywara<andre.przywara@amd.com>
> >>>Cc: Andreas Herrmann<andreas.herrmann3@amd.com>
> >>>Cc: stable@vger.kernel.org # 3.4+
>
>>>Link:<http://lkml.kernel.org/r/1338383402-3838-1-git-send-email-andre.przywara@amd.com>
> >>>[Boris: correct and expand commit message]
> >>>Signed-off-by: Borislav Petkov<borislav.petkov@amd.com>
> >>
> >>Why -stable?  I though we had agreed that we didn''t have
an active
> >>problem (unclean hack, yes, but not an active problem) in 3.4/3.5
as it
> >>currently sits?
> 
> This is probably a leftover from the original patch, before
Konrad''s
> patch got committed.
> 
> >Yes, AFAICT, we need at least one fix for 3.4 where the original patch
> >f7f286a910221 broke xen.
> >
> >So either this one or 1ab46fd319bc should be backported to stable, if
> >I''m not mistaken. If the second, I''ll drop the stable
tag from this one
> >and resend.
> >
> >Konrad, what do you want wrt xen paravirt nullptr breakage for
> >3.4-stable?
> 
> Greg just sent out the review mail for 1ab46fd319bc, so we can drop
> the stable tag from this one.
> 
> Regards,
> Andre.
What?  So I don''t need to do anything?

Totally confused,

greg k-h

Andre Przywara

2012-Jun-07 08:18 UTC

head link

Re: [PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

On 06/07/2012 10:08 AM, Greg KH wrote:> On Thu, Jun 07, 2012 at 09:49:01AM +0200, Andre Przywara wrote:
>> On 06/07/2012 09:21 AM, Borislav Petkov wrote:
>>> On Wed, Jun 06, 2012 at 03:00:14PM -0700, H. Peter Anvin wrote:
>>>> On 06/01/2012 07:52 AM, Borislav Petkov wrote:
>>>>> From: Andre Przywara<andre.przywara@amd.com>
>>>>>
>>>>> f7f286a910221 ("x86/amd: Re-enable CPU topology
extensions in case BIOS
>>>>> has disabled it") wrongfully added code which used the
AMD-specific
>>>>> {rd,wr}msr variants for no real reason.
>>>>>
>>>>> This caused boot panics on xen which wasn''t
initializing the
>>>>> {rd,wr}msr_safe_regs pv_ops members properly.
>>>>>
>>>>> This, in turn, caused a heated discussion leading to us
reviewing all
>>>>> uses of the AMD-specific variants and removing them where
unneeded
>>>>> (almost everywhere except an obscure K8 BIOS fix, see
6b0f43ddfa358).
>>>>>
>>>>> Finally, this patch switches to the standard
{rd,wr}msr*_safe* variants
>>>>> which should''ve been used in the first place
anyway and avoided unneeded
>>>>> excitation with xen.
>>>>>
>>>>> Signed-off-by: Andre Przywara<andre.przywara@amd.com>
>>>>> Cc: Andreas Herrmann<andreas.herrmann3@amd.com>
>>>>> Cc: stable@vger.kernel.org # 3.4+
>>>>>
Link:<http://lkml.kernel.org/r/1338383402-3838-1-git-send-email-andre.przywara@amd.com>
>>>>> [Boris: correct and expand commit message]
>>>>> Signed-off-by: Borislav
Petkov<borislav.petkov@amd.com>
>>>>
>>>> Why -stable?  I though we had agreed that we didn''t
have an active
>>>> problem (unclean hack, yes, but not an active problem) in
3.4/3.5 as it
>>>> currently sits?
>>
>> This is probably a leftover from the original patch, before
Konrad''s
>> patch got committed.
>>
>>> Yes, AFAICT, we need at least one fix for 3.4 where the original
patch
>>> f7f286a910221 broke xen.
>>>
>>> So either this one or 1ab46fd319bc should be backported to stable,
if
>>> I''m not mistaken. If the second, I''ll drop the
stable tag from this one
>>> and resend.
>>>
>>> Konrad, what do you want wrt xen paravirt nullptr breakage for
>>> 3.4-stable?
>>
>> Greg just sent out the review mail for 1ab46fd319bc, so we can drop
>> the stable tag from this one.
>>
>> Regards,
>> Andre.
>
> What?  So I don''t need to do anything?
>
> Totally confused,
Sorry for that.

To fix the issue, we need only one of those patches.
Mine ("Fix crash as Xen Dom0 on AMD Trinity systems", removing
"_amd"
from the rd/wrmsr calls) was sent out earlier, so I added the stable tag.
A bit later Konrad sent his patch (1ab46fd319bc, initializing the 
forgotten PVOPS members), which hpa took (dropping mine). So this fix is 
in 3.5-rc1 and should also be in -stable.
Now for making things smoother Boris sent out mine again - for 3.6 - and 
more or less accidentally kept the stable tag in it.

Long story short: everything is fine, just apply the one you sent the 
review message for.

HTH,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany

Borislav Petkov

2012-Jun-07 13:25 UTC

head link

[GIT PULL] x86, CPU, AMD: Cleanup AMD-specific MSR-rw users

Hi Peter,

so here are the final versions of the patches, as discussed. 3/4 has
lost the stable tag and all have received Konrad''s Acked-by. Other than
that, 1ab46fd319bc takes care of the -stable issue for xen and Greg is
picking that one up. So all those should be queued for 3.6.

Please pull, thanks.

The following changes since commit f8f5701bdaf9134b1f90e5044a82c66324d2073f:

  Linux 3.5-rc1 (2012-06-02 18:29:26 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git
tags/amd-rdmsr-cleanups-for-3.6

for you to fetch changes up to c5214c0192813ebaa5784be36d2ae142034f84db:

  x86, CPU, AMD: Deprecate AMD-specific MSR variants (2012-06-07 12:52:58 +0200)

----------------------------------------------------------------
x86, CPU, AMD: Cleanup AMD-specific MSR-rw users

This patchset takes care of all the {rd,wr}msrl_amd_safe headaches we
had wrt xen. After it is applied, the AMD-specific variants become
private to amd.c and issue a warning when used on anything else beside
K8 because they''re supposed to be used only on K8.

This also contains the two patches from Andre which cleanup the PV-side
of things.

----------------------------------------------------------------
Andre Przywara (2):
      x86, pvops: Remove hooks for {rd,wr}msr_safe_regs
      x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

Borislav Petkov (2):
      x86, CPU: Fix show_msr MSR accessing function
      x86, CPU, AMD: Deprecate AMD-specific MSR variants

 arch/x86/include/asm/msr.h            | 42 ++---------------------------------
 arch/x86/include/asm/paravirt.h       | 39 --------------------------------
 arch/x86/include/asm/paravirt_types.h |  2 --
 arch/x86/kernel/cpu/amd.c             | 37 ++++++++++++++++++++++++++++--
 arch/x86/kernel/cpu/common.c          |  2 +-
 arch/x86/kernel/paravirt.c            |  2 --
 arch/x86/lib/msr-reg-export.c         |  4 ++--
 arch/x86/lib/msr-reg.S                | 10 ++++-----
 arch/x86/xen/enlighten.c              |  2 --
 9 files changed, 45 insertions(+), 95 deletions(-)

-- 
Regards/Gruss,
Boris.

osrc-kernel@elbe.amd.com - where all your Linux questions get answered.

Operating Systems Research Center
Advanced Micro Devices, Inc.

H. Peter Anvin

2012-Jun-07 16:27 UTC

head link

Re: [PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

On 06/07/2012 01:08 AM, Greg KH wrote:> 
> What?  So I don''t need to do anything?
> 
Correct, you''re fine as-is.
	
	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don''t speak on their behalf.

Xen devel - Jun 2012 - [PATCH 0/4] x86, CPU, AMD: Cleanup AMD-specific MSR-rw users

[PATCH 0/4] x86, CPU, AMD: Cleanup AMD-specific MSR-rw users

[PATCH 1/4] x86, pvops: Remove hooks for {rd, wr}msr_safe_regs

[PATCH 2/4] x86, CPU: Fix show_msr MSR accessing function

[PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

[PATCH 4/4] x86, CPU, AMD: Deprecate AMD-specific MSR variants

Re: [PATCH 0/4] x86, CPU, AMD: Cleanup AMD-specific MSR-rw users

Re: [PATCH 0/4] x86, CPU, AMD: Cleanup AMD-specific MSR-rw users

Re: [PATCH 0/4] x86, CPU, AMD: Cleanup AMD-specific MSR-rw users

Re: [PATCH 2/4] x86, CPU: Fix show_msr MSR accessing function

Re: [PATCH 2/4] x86, CPU: Fix show_msr MSR accessing function

Re: [PATCH 1/4] x86, pvops: Remove hooks for {rd,wr}msr_safe_regs

Re: [PATCH 4/4] x86, CPU, AMD: Deprecate AMD-specific MSR variants

Re: [PATCH 2/4] x86, CPU: Fix show_msr MSR accessing function

Re: [PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

Re: [PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

Re: [PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

Re: [PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

Re: [PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

Re: [PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems

[GIT PULL] x86, CPU, AMD: Cleanup AMD-specific MSR-rw users

Re: [PATCH 3/4] x86, AMD: Fix crash as Xen Dom0 on AMD Trinity systems