* [Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup
@ 2019-08-19 2:23 Michał Kowalczyk
2019-08-19 9:04 ` Andrew Cooper
0 siblings, 1 reply; 7+ messages in thread
From: Michał Kowalczyk @ 2019-08-19 2:23 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, Michał Kowalczyk, Andrew Cooper,
Marek Marczykowski-Górecki, Jan Beulich,
Roger Pau Monné
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 <mkow@invisiblethingslab.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup
2019-08-19 2:23 [Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup Michał Kowalczyk
@ 2019-08-19 9:04 ` Andrew Cooper
2019-08-19 13:50 ` Michał Kowalczyk
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2019-08-19 9:04 UTC (permalink / raw)
To: Michał Kowalczyk, xen-devel
Cc: Wei Liu, Marek Marczykowski-Górecki, Jan Beulich,
Roger Pau Monné
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup
2019-08-19 9:04 ` Andrew Cooper
@ 2019-08-19 13:50 ` Michał Kowalczyk
2019-08-19 13:52 ` Andrew Cooper
0 siblings, 1 reply; 7+ messages in thread
From: Michał Kowalczyk @ 2019-08-19 13:50 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: Wei Liu, Marek Marczykowski-Górecki, Jan Beulich,
Roger Pau Monné
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup
2019-08-19 13:50 ` Michał Kowalczyk
@ 2019-08-19 13:52 ` Andrew Cooper
2019-08-19 13:56 ` Michał Kowalczyk
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2019-08-19 13:52 UTC (permalink / raw)
To: Michał Kowalczyk, xen-devel
Cc: Wei Liu, Marek Marczykowski-Górecki, Jan Beulich,
Roger Pau Monné
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup
2019-08-19 13:52 ` Andrew Cooper
@ 2019-08-19 13:56 ` Michał Kowalczyk
2019-08-19 17:28 ` Andrew Cooper
0 siblings, 1 reply; 7+ messages in thread
From: Michał Kowalczyk @ 2019-08-19 13:56 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: Wei Liu, Marek Marczykowski-Górecki, Jan Beulich,
Roger Pau Monné
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup
2019-08-19 13:56 ` Michał Kowalczyk
@ 2019-08-19 17:28 ` Andrew Cooper
2019-08-19 18:01 ` Michał Kowalczyk
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2019-08-19 17:28 UTC (permalink / raw)
To: Michał Kowalczyk, xen-devel
Cc: Wei Liu, Marek Marczykowski-Górecki, Jan Beulich,
Roger Pau Monné
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup
2019-08-19 17:28 ` Andrew Cooper
@ 2019-08-19 18:01 ` Michał Kowalczyk
0 siblings, 0 replies; 7+ messages in thread
From: Michał Kowalczyk @ 2019-08-19 18:01 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: Wei Liu, Marek Marczykowski-Górecki, Jan Beulich,
Roger Pau Monné
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-19 18:01 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-19 2:23 [Xen-devel] [PATCH v1] x86: Restore IA32_MISC_ENABLE on wakeup Michał Kowalczyk
2019-08-19 9:04 ` Andrew Cooper
2019-08-19 13:50 ` Michał Kowalczyk
2019-08-19 13:52 ` Andrew Cooper
2019-08-19 13:56 ` Michał Kowalczyk
2019-08-19 17:28 ` Andrew Cooper
2019-08-19 18:01 ` Michał Kowalczyk
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).