Re: [Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup
On 8/19/19 7:28 PM, Andrew Cooper wrote: > On 19/08/2019 14:56, Michał Kowalczyk wrote: >> On 8/19/19 3:52 PM, Andrew Cooper wrote: >>> On 19/08/2019 14:50, Michał Kowalczyk wrote: On 8/19/19 11:04 AM, Andrew Cooper wrote: > On 19/08/2019 03:23, Michał Kowalczyk wrote: >> diff --git a/xen/arch/x86/boot/trampoline.S >> b/xen/arch/x86/boot/trampoline.S >> index 7c6a2328d2..fcaa3eeaf1 100644 >> --- a/xen/arch/x86/boot/trampoline.S >> +++ b/xen/arch/x86/boot/trampoline.S >> @@ -85,7 +85,7 @@ trampoline_gdt: >> .long trampoline_gdt + BOOT_PSEUDORM_DS + 2 - . >> .popsection >> >> -GLOBAL(trampoline_misc_enable_off) >> +GLOBAL(misc_enable_off) > The overall change is fine, but why have you renamed this variable? The old name had "trampoline_" prefix because the only place where it was used was trampoline_protmode_entry in arch/x86/boot/trampoline.S. Now it's also used in the wakeup code, so I removed the prefix which could be (IMO) misleading. > Without the rename, the patch would be just the single hunk in wakeup.S > and therefore easier to backport. True. Anyway, the decision is on your side, I can leave the old name if you prefer. >>> The trampoline_ prefix indicates where the data lives, which is in the >>> 16 bit trampoline which contains both the AP boot path, and wakeup path. >> Ah, if this is the convention you use then we should leave the old name. >>> If you're happy with this, I can adjust on commit to avoid you sending a >>> second time. >> Would be great, thanks! > Done. > > https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c3cfa5b3084d71bccd8360d044bea813688b587c Looks good. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup
On 19/08/2019 14:56, Michał Kowalczyk wrote: > On 8/19/19 3:52 PM, Andrew Cooper wrote: >> On 19/08/2019 14:50, Michał Kowalczyk wrote: >>> On 8/19/19 11:04 AM, Andrew Cooper wrote: On 19/08/2019 03:23, Michał Kowalczyk wrote: > diff --git a/xen/arch/x86/boot/trampoline.S > b/xen/arch/x86/boot/trampoline.S > index 7c6a2328d2..fcaa3eeaf1 100644 > --- a/xen/arch/x86/boot/trampoline.S > +++ b/xen/arch/x86/boot/trampoline.S > @@ -85,7 +85,7 @@ trampoline_gdt: > .long trampoline_gdt + BOOT_PSEUDORM_DS + 2 - . > .popsection > > -GLOBAL(trampoline_misc_enable_off) > +GLOBAL(misc_enable_off) The overall change is fine, but why have you renamed this variable? >>> The old name had "trampoline_" prefix because the only place where it >>> was used was trampoline_protmode_entry in arch/x86/boot/trampoline.S. >>> Now it's also used in the wakeup code, so I removed the prefix which >>> could be (IMO) misleading. Without the rename, the patch would be just the single hunk in wakeup.S and therefore easier to backport. >>> True. Anyway, the decision is on your side, I can leave the old name if >>> you prefer. >> The trampoline_ prefix indicates where the data lives, which is in the >> 16 bit trampoline which contains both the AP boot path, and wakeup path. > Ah, if this is the convention you use then we should leave the old name. >> If you're happy with this, I can adjust on commit to avoid you sending a >> second time. > Would be great, thanks! Done. https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c3cfa5b3084d71bccd8360d044bea813688b587c ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup
On 8/19/19 3:52 PM, Andrew Cooper wrote: > On 19/08/2019 14:50, Michał Kowalczyk wrote: >> On 8/19/19 11:04 AM, Andrew Cooper wrote: >>> On 19/08/2019 03:23, Michał Kowalczyk wrote: diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S index 7c6a2328d2..fcaa3eeaf1 100644 --- a/xen/arch/x86/boot/trampoline.S +++ b/xen/arch/x86/boot/trampoline.S @@ -85,7 +85,7 @@ trampoline_gdt: .long trampoline_gdt + BOOT_PSEUDORM_DS + 2 - . .popsection -GLOBAL(trampoline_misc_enable_off) +GLOBAL(misc_enable_off) >>> The overall change is fine, but why have you renamed this variable? >> The old name had "trampoline_" prefix because the only place where it >> was used was trampoline_protmode_entry in arch/x86/boot/trampoline.S. >> Now it's also used in the wakeup code, so I removed the prefix which >> could be (IMO) misleading. >>> Without the rename, the patch would be just the single hunk in wakeup.S >>> and therefore easier to backport. >> True. Anyway, the decision is on your side, I can leave the old name if >> you prefer. > The trampoline_ prefix indicates where the data lives, which is in the > 16 bit trampoline which contains both the AP boot path, and wakeup path. Ah, if this is the convention you use then we should leave the old name. > If you're happy with this, I can adjust on commit to avoid you sending a > second time. Would be great, thanks! ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup
On 19/08/2019 14:50, Michał Kowalczyk wrote: > On 8/19/19 11:04 AM, Andrew Cooper wrote: >> On 19/08/2019 03:23, Michał Kowalczyk wrote: >>> diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S >>> index 7c6a2328d2..fcaa3eeaf1 100644 >>> --- a/xen/arch/x86/boot/trampoline.S >>> +++ b/xen/arch/x86/boot/trampoline.S >>> @@ -85,7 +85,7 @@ trampoline_gdt: >>> .long trampoline_gdt + BOOT_PSEUDORM_DS + 2 - . >>> .popsection >>> >>> -GLOBAL(trampoline_misc_enable_off) >>> +GLOBAL(misc_enable_off) >> The overall change is fine, but why have you renamed this variable? > The old name had "trampoline_" prefix because the only place where it > was used was trampoline_protmode_entry in arch/x86/boot/trampoline.S. > Now it's also used in the wakeup code, so I removed the prefix which > could be (IMO) misleading. >> Without the rename, the patch would be just the single hunk in wakeup.S >> and therefore easier to backport. > True. Anyway, the decision is on your side, I can leave the old name if > you prefer. The trampoline_ prefix indicates where the data lives, which is in the 16 bit trampoline which contains both the AP boot path, and wakeup path. If you're happy with this, I can adjust on commit to avoid you sending a second time. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup
On 8/19/19 11:04 AM, Andrew Cooper wrote: > On 19/08/2019 03:23, Michał Kowalczyk wrote: >> diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S >> index 7c6a2328d2..fcaa3eeaf1 100644 >> --- a/xen/arch/x86/boot/trampoline.S >> +++ b/xen/arch/x86/boot/trampoline.S >> @@ -85,7 +85,7 @@ trampoline_gdt: >> .long trampoline_gdt + BOOT_PSEUDORM_DS + 2 - . >> .popsection >> >> -GLOBAL(trampoline_misc_enable_off) >> +GLOBAL(misc_enable_off) > The overall change is fine, but why have you renamed this variable? The old name had "trampoline_" prefix because the only place where it was used was trampoline_protmode_entry in arch/x86/boot/trampoline.S. Now it's also used in the wakeup code, so I removed the prefix which could be (IMO) misleading. > Without the rename, the patch would be just the single hunk in wakeup.S > and therefore easier to backport. True. Anyway, the decision is on your side, I can leave the old name if you prefer. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup
On 19/08/2019 03:23, Michał Kowalczyk wrote: > diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S > index 7c6a2328d2..fcaa3eeaf1 100644 > --- a/xen/arch/x86/boot/trampoline.S > +++ b/xen/arch/x86/boot/trampoline.S > @@ -85,7 +85,7 @@ trampoline_gdt: > .long trampoline_gdt + BOOT_PSEUDORM_DS + 2 - . > .popsection > > -GLOBAL(trampoline_misc_enable_off) > +GLOBAL(misc_enable_off) The overall change is fine, but why have you renamed this variable? Without the rename, the patch would be just the single hunk in wakeup.S and therefore easier to backport. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup
Code in intel.c:early_init_intel() modifies IA32_MISC_ENABLE MSR. Those
modifications must be restored after resuming from S3 (see e.g. Linux wakeup
code), otherwise bad things may happen (e.g. wakeup code may cause #GP when
trying to set IA32_EFER.NXE [1]).
This bug was noticed on a ThinkPad x230 with NX disabled in the BIOS:
Xen could correctly boot, but crashed when resuming from suspend.
Applying this patch fixed the problem.
[1] Intel SDM vol 3: "If the execute-disable capability is not
available, a write to set IA32_EFER.NXE produces a #GP exception."
Signed-off-by: Michał Kowalczyk
---
xen/arch/x86/boot/trampoline.S | 6 +++---
xen/arch/x86/boot/wakeup.S | 15 +++
xen/arch/x86/cpu/intel.c| 2 +-
xen/include/asm-x86/processor.h | 2 +-
4 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 7c6a2328d2..fcaa3eeaf1 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -85,7 +85,7 @@ trampoline_gdt:
.long trampoline_gdt + BOOT_PSEUDORM_DS + 2 - .
.popsection
-GLOBAL(trampoline_misc_enable_off)
+GLOBAL(misc_enable_off)
.quad 0
GLOBAL(cpuid_ext_features)
@@ -117,8 +117,8 @@ trampoline_protmode_entry:
mov %eax,%cr3
/* Adjust IA32_MISC_ENABLE if needed (for NX enabling below). */
-mov bootsym_rel(trampoline_misc_enable_off,4,%esi)
-mov bootsym_rel(trampoline_misc_enable_off+4,4,%edi)
+mov bootsym_rel(misc_enable_off,4,%esi)
+mov bootsym_rel(misc_enable_off+4,4,%edi)
mov %esi,%eax
or %edi,%eax
jz 1f
diff --git a/xen/arch/x86/boot/wakeup.S b/xen/arch/x86/boot/wakeup.S
index e3cb9e033a..b5f825e983 100644
--- a/xen/arch/x86/boot/wakeup.S
+++ b/xen/arch/x86/boot/wakeup.S
@@ -138,6 +138,21 @@ wakeup_32:
add bootsym_rel(trampoline_xen_phys_start,4,%eax)
mov %eax,%cr3
+/* Reapply IA32_MISC_ENABLE modifications from early_init_intel(). */
+mov bootsym_rel(misc_enable_off, 4, %esi)
+mov bootsym_rel(misc_enable_off+4, 4, %edi)
+mov %esi, %eax
+or %edi, %eax
+jz 1f
+mov $MSR_IA32_MISC_ENABLE, %ecx
+rdmsr
+not %esi
+not %edi
+and %esi, %eax
+and %edi, %edx
+wrmsr
+1:
+
/* Will cpuid feature change after resume? */
/* Set up EFER (Extended Feature Enable Register). */
mov bootsym_rel(cpuid_ext_features,4,%edi)
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index 5356a6ae10..a01e519281 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -269,7 +269,7 @@ static void early_init_intel(struct cpuinfo_x86 *c)
MSR_IA32_MISC_ENABLE_XD_DISABLE);
if (disable) {
wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable & ~disable);
- bootsym(trampoline_misc_enable_off) |= disable;
+ bootsym(misc_enable_off) |= disable;
}
if (disable & MSR_IA32_MISC_ENABLE_LIMIT_CPUID)
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 2862321eee..b325e4b0df 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -152,7 +152,7 @@ extern void (*ctxt_switch_masking)(const struct vcpu *next);
extern bool_t opt_cpu_info;
extern u32 cpuid_ext_features;
-extern u64 trampoline_misc_enable_off;
+extern u64 misc_enable_off;
/* Maximum width of physical addresses supported by the hardware. */
extern unsigned int paddr_bits;
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
