linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v2] Fix undefined operation VMXOFF during reboot and crash
@ 2020-06-25 14:45 David P. Reed
  2020-06-25 14:59 ` David P. Reed
  0 siblings, 1 reply; 23+ messages in thread
From: David P. Reed @ 2020-06-25 14:45 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, X86 ML,
	H. Peter Anvin, Allison Randal, Enrico Weigelt,
	Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML

[Sorry: this is resent because my mailer included HTML, rejected by LKML]
Thanks for the response, Sean ... I had thought everyone was too busy to follow up from the first version.
 
I confess I'm not sure why this should be broken up into a patch series, given that it is so very small and is all aimed at the same category of bug.
 
And the "emergency" path pre-existed, I didn't want to propose removing it, since I assumed it was there for a reason. I didn't want to include my own judgement as to whether there should only be one path. (I'm pretty sure I didn't find a VMXOFF in KVM separately from the instance in this include file, but I will check).
 
A question: if I make it a series, I have to test each patch doesn't break something individually, in order to handle the case where one patch is accepted and the others are not. Do I need to test each individual patch thoroughly as an independent patch against all those cases?
I know the combination don't break anything and fixes the issues I've discovered by testing all combinations (and I've done some thorough testing of panics, oopses crashes, kexec, ... under all combinations of CR4.VMXE enablement and crash source to verify the fix fixes the problem's manifestations and to verify that it doesn't break any of the working paths.
 
That said, I'm willing to do a v3 "series" based on these suggestions if it will smooth its acceptance. If it's not going to get accepted after doing that, my motivation is flagging.
On Thursday, June 25, 2020 2:06am, "Sean Christopherson" <sean.j.christopherson@intel.com> said:



> On Thu, Jun 11, 2020 at 03:48:18PM -0400, David P. Reed wrote:
> > -/** Disable VMX on the current CPU
> > +/* Disable VMX on the current CPU
> > *
> > - * vmxoff causes a undefined-opcode exception if vmxon was not run
> > - * on the CPU previously. Only call this function if you know VMX
> > - * is enabled.
> > + * vmxoff causes an undefined-opcode exception if vmxon was not run
> > + * on the CPU previously. Only call this function directly if you know VMX
> > + * is enabled *and* CPU is in VMX root operation.
> > */
> > static inline void cpu_vmxoff(void)
> > {
> > - asm volatile ("vmxoff");
> > + asm volatile ("vmxoff" ::: "cc", "memory"); /* clears all flags on success
> */
> > cr4_clear_bits(X86_CR4_VMXE);
> > }
> >
> > @@ -47,17 +47,35 @@ static inline int cpu_vmx_enabled(void)
> > return __read_cr4() & X86_CR4_VMXE;
> > }
> >
> > -/** Disable VMX if it is enabled on the current CPU
> > - *
> > - * You shouldn't call this if cpu_has_vmx() returns 0.
> > +/*
> > + * Safely disable VMX root operation if active
> > + * Note that if CPU is not in VMX root operation this
> > + * VMXOFF will fault an undefined operation fault,
> > + * so use the exception masking facility to handle that RARE
> > + * case.
> > + * You shouldn't call this directly if cpu_has_vmx() returns 0
> > + */
> > +static inline void cpu_vmxoff_safe(void)
> > +{
> > + asm volatile("1:vmxoff\n\t" /* clears all flags on success */
> 
> Eh, I wouldn't bother with the comment, there are a million other caveats
> with VMXOFF that are far more interesting.
> 
> > + "2:\n\t"
> > + _ASM_EXTABLE(1b, 2b)
> > + ::: "cc", "memory");
> 
> Adding the memory and flags clobber should be a separate patch.
> 
> > + cr4_clear_bits(X86_CR4_VMXE);
> > +}
> 
> 
> I don't see any value in safe/unsafe variants. The only in-kernel user of
> VMXOFF outside of the emergency flows is KVM, which has its own VMXOFF
> helper, i.e. all users of cpu_vmxoff() want the "safe" variant. Just add
> the exception fixup to cpu_vmxoff() and call it good.
> 
> > +
> > +/*
> > + * Force disable VMX if it is enabled on the current CPU,
> > + * when it is unknown whether CPU is in VMX operation.
> > */
> > static inline void __cpu_emergency_vmxoff(void)
> > {
> > - if (cpu_vmx_enabled())
> > - cpu_vmxoff();
> > + if (!cpu_vmx_enabled())
> > + return;
> > + cpu_vmxoff_safe();
> 
> Unnecessary churn.
> 
> > }
> >
> > -/** Disable VMX if it is supported and enabled on the current CPU
> > +/* Force disable VMX if it is supported on current CPU
> > */
> > static inline void cpu_emergency_vmxoff(void)
> > {
> > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> > index e040ba6be27b..b0e6b106a67e 100644
> > --- a/arch/x86/kernel/reboot.c
> > +++ b/arch/x86/kernel/reboot.c
> > @@ -540,21 +540,14 @@ static void emergency_vmx_disable_all(void)
> > *
> > * For safety, we will avoid running the nmi_shootdown_cpus()
> > * stuff unnecessarily, but we don't have a way to check
> > - * if other CPUs have VMX enabled. So we will call it only if the
> > - * CPU we are running on has VMX enabled.
> > - *
> > - * We will miss cases where VMX is not enabled on all CPUs. This
> > - * shouldn't do much harm because KVM always enable VMX on all
> > - * CPUs anyway. But we can miss it on the small window where KVM
> > - * is still enabling VMX.
> > + * if other CPUs have VMX enabled.
> > */
> > - if (cpu_has_vmx() && cpu_vmx_enabled()) {
> > + if (cpu_has_vmx()) {
> > /* Disable VMX on this CPU. */
> > - cpu_vmxoff();
> > + cpu_emergency_vmxoff();
> 
> This also needs to be in a separate patch. And it should use
> __cpu_emergency_vmxoff() instead of cpu_emergency_vmxoff().
> 
> >
> > /* Halt and disable VMX on the other CPUs */
> > nmi_shootdown_cpus(vmxoff_nmi);
> > -
> > }
> > }
> >
> > --
> > 2.26.2
> >
>


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Fix undefined operation VMXOFF during reboot and crash
  2020-06-25 14:45 [PATCH v2] Fix undefined operation VMXOFF during reboot and crash David P. Reed
@ 2020-06-25 14:59 ` David P. Reed
  2020-06-29 20:54   ` David P. Reed
  0 siblings, 1 reply; 23+ messages in thread
From: David P. Reed @ 2020-06-25 14:59 UTC (permalink / raw)
  To: David P. Reed
  Cc: Sean Christopherson, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, X86 ML, H. Peter Anvin, Allison Randal,
	Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
	Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML

Correction to my comment below.
On Thursday, June 25, 2020 10:45am, "David P. Reed" <dpreed@deepplum.com> said:

> [Sorry: this is resent because my mailer included HTML, rejected by LKML]
> Thanks for the response, Sean ... I had thought everyone was too busy to follow up
> from the first version.
>  
> I confess I'm not sure why this should be broken up into a patch series, given
> that it is so very small and is all aimed at the same category of bug.
>  
> And the "emergency" path pre-existed, I didn't want to propose removing it, since
> I assumed it was there for a reason. I didn't want to include my own judgement as
> to whether there should only be one path. (I'm pretty sure I didn't find a VMXOFF
> in KVM separately from the instance in this include file, but I will check).
Just checked. Yes, the kvm code's handling of VMXOFF is separate, and though it uses exception masking, seems to do other things, perhaps related to nested KVM, but I haven't studied the deep logic of KVM nesting.

>  
> A question: if I make it a series, I have to test each patch doesn't break
> something individually, in order to handle the case where one patch is accepted
> and the others are not. Do I need to test each individual patch thoroughly as an
> independent patch against all those cases?
> I know the combination don't break anything and fixes the issues I've discovered
> by testing all combinations (and I've done some thorough testing of panics, oopses
> crashes, kexec, ... under all combinations of CR4.VMXE enablement and crash source
> to verify the fix fixes the problem's manifestations and to verify that it doesn't
> break any of the working paths.
>  
> That said, I'm willing to do a v3 "series" based on these suggestions if it will
> smooth its acceptance. If it's not going to get accepted after doing that, my
> motivation is flagging.
> On Thursday, June 25, 2020 2:06am, "Sean Christopherson"
> <sean.j.christopherson@intel.com> said:
> 
> 
> 
>> On Thu, Jun 11, 2020 at 03:48:18PM -0400, David P. Reed wrote:
>> > -/** Disable VMX on the current CPU
>> > +/* Disable VMX on the current CPU
>> > *
>> > - * vmxoff causes a undefined-opcode exception if vmxon was not run
>> > - * on the CPU previously. Only call this function if you know VMX
>> > - * is enabled.
>> > + * vmxoff causes an undefined-opcode exception if vmxon was not run
>> > + * on the CPU previously. Only call this function directly if you know VMX
>> > + * is enabled *and* CPU is in VMX root operation.
>> > */
>> > static inline void cpu_vmxoff(void)
>> > {
>> > - asm volatile ("vmxoff");
>> > + asm volatile ("vmxoff" ::: "cc", "memory"); /* clears all flags on success
>> */
>> > cr4_clear_bits(X86_CR4_VMXE);
>> > }
>> >
>> > @@ -47,17 +47,35 @@ static inline int cpu_vmx_enabled(void)
>> > return __read_cr4() & X86_CR4_VMXE;
>> > }
>> >
>> > -/** Disable VMX if it is enabled on the current CPU
>> > - *
>> > - * You shouldn't call this if cpu_has_vmx() returns 0.
>> > +/*
>> > + * Safely disable VMX root operation if active
>> > + * Note that if CPU is not in VMX root operation this
>> > + * VMXOFF will fault an undefined operation fault,
>> > + * so use the exception masking facility to handle that RARE
>> > + * case.
>> > + * You shouldn't call this directly if cpu_has_vmx() returns 0
>> > + */
>> > +static inline void cpu_vmxoff_safe(void)
>> > +{
>> > + asm volatile("1:vmxoff\n\t" /* clears all flags on success */
>>
>> Eh, I wouldn't bother with the comment, there are a million other caveats
>> with VMXOFF that are far more interesting.
>>
>> > + "2:\n\t"
>> > + _ASM_EXTABLE(1b, 2b)
>> > + ::: "cc", "memory");
>>
>> Adding the memory and flags clobber should be a separate patch.
>>
>> > + cr4_clear_bits(X86_CR4_VMXE);
>> > +}
>>
>>
>> I don't see any value in safe/unsafe variants. The only in-kernel user of
>> VMXOFF outside of the emergency flows is KVM, which has its own VMXOFF
>> helper, i.e. all users of cpu_vmxoff() want the "safe" variant. Just add
>> the exception fixup to cpu_vmxoff() and call it good.
>>
>> > +
>> > +/*
>> > + * Force disable VMX if it is enabled on the current CPU,
>> > + * when it is unknown whether CPU is in VMX operation.
>> > */
>> > static inline void __cpu_emergency_vmxoff(void)
>> > {
>> > - if (cpu_vmx_enabled())
>> > - cpu_vmxoff();
>> > + if (!cpu_vmx_enabled())
>> > + return;
>> > + cpu_vmxoff_safe();
>>
>> Unnecessary churn.
>>
>> > }
>> >
>> > -/** Disable VMX if it is supported and enabled on the current CPU
>> > +/* Force disable VMX if it is supported on current CPU
>> > */
>> > static inline void cpu_emergency_vmxoff(void)
>> > {
>> > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
>> > index e040ba6be27b..b0e6b106a67e 100644
>> > --- a/arch/x86/kernel/reboot.c
>> > +++ b/arch/x86/kernel/reboot.c
>> > @@ -540,21 +540,14 @@ static void emergency_vmx_disable_all(void)
>> > *
>> > * For safety, we will avoid running the nmi_shootdown_cpus()
>> > * stuff unnecessarily, but we don't have a way to check
>> > - * if other CPUs have VMX enabled. So we will call it only if the
>> > - * CPU we are running on has VMX enabled.
>> > - *
>> > - * We will miss cases where VMX is not enabled on all CPUs. This
>> > - * shouldn't do much harm because KVM always enable VMX on all
>> > - * CPUs anyway. But we can miss it on the small window where KVM
>> > - * is still enabling VMX.
>> > + * if other CPUs have VMX enabled.
>> > */
>> > - if (cpu_has_vmx() && cpu_vmx_enabled()) {
>> > + if (cpu_has_vmx()) {
>> > /* Disable VMX on this CPU. */
>> > - cpu_vmxoff();
>> > + cpu_emergency_vmxoff();
>>
>> This also needs to be in a separate patch. And it should use
>> __cpu_emergency_vmxoff() instead of cpu_emergency_vmxoff().
>>
>> >
>> > /* Halt and disable VMX on the other CPUs */
>> > nmi_shootdown_cpus(vmxoff_nmi);
>> > -
>> > }
>> > }
>> >
>> > --
>> > 2.26.2
>> >
>>
> 
> 



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Fix undefined operation VMXOFF during reboot and crash
  2020-06-25 14:59 ` David P. Reed
@ 2020-06-29 20:54   ` David P. Reed
  2020-06-29 21:22     ` Andy Lutomirski
  0 siblings, 1 reply; 23+ messages in thread
From: David P. Reed @ 2020-06-29 20:54 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Sean Christopherson, Ingo Molnar, Borislav Petkov, X86 ML,
	H. Peter Anvin, Allison Randal, Enrico Weigelt,
	Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML

Simple question for those on the To: and CC: list here. Should I abandon any hope of this patch being accepted? It's been a long time.

The non-response after I acknowledged that this was discovered by when working on a personal, non-commercial research project - which is "out-of-tree" (apparently dirty words on LKML) has me thinking my contribution is unwanted. That's fine, I suppose. I can maintain this patch out-of-tree as well.
I did incorporate all the helpful suggestions I received in this second patch, and given some encouragement, will happily submit a revised v3 if there is any likelihood of acceptance. I'm wary of doing more radical changes (like combining emergency and normal paths).


On Thursday, June 25, 2020 10:59am, "David P. Reed" <dpreed@deepplum.com> said:

> Correction to my comment below.
> On Thursday, June 25, 2020 10:45am, "David P. Reed" <dpreed@deepplum.com> said:
> 
>> [Sorry: this is resent because my mailer included HTML, rejected by LKML]
>> Thanks for the response, Sean ... I had thought everyone was too busy to follow
>> up
>> from the first version.
>>  
>> I confess I'm not sure why this should be broken up into a patch series, given
>> that it is so very small and is all aimed at the same category of bug.
>>  
>> And the "emergency" path pre-existed, I didn't want to propose removing it, since
>> I assumed it was there for a reason. I didn't want to include my own judgement as
>> to whether there should only be one path. (I'm pretty sure I didn't find a VMXOFF
>> in KVM separately from the instance in this include file, but I will check).
> Just checked. Yes, the kvm code's handling of VMXOFF is separate, and though it
> uses exception masking, seems to do other things, perhaps related to nested KVM,
> but I haven't studied the deep logic of KVM nesting.
> 
>>  
>> A question: if I make it a series, I have to test each patch doesn't break
>> something individually, in order to handle the case where one patch is accepted
>> and the others are not. Do I need to test each individual patch thoroughly as an
>> independent patch against all those cases?
>> I know the combination don't break anything and fixes the issues I've discovered
>> by testing all combinations (and I've done some thorough testing of panics,
>> oopses
>> crashes, kexec, ... under all combinations of CR4.VMXE enablement and crash
>> source
>> to verify the fix fixes the problem's manifestations and to verify that it
>> doesn't
>> break any of the working paths.
>>  
>> That said, I'm willing to do a v3 "series" based on these suggestions if it will
>> smooth its acceptance. If it's not going to get accepted after doing that, my
>> motivation is flagging.
>> On Thursday, June 25, 2020 2:06am, "Sean Christopherson"
>> <sean.j.christopherson@intel.com> said:
>>
>>
>>
>>> On Thu, Jun 11, 2020 at 03:48:18PM -0400, David P. Reed wrote:
>>> > -/** Disable VMX on the current CPU
>>> > +/* Disable VMX on the current CPU
>>> > *
>>> > - * vmxoff causes a undefined-opcode exception if vmxon was not run
>>> > - * on the CPU previously. Only call this function if you know VMX
>>> > - * is enabled.
>>> > + * vmxoff causes an undefined-opcode exception if vmxon was not run
>>> > + * on the CPU previously. Only call this function directly if you know VMX
>>> > + * is enabled *and* CPU is in VMX root operation.
>>> > */
>>> > static inline void cpu_vmxoff(void)
>>> > {
>>> > - asm volatile ("vmxoff");
>>> > + asm volatile ("vmxoff" ::: "cc", "memory"); /* clears all flags on success
>>> */
>>> > cr4_clear_bits(X86_CR4_VMXE);
>>> > }
>>> >
>>> > @@ -47,17 +47,35 @@ static inline int cpu_vmx_enabled(void)
>>> > return __read_cr4() & X86_CR4_VMXE;
>>> > }
>>> >
>>> > -/** Disable VMX if it is enabled on the current CPU
>>> > - *
>>> > - * You shouldn't call this if cpu_has_vmx() returns 0.
>>> > +/*
>>> > + * Safely disable VMX root operation if active
>>> > + * Note that if CPU is not in VMX root operation this
>>> > + * VMXOFF will fault an undefined operation fault,
>>> > + * so use the exception masking facility to handle that RARE
>>> > + * case.
>>> > + * You shouldn't call this directly if cpu_has_vmx() returns 0
>>> > + */
>>> > +static inline void cpu_vmxoff_safe(void)
>>> > +{
>>> > + asm volatile("1:vmxoff\n\t" /* clears all flags on success */
>>>
>>> Eh, I wouldn't bother with the comment, there are a million other caveats
>>> with VMXOFF that are far more interesting.
>>>
>>> > + "2:\n\t"
>>> > + _ASM_EXTABLE(1b, 2b)
>>> > + ::: "cc", "memory");
>>>
>>> Adding the memory and flags clobber should be a separate patch.
>>>
>>> > + cr4_clear_bits(X86_CR4_VMXE);
>>> > +}
>>>
>>>
>>> I don't see any value in safe/unsafe variants. The only in-kernel user of
>>> VMXOFF outside of the emergency flows is KVM, which has its own VMXOFF
>>> helper, i.e. all users of cpu_vmxoff() want the "safe" variant. Just add
>>> the exception fixup to cpu_vmxoff() and call it good.
>>>
>>> > +
>>> > +/*
>>> > + * Force disable VMX if it is enabled on the current CPU,
>>> > + * when it is unknown whether CPU is in VMX operation.
>>> > */
>>> > static inline void __cpu_emergency_vmxoff(void)
>>> > {
>>> > - if (cpu_vmx_enabled())
>>> > - cpu_vmxoff();
>>> > + if (!cpu_vmx_enabled())
>>> > + return;
>>> > + cpu_vmxoff_safe();
>>>
>>> Unnecessary churn.
>>>
>>> > }
>>> >
>>> > -/** Disable VMX if it is supported and enabled on the current CPU
>>> > +/* Force disable VMX if it is supported on current CPU
>>> > */
>>> > static inline void cpu_emergency_vmxoff(void)
>>> > {
>>> > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
>>> > index e040ba6be27b..b0e6b106a67e 100644
>>> > --- a/arch/x86/kernel/reboot.c
>>> > +++ b/arch/x86/kernel/reboot.c
>>> > @@ -540,21 +540,14 @@ static void emergency_vmx_disable_all(void)
>>> > *
>>> > * For safety, we will avoid running the nmi_shootdown_cpus()
>>> > * stuff unnecessarily, but we don't have a way to check
>>> > - * if other CPUs have VMX enabled. So we will call it only if the
>>> > - * CPU we are running on has VMX enabled.
>>> > - *
>>> > - * We will miss cases where VMX is not enabled on all CPUs. This
>>> > - * shouldn't do much harm because KVM always enable VMX on all
>>> > - * CPUs anyway. But we can miss it on the small window where KVM
>>> > - * is still enabling VMX.
>>> > + * if other CPUs have VMX enabled.
>>> > */
>>> > - if (cpu_has_vmx() && cpu_vmx_enabled()) {
>>> > + if (cpu_has_vmx()) {
>>> > /* Disable VMX on this CPU. */
>>> > - cpu_vmxoff();
>>> > + cpu_emergency_vmxoff();
>>>
>>> This also needs to be in a separate patch. And it should use
>>> __cpu_emergency_vmxoff() instead of cpu_emergency_vmxoff().
>>>
>>> >
>>> > /* Halt and disable VMX on the other CPUs */
>>> > nmi_shootdown_cpus(vmxoff_nmi);
>>> > -
>>> > }
>>> > }
>>> >
>>> > --
>>> > 2.26.2
>>> >
>>>
>>
>>
> 
> 
> 



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Fix undefined operation VMXOFF during reboot and crash
  2020-06-29 20:54   ` David P. Reed
@ 2020-06-29 21:22     ` Andy Lutomirski
  2020-06-29 21:49       ` Sean Christopherson
  0 siblings, 1 reply; 23+ messages in thread
From: Andy Lutomirski @ 2020-06-29 21:22 UTC (permalink / raw)
  To: David P. Reed
  Cc: Thomas Gleixner, Sean Christopherson, Ingo Molnar,
	Borislav Petkov, X86 ML, H. Peter Anvin, Allison Randal,
	Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
	Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML



> On Jun 29, 2020, at 1:54 PM, David P. Reed <dpreed@deepplum.com> wrote:
> 
> Simple question for those on the To: and CC: list here. Should I abandon any hope of this patch being accepted? It's been a long time.
> 
> The non-response after I acknowledged that this was discovered by when working on a personal, non-commercial research project - which is "out-of-tree" (apparently dirty words on LKML) has me thinking my contribution is unwanted. That's fine, I suppose. I can maintain this patch out-of-tree as well.
> I did incorporate all the helpful suggestions I received in this second patch, and given some encouragement, will happily submit a revised v3 if there is any likelihood of acceptance. I'm wary of doing more radical changes (like combining emergency and normal paths).
> 

Sorry about being slow and less actively encouraging than we should be. We absolutely welcome personal contributions. The actual problem is that everyone is worked and we’re all slow. Also, you may be hitting a corner case in the process: is this a KVM patch or an x86 patch?

> 
> On Thursday, June 25, 2020 10:59am, "David P. Reed" <dpreed@deepplum.com> said:
> 
>> Correction to my comment below.
>> On Thursday, June 25, 2020 10:45am, "David P. Reed" <dpreed@deepplum.com> said:
>> 
>>> [Sorry: this is resent because my mailer included HTML, rejected by LKML]
>>> Thanks for the response, Sean ... I had thought everyone was too busy to follow
>>> up
>>> from the first version.
>>>  
>>> I confess I'm not sure why this should be broken up into a patch series, given
>>> that it is so very small and is all aimed at the same category of bug.
>>>  
>>> And the "emergency" path pre-existed, I didn't want to propose removing it, since
>>> I assumed it was there for a reason. I didn't want to include my own judgement as
>>> to whether there should only be one path. (I'm pretty sure I didn't find a VMXOFF
>>> in KVM separately from the instance in this include file, but I will check).
>> Just checked. Yes, the kvm code's handling of VMXOFF is separate, and though it
>> uses exception masking, seems to do other things, perhaps related to nested KVM,
>> but I haven't studied the deep logic of KVM nesting.
>> 
>>>  
>>> A question: if I make it a series, I have to test each patch doesn't break
>>> something individually, in order to handle the case where one patch is accepted
>>> and the others are not. Do I need to test each individual patch thoroughly as an
>>> independent patch against all those cases?
>>> I know the combination don't break anything and fixes the issues I've discovered
>>> by testing all combinations (and I've done some thorough testing of panics,
>>> oopses
>>> crashes, kexec, ... under all combinations of CR4.VMXE enablement and crash
>>> source
>>> to verify the fix fixes the problem's manifestations and to verify that it
>>> doesn't
>>> break any of the working paths.
>>>  
>>> That said, I'm willing to do a v3 "series" based on these suggestions if it will
>>> smooth its acceptance. If it's not going to get accepted after doing that, my
>>> motivation is flagging.
>>> On Thursday, June 25, 2020 2:06am, "Sean Christopherson"
>>> <sean.j.christopherson@intel.com> said:
>>> 
>>> 
>>> 
>>>>> On Thu, Jun 11, 2020 at 03:48:18PM -0400, David P. Reed wrote:
>>>>>> -/** Disable VMX on the current CPU
>>>>>> +/* Disable VMX on the current CPU
>>>>>> *
>>>>>> - * vmxoff causes a undefined-opcode exception if vmxon was not run
>>>>>> - * on the CPU previously. Only call this function if you know VMX
>>>>>> - * is enabled.
>>>>>> + * vmxoff causes an undefined-opcode exception if vmxon was not run
>>>>>> + * on the CPU previously. Only call this function directly if you know VMX
>>>>>> + * is enabled *and* CPU is in VMX root operation.
>>>>>> */
>>>>>> static inline void cpu_vmxoff(void)
>>>>>> {
>>>>>> - asm volatile ("vmxoff");
>>>>>> + asm volatile ("vmxoff" ::: "cc", "memory"); /* clears all flags on success
>>>>> */
>>>>>> cr4_clear_bits(X86_CR4_VMXE);
>>>>>> }
>>>>>> 
>>>>>> @@ -47,17 +47,35 @@ static inline int cpu_vmx_enabled(void)
>>>>>> return __read_cr4() & X86_CR4_VMXE;
>>>>>> }
>>>>>> 
>>>>>> -/** Disable VMX if it is enabled on the current CPU
>>>>>> - *
>>>>>> - * You shouldn't call this if cpu_has_vmx() returns 0.
>>>>>> +/*
>>>>>> + * Safely disable VMX root operation if active
>>>>>> + * Note that if CPU is not in VMX root operation this
>>>>>> + * VMXOFF will fault an undefined operation fault,
>>>>>> + * so use the exception masking facility to handle that RARE
>>>>>> + * case.
>>>>>> + * You shouldn't call this directly if cpu_has_vmx() returns 0
>>>>>> + */
>>>>>> +static inline void cpu_vmxoff_safe(void)
>>>>>> +{
>>>>>> + asm volatile("1:vmxoff\n\t" /* clears all flags on success */
>>>>> 
>>>>> Eh, I wouldn't bother with the comment, there are a million other caveats
>>>>> with VMXOFF that are far more interesting.
>>>>> 
>>>>>> + "2:\n\t"
>>>>>> + _ASM_EXTABLE(1b, 2b)
>>>>>> + ::: "cc", "memory");
>>>>> 
>>>>> Adding the memory and flags clobber should be a separate patch.
>>>>> 
>>>>>> + cr4_clear_bits(X86_CR4_VMXE);
>>>>>> +}
>>>>> 
>>>>> 
>>>>> I don't see any value in safe/unsafe variants. The only in-kernel user of
>>>>> VMXOFF outside of the emergency flows is KVM, which has its own VMXOFF
>>>>> helper, i.e. all users of cpu_vmxoff() want the "safe" variant. Just add
>>>>> the exception fixup to cpu_vmxoff() and call it good.
>>>>> 
>>>>>> +
>>>>>> +/*
>>>>>> + * Force disable VMX if it is enabled on the current CPU,
>>>>>> + * when it is unknown whether CPU is in VMX operation.
>>>>>> */
>>>>>> static inline void __cpu_emergency_vmxoff(void)
>>>>>> {
>>>>>> - if (cpu_vmx_enabled())
>>>>>> - cpu_vmxoff();
>>>>>> + if (!cpu_vmx_enabled())
>>>>>> + return;
>>>>>> + cpu_vmxoff_safe();
>>>>> 
>>>>> Unnecessary churn.
>>>>> 
>>>>>> }
>>>>>> 
>>>>>> -/** Disable VMX if it is supported and enabled on the current CPU
>>>>>> +/* Force disable VMX if it is supported on current CPU
>>>>>> */
>>>>>> static inline void cpu_emergency_vmxoff(void)
>>>>>> {
>>>>>> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
>>>>>> index e040ba6be27b..b0e6b106a67e 100644
>>>>>> --- a/arch/x86/kernel/reboot.c
>>>>>> +++ b/arch/x86/kernel/reboot.c
>>>>>> @@ -540,21 +540,14 @@ static void emergency_vmx_disable_all(void)
>>>>>> *
>>>>>> * For safety, we will avoid running the nmi_shootdown_cpus()
>>>>>> * stuff unnecessarily, but we don't have a way to check
>>>>>> - * if other CPUs have VMX enabled. So we will call it only if the
>>>>>> - * CPU we are running on has VMX enabled.
>>>>>> - *
>>>>>> - * We will miss cases where VMX is not enabled on all CPUs. This
>>>>>> - * shouldn't do much harm because KVM always enable VMX on all
>>>>>> - * CPUs anyway. But we can miss it on the small window where KVM
>>>>>> - * is still enabling VMX.
>>>>>> + * if other CPUs have VMX enabled.
>>>>>> */
>>>>>> - if (cpu_has_vmx() && cpu_vmx_enabled()) {
>>>>>> + if (cpu_has_vmx()) {
>>>>>> /* Disable VMX on this CPU. */
>>>>>> - cpu_vmxoff();
>>>>>> + cpu_emergency_vmxoff();
>>>>> 
>>>>> This also needs to be in a separate patch. And it should use
>>>>> __cpu_emergency_vmxoff() instead of cpu_emergency_vmxoff().
>>>>> 
>>>>>> 
>>>>>> /* Halt and disable VMX on the other CPUs */
>>>>>> nmi_shootdown_cpus(vmxoff_nmi);
>>>>>> -
>>>>>> }
>>>>>> }
>>>>>> 
>>>>>> --
>>>>>> 2.26.2
>>>>>> 
>>>>> 
>>> 
>>> 
>> 
>> 
>> 
> 
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Fix undefined operation VMXOFF during reboot and crash
  2020-06-29 21:22     ` Andy Lutomirski
@ 2020-06-29 21:49       ` Sean Christopherson
  2020-06-29 22:46         ` David P. Reed
  2020-07-04 20:38         ` [PATCH v3 0/3] " David P. Reed
  0 siblings, 2 replies; 23+ messages in thread
From: Sean Christopherson @ 2020-06-29 21:49 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: David P. Reed, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	X86 ML, H. Peter Anvin, Allison Randal, Enrico Weigelt,
	Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML

On Mon, Jun 29, 2020 at 02:22:45PM -0700, Andy Lutomirski wrote:
> 
> 
> > On Jun 29, 2020, at 1:54 PM, David P. Reed <dpreed@deepplum.com> wrote:
> > 
> > Simple question for those on the To: and CC: list here. Should I
> > abandon any hope of this patch being accepted? It's been a long time.
> > 
> > The non-response after I acknowledged that this was discovered by when
> > working on a personal, non-commercial research project - which is
> > "out-of-tree" (apparently dirty words on LKML) has me thinking my
> > contribution is unwanted. That's fine, I suppose. I can maintain this patch
> > out-of-tree as well.  I did incorporate all the helpful suggestions I
> > received in this second patch, and given some encouragement, will happily
> > submit a revised v3 if there is any likelihood of acceptance. I'm wary of
> > doing more radical changes (like combining emergency and normal paths).
> > 
> 
> Sorry about being slow and less actively encouraging than we should be. We
> absolutely welcome personal contributions. The actual problem is that
> everyone is worked and we’re all slow. Also, you may be hitting a corner case
> in the process: is this a KVM patch or an x86 patch?

It's an x86 patch as it's not KVM specific, e.g. this code also helps play
nice with out of tree hypervisors.

The code change is mostly good, but it needs to be split up as there are
three separate fixes:

  1. Handle #UD on VMXON due to a race.
  2. Mark memory and flags as clobbered by VMXON.
  3. Change emergency_vmx_disable_all() to not manually check cpu_vmx_enabled().

Yes, the changes are tiny, but if for example #3 introduces a bug then we
don't have to revert #1 and #2.  Or perhaps older kernels are only subject
to the #1 and #2 and thus dumping all three changes into a single patch makes
it all harder to backport.  In other words, all the usual "one change per
patch" reasons.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Fix undefined operation VMXOFF during reboot and crash
  2020-06-29 21:49       ` Sean Christopherson
@ 2020-06-29 22:46         ` David P. Reed
  2020-07-04 20:38         ` [PATCH v3 0/3] " David P. Reed
  1 sibling, 0 replies; 23+ messages in thread
From: David P. Reed @ 2020-06-29 22:46 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	X86 ML, H. Peter Anvin, Allison Randal, Enrico Weigelt,
	Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML



On Monday, June 29, 2020 5:49pm, "Sean Christopherson" <sean.j.christopherson@intel.com> said:

> On Mon, Jun 29, 2020 at 02:22:45PM -0700, Andy Lutomirski wrote:
>>
>>
>> > On Jun 29, 2020, at 1:54 PM, David P. Reed <dpreed@deepplum.com> wrote:
>> >
>> > Simple question for those on the To: and CC: list here. Should I
>> > abandon any hope of this patch being accepted? It's been a long time.
>> >
>> > The non-response after I acknowledged that this was discovered by when
>> > working on a personal, non-commercial research project - which is
>> > "out-of-tree" (apparently dirty words on LKML) has me thinking my
>> > contribution is unwanted. That's fine, I suppose. I can maintain this patch
>> > out-of-tree as well.  I did incorporate all the helpful suggestions I
>> > received in this second patch, and given some encouragement, will happily
>> > submit a revised v3 if there is any likelihood of acceptance. I'm wary of
>> > doing more radical changes (like combining emergency and normal paths).
>> >
>>
>> Sorry about being slow and less actively encouraging than we should be. We
>> absolutely welcome personal contributions. The actual problem is that
>> everyone is worked and we’re all slow. Also, you may be hitting a corner
>> case
>> in the process: is this a KVM patch or an x86 patch?
> 
> It's an x86 patch as it's not KVM specific, e.g. this code also helps play
> nice with out of tree hypervisors.
> 
> The code change is mostly good, but it needs to be split up as there are
> three separate fixes:
> 
>   1. Handle #UD on VMXON due to a race.
>   2. Mark memory and flags as clobbered by VMXON.
>   3. Change emergency_vmx_disable_all() to not manually check cpu_vmx_enabled().
> 
> Yes, the changes are tiny, but if for example #3 introduces a bug then we
> don't have to revert #1 and #2.  Or perhaps older kernels are only subject
> to the #1 and #2 and thus dumping all three changes into a single patch makes
> it all harder to backport.  In other words, all the usual "one change per
> patch" reasons.
> 
Thanks. If no one else responds with additional suggestions, I will make it into 3 patches.
I'm happy to learn the nuances of the kernel patch regimen.




^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 0/3] Fix undefined operation VMXOFF during reboot and crash
  2020-06-29 21:49       ` Sean Christopherson
  2020-06-29 22:46         ` David P. Reed
@ 2020-07-04 20:38         ` David P. Reed
  2020-07-04 20:38           ` [PATCH v3 1/3] Correct asm VMXOFF side effects David P. Reed
                             ` (2 more replies)
  1 sibling, 3 replies; 23+ messages in thread
From: David P. Reed @ 2020-07-04 20:38 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: David P. Reed, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, X86 ML, H. Peter Anvin, Allison Randal,
	Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
	Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML

At the request of Sean Christopherson, the original patch was split
into three patches, each fixing a distinct issue related to the original
bug, of a hang due to VMXOFF causing an undefined operation fault
when the kernel reboots with CR4.VMXE set. The combination of
the patches is the complete fix to the reported bug, and a lurking
error in asm side effects.

David P. Reed (3):
  Correct asm VMXOFF side effects
  Fix undefined operation fault that can hang a cpu on crash or panic
  Force all cpus to exit VMX root operation on crash/panic reliably

 arch/x86/include/asm/virtext.h | 24 ++++++++++++++++--------
 arch/x86/kernel/reboot.c       | 20 +++++++-------------
 2 files changed, 23 insertions(+), 21 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 1/3] Correct asm VMXOFF side effects
  2020-07-04 20:38         ` [PATCH v3 0/3] " David P. Reed
@ 2020-07-04 20:38           ` David P. Reed
  2020-07-05  5:46             ` Randy Dunlap
  2020-07-04 20:38           ` [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic David P. Reed
  2020-07-04 20:38           ` [PATCH v3 3/3] Force all cpus to exit VMX root operation on crash/panic reliably David P. Reed
  2 siblings, 1 reply; 23+ messages in thread
From: David P. Reed @ 2020-07-04 20:38 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: David P. Reed, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, X86 ML, H. Peter Anvin, Allison Randal,
	Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
	Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML

Tell gcc that VMXOFF instruction clobbers condition codes
and memory when executed.
Also, correct original comments to remove kernel-doc syntax
per Randy Dunlap's request.

Suggested-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: David P. Reed <dpreed@deepplum.com>
---
 arch/x86/include/asm/virtext.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
index 9aad0e0876fb..0ede8d04535a 100644
--- a/arch/x86/include/asm/virtext.h
+++ b/arch/x86/include/asm/virtext.h
@@ -30,7 +30,7 @@ static inline int cpu_has_vmx(void)
 }
 
 
-/** Disable VMX on the current CPU
+/* Disable VMX on the current CPU
  *
  * vmxoff causes a undefined-opcode exception if vmxon was not run
  * on the CPU previously. Only call this function if you know VMX
@@ -38,7 +38,7 @@ static inline int cpu_has_vmx(void)
  */
 static inline void cpu_vmxoff(void)
 {
-	asm volatile ("vmxoff");
+	asm volatile ("vmxoff" ::: "cc", "memory");
 	cr4_clear_bits(X86_CR4_VMXE);
 }
 
@@ -47,7 +47,7 @@ static inline int cpu_vmx_enabled(void)
 	return __read_cr4() & X86_CR4_VMXE;
 }
 
-/** Disable VMX if it is enabled on the current CPU
+/* Disable VMX if it is enabled on the current CPU
  *
  * You shouldn't call this if cpu_has_vmx() returns 0.
  */
@@ -57,7 +57,7 @@ static inline void __cpu_emergency_vmxoff(void)
 		cpu_vmxoff();
 }
 
-/** Disable VMX if it is supported and enabled on the current CPU
+/* Disable VMX if it is supported and enabled on the current CPU
  */
 static inline void cpu_emergency_vmxoff(void)
 {
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic
  2020-07-04 20:38         ` [PATCH v3 0/3] " David P. Reed
  2020-07-04 20:38           ` [PATCH v3 1/3] Correct asm VMXOFF side effects David P. Reed
@ 2020-07-04 20:38           ` David P. Reed
  2020-07-05 18:22             ` Andy Lutomirski
  2020-07-07  5:09             ` Sean Christopherson
  2020-07-04 20:38           ` [PATCH v3 3/3] Force all cpus to exit VMX root operation on crash/panic reliably David P. Reed
  2 siblings, 2 replies; 23+ messages in thread
From: David P. Reed @ 2020-07-04 20:38 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: David P. Reed, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, X86 ML, H. Peter Anvin, Allison Randal,
	Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
	Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML

Fix: Mask undefined operation fault during emergency VMXOFF that must be
attempted to force cpu exit from VMX root operation.
Explanation: When a cpu may be in VMX root operation (only possible when
CR4.VMXE is set), crash or panic reboot tries to exit VMX root operation
using VMXOFF. This is necessary, because any INIT will be masked while cpu
is in VMX root operation, but that state cannot be reliably
discerned by the state of the cpu.
VMXOFF faults if the cpu is not actually in VMX root operation, signalling
undefined operation.
Discovered while debugging an out-of-tree x-visor with a race. Can happen
due to certain kinds of bugs in KVM.

Fixes: 208067 <https://bugzilla.kernel.org/show_bug.cgi?id=208067>
Reported-by: David P. Reed <dpreed@deepplum.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: David P. Reed <dpreed@deepplum.com>
---
 arch/x86/include/asm/virtext.h | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
index 0ede8d04535a..0e0900eacb9c 100644
--- a/arch/x86/include/asm/virtext.h
+++ b/arch/x86/include/asm/virtext.h
@@ -30,11 +30,11 @@ static inline int cpu_has_vmx(void)
 }
 
 
-/* Disable VMX on the current CPU
+/* Exit VMX root mode and isable VMX on the current CPU.
  *
  * vmxoff causes a undefined-opcode exception if vmxon was not run
- * on the CPU previously. Only call this function if you know VMX
- * is enabled.
+ * on the CPU previously. Only call this function if you know cpu
+ * is in VMX root mode.
  */
 static inline void cpu_vmxoff(void)
 {
@@ -47,14 +47,22 @@ static inline int cpu_vmx_enabled(void)
 	return __read_cr4() & X86_CR4_VMXE;
 }
 
-/* Disable VMX if it is enabled on the current CPU
+/* Safely exit VMX root mode and disable VMX if VMX enabled
+ * on the current CPU. Handle undefined-opcode fault
+ * that can occur if cpu is not in VMX root mode, due
+ * to a race.
  *
  * You shouldn't call this if cpu_has_vmx() returns 0.
  */
 static inline void __cpu_emergency_vmxoff(void)
 {
-	if (cpu_vmx_enabled())
-		cpu_vmxoff();
+	if (!cpu_vmx_enabled())
+		return;
+	asm volatile ("1:vmxoff\n\t"
+		      "2:\n\t"
+		      _ASM_EXTABLE(1b, 2b)
+		      ::: "cc", "memory");
+	cr4_clear_bits(X86_CR4_VMXE);
 }
 
 /* Disable VMX if it is supported and enabled on the current CPU
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v3 3/3] Force all cpus to exit VMX root operation on crash/panic reliably
  2020-07-04 20:38         ` [PATCH v3 0/3] " David P. Reed
  2020-07-04 20:38           ` [PATCH v3 1/3] Correct asm VMXOFF side effects David P. Reed
  2020-07-04 20:38           ` [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic David P. Reed
@ 2020-07-04 20:38           ` David P. Reed
  2020-07-05 18:26             ` Andy Lutomirski
  2 siblings, 1 reply; 23+ messages in thread
From: David P. Reed @ 2020-07-04 20:38 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: David P. Reed, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, X86 ML, H. Peter Anvin, Allison Randal,
	Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
	Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML

Fix the logic during crash/panic reboot on Intel processors that
can support VMX operation to ensure that all processors are not
in VMX root operation. Prior code made optimistic assumptions
about other cpus that would leave other cpus in VMX root operation
depending on timing of crash/panic reboot.
Builds on cpu_ermergency_vmxoff() and __cpu_emergency_vmxoff() created
in a prior patch.

Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: David P. Reed <dpreed@deepplum.com>
---
 arch/x86/kernel/reboot.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 0ec7ced727fe..c8e96ba78efc 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -543,24 +543,18 @@ static void emergency_vmx_disable_all(void)
 	 * signals when VMX is enabled.
 	 *
 	 * We can't take any locks and we may be on an inconsistent
-	 * state, so we use NMIs as IPIs to tell the other CPUs to disable
-	 * VMX and halt.
+	 * state, so we use NMIs as IPIs to tell the other CPUs to exit
+	 * VMX root operation and halt.
 	 *
 	 * For safety, we will avoid running the nmi_shootdown_cpus()
 	 * stuff unnecessarily, but we don't have a way to check
-	 * if other CPUs have VMX enabled. So we will call it only if the
-	 * CPU we are running on has VMX enabled.
-	 *
-	 * We will miss cases where VMX is not enabled on all CPUs. This
-	 * shouldn't do much harm because KVM always enable VMX on all
-	 * CPUs anyway. But we can miss it on the small window where KVM
-	 * is still enabling VMX.
+	 * if other CPUs might be in VMX root operation.
 	 */
-	if (cpu_has_vmx() && cpu_vmx_enabled()) {
-		/* Disable VMX on this CPU. */
-		cpu_vmxoff();
+	if (cpu_has_vmx()) {
+		/* Safely force out of VMX root operation on this CPU. */
+		__cpu_emergency_vmxoff();
 
-		/* Halt and disable VMX on the other CPUs */
+		/* Halt and exit VMX root operation on the other CPUs */
 		nmi_shootdown_cpus(vmxoff_nmi);
 
 	}
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/3] Correct asm VMXOFF side effects
  2020-07-04 20:38           ` [PATCH v3 1/3] Correct asm VMXOFF side effects David P. Reed
@ 2020-07-05  5:46             ` Randy Dunlap
  0 siblings, 0 replies; 23+ messages in thread
From: Randy Dunlap @ 2020-07-05  5:46 UTC (permalink / raw)
  To: David P. Reed, Sean Christopherson
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	X86 ML, H. Peter Anvin, Allison Randal, Enrico Weigelt,
	Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
	Martin Molnar, Andy Lutomirski, Alexandre Chartre, Jann Horn,
	Dave Hansen, LKML

On 7/4/20 1:38 PM, David P. Reed wrote:
> Tell gcc that VMXOFF instruction clobbers condition codes
> and memory when executed.
> Also, correct original comments to remove kernel-doc syntax
> per Randy Dunlap's request.

Looks good. Thanks.  For the comment changes:
Acked-by: Randy Dunlap <rdunlap@infradead.org>

> Suggested-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: David P. Reed <dpreed@deepplum.com>
> ---
>  arch/x86/include/asm/virtext.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
> index 9aad0e0876fb..0ede8d04535a 100644
> --- a/arch/x86/include/asm/virtext.h
> +++ b/arch/x86/include/asm/virtext.h
> @@ -30,7 +30,7 @@ static inline int cpu_has_vmx(void)
>  }
>  
>  
> -/** Disable VMX on the current CPU
> +/* Disable VMX on the current CPU
>   *
>   * vmxoff causes a undefined-opcode exception if vmxon was not run
>   * on the CPU previously. Only call this function if you know VMX
> @@ -38,7 +38,7 @@ static inline int cpu_has_vmx(void)
>   */
>  static inline void cpu_vmxoff(void)
>  {
> -	asm volatile ("vmxoff");
> +	asm volatile ("vmxoff" ::: "cc", "memory");
>  	cr4_clear_bits(X86_CR4_VMXE);
>  }
>  
> @@ -47,7 +47,7 @@ static inline int cpu_vmx_enabled(void)
>  	return __read_cr4() & X86_CR4_VMXE;
>  }
>  
> -/** Disable VMX if it is enabled on the current CPU
> +/* Disable VMX if it is enabled on the current CPU
>   *
>   * You shouldn't call this if cpu_has_vmx() returns 0.
>   */
> @@ -57,7 +57,7 @@ static inline void __cpu_emergency_vmxoff(void)
>  		cpu_vmxoff();
>  }
>  
> -/** Disable VMX if it is supported and enabled on the current CPU
> +/* Disable VMX if it is supported and enabled on the current CPU
>   */
>  static inline void cpu_emergency_vmxoff(void)
>  {
> 


-- 
~Randy


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic
  2020-07-04 20:38           ` [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic David P. Reed
@ 2020-07-05 18:22             ` Andy Lutomirski
  2020-07-05 19:52               ` David P. Reed
  2020-07-07  5:09             ` Sean Christopherson
  1 sibling, 1 reply; 23+ messages in thread
From: Andy Lutomirski @ 2020-07-05 18:22 UTC (permalink / raw)
  To: David P. Reed
  Cc: Sean Christopherson, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, X86 ML, H. Peter Anvin, Allison Randal,
	Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
	Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML

On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <dpreed@deepplum.com> wrote:
>
> Fix: Mask undefined operation fault during emergency VMXOFF that must be
> attempted to force cpu exit from VMX root operation.
> Explanation: When a cpu may be in VMX root operation (only possible when
> CR4.VMXE is set), crash or panic reboot tries to exit VMX root operation
> using VMXOFF. This is necessary, because any INIT will be masked while cpu
> is in VMX root operation, but that state cannot be reliably
> discerned by the state of the cpu.
> VMXOFF faults if the cpu is not actually in VMX root operation, signalling
> undefined operation.
> Discovered while debugging an out-of-tree x-visor with a race. Can happen
> due to certain kinds of bugs in KVM.

Can you re-wrap lines to 68 characters?  Also, the Fix: and
Explanation: is probably unnecessary.  You could say:

Ignore a potential #UD failut during emergency VMXOFF ...

When a cpu may be in VMX ...

>
> Fixes: 208067 <https://bugzilla.kernel.org/show_bug.cgi?id=208067>
> Reported-by: David P. Reed <dpreed@deepplum.com>

It's not really necessary to say that you, the author, reported the
problem, but I guess it's harmless.

> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Suggested-by: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: David P. Reed <dpreed@deepplum.com>
> ---
>  arch/x86/include/asm/virtext.h | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
> index 0ede8d04535a..0e0900eacb9c 100644
> --- a/arch/x86/include/asm/virtext.h
> +++ b/arch/x86/include/asm/virtext.h
> @@ -30,11 +30,11 @@ static inline int cpu_has_vmx(void)
>  }
>
>
> -/* Disable VMX on the current CPU
> +/* Exit VMX root mode and isable VMX on the current CPU.

s/isable/disable/


>  /* Disable VMX if it is supported and enabled on the current CPU
> --
> 2.26.2
>

Other than that:

Reviewed-by: Andy Lutomirski <luto@kernel.org>

--Andy

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 3/3] Force all cpus to exit VMX root operation on crash/panic reliably
  2020-07-04 20:38           ` [PATCH v3 3/3] Force all cpus to exit VMX root operation on crash/panic reliably David P. Reed
@ 2020-07-05 18:26             ` Andy Lutomirski
  2020-07-05 20:00               ` David P. Reed
  0 siblings, 1 reply; 23+ messages in thread
From: Andy Lutomirski @ 2020-07-05 18:26 UTC (permalink / raw)
  To: David P. Reed
  Cc: Sean Christopherson, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, X86 ML, H. Peter Anvin, Allison Randal,
	Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
	Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML

On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <dpreed@deepplum.com> wrote:
>
> Fix the logic during crash/panic reboot on Intel processors that
> can support VMX operation to ensure that all processors are not
> in VMX root operation. Prior code made optimistic assumptions
> about other cpus that would leave other cpus in VMX root operation
> depending on timing of crash/panic reboot.
> Builds on cpu_ermergency_vmxoff() and __cpu_emergency_vmxoff() created
> in a prior patch.
>
> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: David P. Reed <dpreed@deepplum.com>
> ---
>  arch/x86/kernel/reboot.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 0ec7ced727fe..c8e96ba78efc 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -543,24 +543,18 @@ static void emergency_vmx_disable_all(void)
>          * signals when VMX is enabled.
>          *
>          * We can't take any locks and we may be on an inconsistent
> -        * state, so we use NMIs as IPIs to tell the other CPUs to disable
> -        * VMX and halt.
> +        * state, so we use NMIs as IPIs to tell the other CPUs to exit
> +        * VMX root operation and halt.
>          *
>          * For safety, we will avoid running the nmi_shootdown_cpus()
>          * stuff unnecessarily, but we don't have a way to check
> -        * if other CPUs have VMX enabled. So we will call it only if the
> -        * CPU we are running on has VMX enabled.
> -        *
> -        * We will miss cases where VMX is not enabled on all CPUs. This
> -        * shouldn't do much harm because KVM always enable VMX on all
> -        * CPUs anyway. But we can miss it on the small window where KVM
> -        * is still enabling VMX.
> +        * if other CPUs might be in VMX root operation.
>          */
> -       if (cpu_has_vmx() && cpu_vmx_enabled()) {
> -               /* Disable VMX on this CPU. */
> -               cpu_vmxoff();
> +       if (cpu_has_vmx()) {
> +               /* Safely force out of VMX root operation on this CPU. */
> +               __cpu_emergency_vmxoff();
>
> -               /* Halt and disable VMX on the other CPUs */
> +               /* Halt and exit VMX root operation on the other CPUs */
>                 nmi_shootdown_cpus(vmxoff_nmi);
>
>         }

Seems reasonable to me.

As a minor caveat, doing cr4_clear_bits() in NMI context is not really
okay, but we're about to reboot, so nothing too awful should happen.
And this has very little to do with your patch.

Reviewed-by: Andy Lutomirski <luto@kernel.org>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic
  2020-07-05 18:22             ` Andy Lutomirski
@ 2020-07-05 19:52               ` David P. Reed
  2020-07-05 20:55                 ` Andy Lutomirski
  0 siblings, 1 reply; 23+ messages in thread
From: David P. Reed @ 2020-07-05 19:52 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Sean Christopherson, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, X86 ML, H. Peter Anvin, Allison Randal,
	Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
	Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML

Thanks, will handle these. 2 questions below.

On Sunday, July 5, 2020 2:22pm, "Andy Lutomirski" <luto@kernel.org> said:

> On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <dpreed@deepplum.com> wrote:
>>
>> Fix: Mask undefined operation fault during emergency VMXOFF that must be
>> attempted to force cpu exit from VMX root operation.
>> Explanation: When a cpu may be in VMX root operation (only possible when
>> CR4.VMXE is set), crash or panic reboot tries to exit VMX root operation
>> using VMXOFF. This is necessary, because any INIT will be masked while cpu
>> is in VMX root operation, but that state cannot be reliably
>> discerned by the state of the cpu.
>> VMXOFF faults if the cpu is not actually in VMX root operation, signalling
>> undefined operation.
>> Discovered while debugging an out-of-tree x-visor with a race. Can happen
>> due to certain kinds of bugs in KVM.
> 
> Can you re-wrap lines to 68 characters?  Also, the Fix: and

I used 'scripts/checkpatch.pl' and it had me wrap to 75 chars:
"WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)"

Should I submit a fix to checkpatch.pl to say 68? 

> Explanation: is probably unnecessary.  You could say:
> 
> Ignore a potential #UD failut during emergency VMXOFF ...
> 
> When a cpu may be in VMX ...
> 
>>
>> Fixes: 208067 <https://bugzilla.kernel.org/show_bug.cgi?id=208067>
>> Reported-by: David P. Reed <dpreed@deepplum.com>
> 
> It's not really necessary to say that you, the author, reported the
> problem, but I guess it's harmless.
> 
>> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
>> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> Suggested-by: Andy Lutomirski <luto@kernel.org>
>> Signed-off-by: David P. Reed <dpreed@deepplum.com>
>> ---
>>  arch/x86/include/asm/virtext.h | 20 ++++++++++++++------
>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
>> index 0ede8d04535a..0e0900eacb9c 100644
>> --- a/arch/x86/include/asm/virtext.h
>> +++ b/arch/x86/include/asm/virtext.h
>> @@ -30,11 +30,11 @@ static inline int cpu_has_vmx(void)
>>  }
>>
>>
>> -/* Disable VMX on the current CPU
>> +/* Exit VMX root mode and isable VMX on the current CPU.
> 
> s/isable/disable/
> 
> 
>>  /* Disable VMX if it is supported and enabled on the current CPU
>> --
>> 2.26.2
>>
> 
> Other than that:
> 
> Reviewed-by: Andy Lutomirski <luto@kernel.org>

As a newbie, I have a process question - should I resend the patch with the 'Reviewed-by' line, as well as correcting the other wording? Thanks!

> 
> --Andy
> 



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 3/3] Force all cpus to exit VMX root operation on crash/panic reliably
  2020-07-05 18:26             ` Andy Lutomirski
@ 2020-07-05 20:00               ` David P. Reed
  2020-07-05 20:53                 ` Andy Lutomirski
  0 siblings, 1 reply; 23+ messages in thread
From: David P. Reed @ 2020-07-05 20:00 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Sean Christopherson, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, X86 ML, H. Peter Anvin, Allison Randal,
	Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
	Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML



On Sunday, July 5, 2020 2:26pm, "Andy Lutomirski" <luto@kernel.org> said:

> On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <dpreed@deepplum.com> wrote:
>>
>> Fix the logic during crash/panic reboot on Intel processors that
>> can support VMX operation to ensure that all processors are not
>> in VMX root operation. Prior code made optimistic assumptions
>> about other cpus that would leave other cpus in VMX root operation
>> depending on timing of crash/panic reboot.
>> Builds on cpu_ermergency_vmxoff() and __cpu_emergency_vmxoff() created
>> in a prior patch.
>>
>> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> Signed-off-by: David P. Reed <dpreed@deepplum.com>
>> ---
>>  arch/x86/kernel/reboot.c | 20 +++++++-------------
>>  1 file changed, 7 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
>> index 0ec7ced727fe..c8e96ba78efc 100644
>> --- a/arch/x86/kernel/reboot.c
>> +++ b/arch/x86/kernel/reboot.c
>> @@ -543,24 +543,18 @@ static void emergency_vmx_disable_all(void)
>>          * signals when VMX is enabled.
>>          *
>>          * We can't take any locks and we may be on an inconsistent
>> -        * state, so we use NMIs as IPIs to tell the other CPUs to disable
>> -        * VMX and halt.
>> +        * state, so we use NMIs as IPIs to tell the other CPUs to exit
>> +        * VMX root operation and halt.
>>          *
>>          * For safety, we will avoid running the nmi_shootdown_cpus()
>>          * stuff unnecessarily, but we don't have a way to check
>> -        * if other CPUs have VMX enabled. So we will call it only if the
>> -        * CPU we are running on has VMX enabled.
>> -        *
>> -        * We will miss cases where VMX is not enabled on all CPUs. This
>> -        * shouldn't do much harm because KVM always enable VMX on all
>> -        * CPUs anyway. But we can miss it on the small window where KVM
>> -        * is still enabling VMX.
>> +        * if other CPUs might be in VMX root operation.
>>          */
>> -       if (cpu_has_vmx() && cpu_vmx_enabled()) {
>> -               /* Disable VMX on this CPU. */
>> -               cpu_vmxoff();
>> +       if (cpu_has_vmx()) {
>> +               /* Safely force out of VMX root operation on this CPU. */
>> +               __cpu_emergency_vmxoff();
>>
>> -               /* Halt and disable VMX on the other CPUs */
>> +               /* Halt and exit VMX root operation on the other CPUs */
>>                 nmi_shootdown_cpus(vmxoff_nmi);
>>
>>         }
> 
> Seems reasonable to me.
> 
> As a minor caveat, doing cr4_clear_bits() in NMI context is not really
> okay, but we're about to reboot, so nothing too awful should happen.
> And this has very little to do with your patch.

I had wondered why the bit is cleared, too. (I assumed it was OK or desirable, because it was being cleared in NMI context before). Happy to submit a separate patch to eliminate that issue as well, since the point of emergency vmxoff is only to get out of VMX root mode - CR4.VMXE's state is irrelevant. Of course, clearing it prevents any future emergency vmxoff attempts. (there seemed to be some confusion about "enabling" VMX vs. "in VMX operation" in the comments)  Should I?

> 
> Reviewed-by: Andy Lutomirski <luto@kernel.org>
> 



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 3/3] Force all cpus to exit VMX root operation on crash/panic reliably
  2020-07-05 20:00               ` David P. Reed
@ 2020-07-05 20:53                 ` Andy Lutomirski
  2020-07-07  5:29                   ` Sean Christopherson
  0 siblings, 1 reply; 23+ messages in thread
From: Andy Lutomirski @ 2020-07-05 20:53 UTC (permalink / raw)
  To: David P. Reed
  Cc: Andy Lutomirski, Sean Christopherson, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, X86 ML, H. Peter Anvin,
	Allison Randal, Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
	Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Alexandre Chartre, Jann Horn,
	Dave Hansen, LKML

On Sun, Jul 5, 2020 at 1:00 PM David P. Reed <dpreed@deepplum.com> wrote:
>
>
>
> On Sunday, July 5, 2020 2:26pm, "Andy Lutomirski" <luto@kernel.org> said:
>
> > On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <dpreed@deepplum.com> wrote:
> >>
> >> Fix the logic during crash/panic reboot on Intel processors that
> >> can support VMX operation to ensure that all processors are not
> >> in VMX root operation. Prior code made optimistic assumptions
> >> about other cpus that would leave other cpus in VMX root operation
> >> depending on timing of crash/panic reboot.
> >> Builds on cpu_ermergency_vmxoff() and __cpu_emergency_vmxoff() created
> >> in a prior patch.
> >>
> >> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >> Signed-off-by: David P. Reed <dpreed@deepplum.com>
> >> ---
> >>  arch/x86/kernel/reboot.c | 20 +++++++-------------
> >>  1 file changed, 7 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> >> index 0ec7ced727fe..c8e96ba78efc 100644
> >> --- a/arch/x86/kernel/reboot.c
> >> +++ b/arch/x86/kernel/reboot.c
> >> @@ -543,24 +543,18 @@ static void emergency_vmx_disable_all(void)
> >>          * signals when VMX is enabled.
> >>          *
> >>          * We can't take any locks and we may be on an inconsistent
> >> -        * state, so we use NMIs as IPIs to tell the other CPUs to disable
> >> -        * VMX and halt.
> >> +        * state, so we use NMIs as IPIs to tell the other CPUs to exit
> >> +        * VMX root operation and halt.
> >>          *
> >>          * For safety, we will avoid running the nmi_shootdown_cpus()
> >>          * stuff unnecessarily, but we don't have a way to check
> >> -        * if other CPUs have VMX enabled. So we will call it only if the
> >> -        * CPU we are running on has VMX enabled.
> >> -        *
> >> -        * We will miss cases where VMX is not enabled on all CPUs. This
> >> -        * shouldn't do much harm because KVM always enable VMX on all
> >> -        * CPUs anyway. But we can miss it on the small window where KVM
> >> -        * is still enabling VMX.
> >> +        * if other CPUs might be in VMX root operation.
> >>          */
> >> -       if (cpu_has_vmx() && cpu_vmx_enabled()) {
> >> -               /* Disable VMX on this CPU. */
> >> -               cpu_vmxoff();
> >> +       if (cpu_has_vmx()) {
> >> +               /* Safely force out of VMX root operation on this CPU. */
> >> +               __cpu_emergency_vmxoff();
> >>
> >> -               /* Halt and disable VMX on the other CPUs */
> >> +               /* Halt and exit VMX root operation on the other CPUs */
> >>                 nmi_shootdown_cpus(vmxoff_nmi);
> >>
> >>         }
> >
> > Seems reasonable to me.
> >
> > As a minor caveat, doing cr4_clear_bits() in NMI context is not really
> > okay, but we're about to reboot, so nothing too awful should happen.
> > And this has very little to do with your patch.
>
> I had wondered why the bit is cleared, too. (I assumed it was OK or desirable, because it was being cleared in NMI context before). Happy to submit a separate patch to eliminate that issue as well, since the point of emergency vmxoff is only to get out of VMX root mode - CR4.VMXE's state is irrelevant. Of course, clearing it prevents any future emergency vmxoff attempts. (there seemed to be some confusion about "enabling" VMX vs. "in VMX operation" in the comments)  Should I?

I have a vague recollection of some firmwares that got upset if
rebooted with CR4.VMXE set.  Sean?

The real issue here is that the percpu CR4 machinery uses IRQ-offness
as a lock, and NMI breaks this.

--Andy

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic
  2020-07-05 19:52               ` David P. Reed
@ 2020-07-05 20:55                 ` Andy Lutomirski
  2020-07-05 22:07                   ` David P. Reed
  0 siblings, 1 reply; 23+ messages in thread
From: Andy Lutomirski @ 2020-07-05 20:55 UTC (permalink / raw)
  To: David P. Reed
  Cc: Andy Lutomirski, Sean Christopherson, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, X86 ML, H. Peter Anvin,
	Allison Randal, Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
	Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Alexandre Chartre, Jann Horn,
	Dave Hansen, LKML

On Sun, Jul 5, 2020 at 12:52 PM David P. Reed <dpreed@deepplum.com> wrote:
>
> Thanks, will handle these. 2 questions below.
>
> On Sunday, July 5, 2020 2:22pm, "Andy Lutomirski" <luto@kernel.org> said:
>
> > On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <dpreed@deepplum.com> wrote:
> >>
> >> Fix: Mask undefined operation fault during emergency VMXOFF that must be
> >> attempted to force cpu exit from VMX root operation.
> >> Explanation: When a cpu may be in VMX root operation (only possible when
> >> CR4.VMXE is set), crash or panic reboot tries to exit VMX root operation
> >> using VMXOFF. This is necessary, because any INIT will be masked while cpu
> >> is in VMX root operation, but that state cannot be reliably
> >> discerned by the state of the cpu.
> >> VMXOFF faults if the cpu is not actually in VMX root operation, signalling
> >> undefined operation.
> >> Discovered while debugging an out-of-tree x-visor with a race. Can happen
> >> due to certain kinds of bugs in KVM.
> >
> > Can you re-wrap lines to 68 characters?  Also, the Fix: and
>
> I used 'scripts/checkpatch.pl' and it had me wrap to 75 chars:
> "WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)"
>
> Should I submit a fix to checkpatch.pl to say 68?

75 is probably fine too, but something is odd about your wrapping.
You have long lines mostly alternating with short lines.  It's as if
you wrote 120-ish character lines and then wrapped to 75 without
reflowing.

>
> > Explanation: is probably unnecessary.  You could say:
> >
> > Ignore a potential #UD failut during emergency VMXOFF ...
> >
> > When a cpu may be in VMX ...
> >
> >>
> >> Fixes: 208067 <https://bugzilla.kernel.org/show_bug.cgi?id=208067>
> >> Reported-by: David P. Reed <dpreed@deepplum.com>
> >
> > It's not really necessary to say that you, the author, reported the
> > problem, but I guess it's harmless.
> >
> >> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> >> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >> Suggested-by: Andy Lutomirski <luto@kernel.org>
> >> Signed-off-by: David P. Reed <dpreed@deepplum.com>
> >> ---
> >>  arch/x86/include/asm/virtext.h | 20 ++++++++++++++------
> >>  1 file changed, 14 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
> >> index 0ede8d04535a..0e0900eacb9c 100644
> >> --- a/arch/x86/include/asm/virtext.h
> >> +++ b/arch/x86/include/asm/virtext.h
> >> @@ -30,11 +30,11 @@ static inline int cpu_has_vmx(void)
> >>  }
> >>
> >>
> >> -/* Disable VMX on the current CPU
> >> +/* Exit VMX root mode and isable VMX on the current CPU.
> >
> > s/isable/disable/
> >
> >
> >>  /* Disable VMX if it is supported and enabled on the current CPU
> >> --
> >> 2.26.2
> >>
> >
> > Other than that:
> >
> > Reviewed-by: Andy Lutomirski <luto@kernel.org>
>
> As a newbie, I have a process question - should I resend the patch with the 'Reviewed-by' line, as well as correcting the other wording? Thanks!

Probably.  Sometimes a maintainer will apply the patch and make these
types of cosmetic changes, but it's easier if you resubmit.  That
being said, for non-urgent patches, it's usually considered polite to
wait a day or two to give other people a chance to comment.

--Andy

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic
  2020-07-05 20:55                 ` Andy Lutomirski
@ 2020-07-05 22:07                   ` David P. Reed
  0 siblings, 0 replies; 23+ messages in thread
From: David P. Reed @ 2020-07-05 22:07 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Andy Lutomirski, Sean Christopherson, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, X86 ML, H. Peter Anvin,
	Allison Randal, Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
	Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Alexandre Chartre, Jann Horn,
	Dave Hansen, LKML



On Sunday, July 5, 2020 4:55pm, "Andy Lutomirski" <luto@kernel.org> said:

> On Sun, Jul 5, 2020 at 12:52 PM David P. Reed <dpreed@deepplum.com> wrote:
>>
>> Thanks, will handle these. 2 questions below.
>>
>> On Sunday, July 5, 2020 2:22pm, "Andy Lutomirski" <luto@kernel.org> said:
>>
>> > On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <dpreed@deepplum.com> wrote:
>> >>
>> >> Fix: Mask undefined operation fault during emergency VMXOFF that must be
>> >> attempted to force cpu exit from VMX root operation.
>> >> Explanation: When a cpu may be in VMX root operation (only possible when
>> >> CR4.VMXE is set), crash or panic reboot tries to exit VMX root operation
>> >> using VMXOFF. This is necessary, because any INIT will be masked while cpu
>> >> is in VMX root operation, but that state cannot be reliably
>> >> discerned by the state of the cpu.
>> >> VMXOFF faults if the cpu is not actually in VMX root operation, signalling
>> >> undefined operation.
>> >> Discovered while debugging an out-of-tree x-visor with a race. Can happen
>> >> due to certain kinds of bugs in KVM.
>> >
>> > Can you re-wrap lines to 68 characters?  Also, the Fix: and
>>
>> I used 'scripts/checkpatch.pl' and it had me wrap to 75 chars:
>> "WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per
>> line)"
>>
>> Should I submit a fix to checkpatch.pl to say 68?
> 
> 75 is probably fine too, but something is odd about your wrapping.
> You have long lines mostly alternating with short lines.  It's as if
> you wrote 120-ish character lines and then wrapped to 75 without
> reflowing.
My emacs settings tend to wrap at about 85 depending on file type (big screens). I did the shortening manually, aimed at breaking at meaningful points, not worrying too much about
line-length uniformity.

> 
>>
>> > Explanation: is probably unnecessary.  You could say:
>> >
>> > Ignore a potential #UD failut during emergency VMXOFF ...
>> >
>> > When a cpu may be in VMX ...
>> >
>> >>
>> >> Fixes: 208067 <https://bugzilla.kernel.org/show_bug.cgi?id=208067>
>> >> Reported-by: David P. Reed <dpreed@deepplum.com>
>> >
>> > It's not really necessary to say that you, the author, reported the
>> > problem, but I guess it's harmless.
>> >
>> >> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
>> >> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> >> Suggested-by: Andy Lutomirski <luto@kernel.org>
>> >> Signed-off-by: David P. Reed <dpreed@deepplum.com>
>> >> ---
>> >>  arch/x86/include/asm/virtext.h | 20 ++++++++++++++------
>> >>  1 file changed, 14 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
>> >> index 0ede8d04535a..0e0900eacb9c 100644
>> >> --- a/arch/x86/include/asm/virtext.h
>> >> +++ b/arch/x86/include/asm/virtext.h
>> >> @@ -30,11 +30,11 @@ static inline int cpu_has_vmx(void)
>> >>  }
>> >>
>> >>
>> >> -/* Disable VMX on the current CPU
>> >> +/* Exit VMX root mode and isable VMX on the current CPU.
>> >
>> > s/isable/disable/
>> >
>> >
>> >>  /* Disable VMX if it is supported and enabled on the current CPU
>> >> --
>> >> 2.26.2
>> >>
>> >
>> > Other than that:
>> >
>> > Reviewed-by: Andy Lutomirski <luto@kernel.org>
>>
>> As a newbie, I have a process question - should I resend the patch with the
>> 'Reviewed-by' line, as well as correcting the other wording? Thanks!
> 
> Probably.  Sometimes a maintainer will apply the patch and make these
> types of cosmetic changes, but it's easier if you resubmit.  That
> being said, for non-urgent patches, it's usually considered polite to
> wait a day or two to give other people a chance to comment.

I'm not sure which maintainer will move the patches along. I am waiting for additional input, but will resubmit in a day or two.

> 
> --Andy
> 



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic
  2020-07-04 20:38           ` [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic David P. Reed
  2020-07-05 18:22             ` Andy Lutomirski
@ 2020-07-07  5:09             ` Sean Christopherson
  2020-07-07 19:09               ` David P. Reed
  1 sibling, 1 reply; 23+ messages in thread
From: Sean Christopherson @ 2020-07-07  5:09 UTC (permalink / raw)
  To: David P. Reed
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	X86 ML, H. Peter Anvin, Allison Randal, Enrico Weigelt,
	Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML

On Sat, Jul 04, 2020 at 04:38:08PM -0400, David P. Reed wrote:
> Fix: Mask undefined operation fault during emergency VMXOFF that must be
> attempted to force cpu exit from VMX root operation.
> Explanation: When a cpu may be in VMX root operation (only possible when
> CR4.VMXE is set), crash or panic reboot tries to exit VMX root operation
> using VMXOFF. This is necessary, because any INIT will be masked while cpu
> is in VMX root operation, but that state cannot be reliably
> discerned by the state of the cpu.
> VMXOFF faults if the cpu is not actually in VMX root operation, signalling
> undefined operation.
> Discovered while debugging an out-of-tree x-visor with a race. Can happen
> due to certain kinds of bugs in KVM.
> 
> Fixes: 208067 <https://bugzilla.kernel.org/show_bug.cgi?id=208067>
> Reported-by: David P. Reed <dpreed@deepplum.com>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Suggested-by: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: David P. Reed <dpreed@deepplum.com>
> ---
>  arch/x86/include/asm/virtext.h | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
> index 0ede8d04535a..0e0900eacb9c 100644
> --- a/arch/x86/include/asm/virtext.h
> +++ b/arch/x86/include/asm/virtext.h
> @@ -30,11 +30,11 @@ static inline int cpu_has_vmx(void)
>  }
>  
>  
> -/* Disable VMX on the current CPU
> +/* Exit VMX root mode and isable VMX on the current CPU.
>   *
>   * vmxoff causes a undefined-opcode exception if vmxon was not run
> - * on the CPU previously. Only call this function if you know VMX
> - * is enabled.
> + * on the CPU previously. Only call this function if you know cpu
> + * is in VMX root mode.
>   */
>  static inline void cpu_vmxoff(void)
>  {
> @@ -47,14 +47,22 @@ static inline int cpu_vmx_enabled(void)
>  	return __read_cr4() & X86_CR4_VMXE;
>  }
>  
> -/* Disable VMX if it is enabled on the current CPU
> +/* Safely exit VMX root mode and disable VMX if VMX enabled
> + * on the current CPU. Handle undefined-opcode fault
> + * that can occur if cpu is not in VMX root mode, due
> + * to a race.
>   *
>   * You shouldn't call this if cpu_has_vmx() returns 0.
>   */
>  static inline void __cpu_emergency_vmxoff(void)
>  {
> -	if (cpu_vmx_enabled())
> -		cpu_vmxoff();
> +	if (!cpu_vmx_enabled())
> +		return;
> +	asm volatile ("1:vmxoff\n\t"
> +		      "2:\n\t"
> +		      _ASM_EXTABLE(1b, 2b)
> +		      ::: "cc", "memory");
> +	cr4_clear_bits(X86_CR4_VMXE);

Open coding vmxoff doesn't make sense, and IMO is flat out wrong as it fixes
flows that use __cpu_emergency_vmxoff() but leaves the same bug hanging
around in emergency_vmx_disable_all() until the next patch.

The reason I say it doesn't make sense is that there is no sane scenario
where the generic vmxoff helper should _not_ eat the fault.  All other VMXOFF
faults are mode related, i.e. any fault is guaranteed to be due to the
!post-VMXON check unless we're magically in RM, VM86, compat mode, or at
CPL>0.  Given that the whole point of this series is that it's impossible to
determine whether or not the CPU if post-VMXON if CR4.VMXE=1 without taking a
fault of some form, there's simply no way that anything except the hypervisor
(in normal operation) can know the state of VMX.  And given that the only
in-tree hypervisor (KVM) has its own version of vmxoff, that means there is
no scenario in which cpu_vmxoff() can safely be used.  Case in point, after
the next patch there are no users of cpu_vmxoff().

TL;DR: Just do fixup on cpu_vmxoff().

>  }
>  
>  /* Disable VMX if it is supported and enabled on the current CPU
> -- 
> 2.26.2
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 3/3] Force all cpus to exit VMX root operation on crash/panic reliably
  2020-07-05 20:53                 ` Andy Lutomirski
@ 2020-07-07  5:29                   ` Sean Christopherson
  0 siblings, 0 replies; 23+ messages in thread
From: Sean Christopherson @ 2020-07-07  5:29 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: David P. Reed, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	X86 ML, H. Peter Anvin, Allison Randal, Enrico Weigelt,
	Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Alexandre Chartre, Jann Horn,
	Dave Hansen, LKML

On Sun, Jul 05, 2020 at 01:53:39PM -0700, Andy Lutomirski wrote:
> On Sun, Jul 5, 2020 at 1:00 PM David P. Reed <dpreed@deepplum.com> wrote:
> >
> > On Sunday, July 5, 2020 2:26pm, "Andy Lutomirski" <luto@kernel.org> said:
> > > As a minor caveat, doing cr4_clear_bits() in NMI context is not really
> > > okay, but we're about to reboot, so nothing too awful should happen.
> > > And this has very little to do with your patch.
> >
> > I had wondered why the bit is cleared, too. (I assumed it was OK or
> > desirable, because it was being cleared in NMI context before). Happy to
> > submit a separate patch to eliminate that issue as well, since the point of
> > emergency vmxoff is only to get out of VMX root mode - CR4.VMXE's state is
> > irrelevant. Of course, clearing it prevents any future emergency vmxoff
> > attempts. (there seemed to be some confusion about "enabling" VMX vs. "in
> > VMX operation" in the comments)  Should I?
> 
> I have a vague recollection of some firmwares that got upset if rebooted with
> CR4.VMXE set.  Sean?

Hmm, rebooting with CR4.VMXE=1 shouldn't be a problem.  VMXON does all the
special stuff that causes problems with reboot, e.g. blocks INIT, prevents
disabling paging, etc...

That being said, I think it makes sense to keep the clearing of CR4.VMXE out
of paranoia as BIOS will be BIOS, and there is no real downside.  Not only
is the system about to reboot, but the CPUs that call cr4_clear_bits() from
NMI context are also being put into an infinite loop by crash_nmi_callback(),
i.e. they never leave NMI context.  And we rely on that behavior, otherwise
KVM could set CR4.VMXE and do VMXON after the NMI and the whole thing would
be for naught.
 
> The real issue here is that the percpu CR4 machinery uses IRQ-offness as a
> lock, and NMI breaks this.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic
  2020-07-07  5:09             ` Sean Christopherson
@ 2020-07-07 19:09               ` David P. Reed
  2020-07-07 19:24                 ` Sean Christopherson
  0 siblings, 1 reply; 23+ messages in thread
From: David P. Reed @ 2020-07-07 19:09 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	X86 ML, H. Peter Anvin, Allison Randal, Enrico Weigelt,
	Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML



On Tuesday, July 7, 2020 1:09am, "Sean Christopherson" <sean.j.christopherson@intel.com> said:

> On Sat, Jul 04, 2020 at 04:38:08PM -0400, David P. Reed wrote:
>> Fix: Mask undefined operation fault during emergency VMXOFF that must be
>> attempted to force cpu exit from VMX root operation.
>> Explanation: When a cpu may be in VMX root operation (only possible when
>> CR4.VMXE is set), crash or panic reboot tries to exit VMX root operation
>> using VMXOFF. This is necessary, because any INIT will be masked while cpu
>> is in VMX root operation, but that state cannot be reliably
>> discerned by the state of the cpu.
>> VMXOFF faults if the cpu is not actually in VMX root operation, signalling
>> undefined operation.
>> Discovered while debugging an out-of-tree x-visor with a race. Can happen
>> due to certain kinds of bugs in KVM.
>>
>> Fixes: 208067 <https://bugzilla.kernel.org/show_bug.cgi?id=208067>
>> Reported-by: David P. Reed <dpreed@deepplum.com>
>> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
>> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> Suggested-by: Andy Lutomirski <luto@kernel.org>
>> Signed-off-by: David P. Reed <dpreed@deepplum.com>
>> ---
>>  arch/x86/include/asm/virtext.h | 20 ++++++++++++++------
>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
>> index 0ede8d04535a..0e0900eacb9c 100644
>> --- a/arch/x86/include/asm/virtext.h
>> +++ b/arch/x86/include/asm/virtext.h
>> @@ -30,11 +30,11 @@ static inline int cpu_has_vmx(void)
>>  }
>>
>>
>> -/* Disable VMX on the current CPU
>> +/* Exit VMX root mode and isable VMX on the current CPU.
>>   *
>>   * vmxoff causes a undefined-opcode exception if vmxon was not run
>> - * on the CPU previously. Only call this function if you know VMX
>> - * is enabled.
>> + * on the CPU previously. Only call this function if you know cpu
>> + * is in VMX root mode.
>>   */
>>  static inline void cpu_vmxoff(void)
>>  {
>> @@ -47,14 +47,22 @@ static inline int cpu_vmx_enabled(void)
>>  	return __read_cr4() & X86_CR4_VMXE;
>>  }
>>
>> -/* Disable VMX if it is enabled on the current CPU
>> +/* Safely exit VMX root mode and disable VMX if VMX enabled
>> + * on the current CPU. Handle undefined-opcode fault
>> + * that can occur if cpu is not in VMX root mode, due
>> + * to a race.
>>   *
>>   * You shouldn't call this if cpu_has_vmx() returns 0.
>>   */
>>  static inline void __cpu_emergency_vmxoff(void)
>>  {
>> -	if (cpu_vmx_enabled())
>> -		cpu_vmxoff();
>> +	if (!cpu_vmx_enabled())
>> +		return;
>> +	asm volatile ("1:vmxoff\n\t"
>> +		      "2:\n\t"
>> +		      _ASM_EXTABLE(1b, 2b)
>> +		      ::: "cc", "memory");
>> +	cr4_clear_bits(X86_CR4_VMXE);
> 
> Open coding vmxoff doesn't make sense, and IMO is flat out wrong as it fixes
> flows that use __cpu_emergency_vmxoff() but leaves the same bug hanging
> around in emergency_vmx_disable_all() until the next patch.
> 
> The reason I say it doesn't make sense is that there is no sane scenario
> where the generic vmxoff helper should _not_ eat the fault.  All other VMXOFF
> faults are mode related, i.e. any fault is guaranteed to be due to the
> !post-VMXON check unless we're magically in RM, VM86, compat mode, or at
> CPL>0.  Given that the whole point of this series is that it's impossible to
> determine whether or not the CPU if post-VMXON if CR4.VMXE=1 without taking a
> fault of some form, there's simply no way that anything except the hypervisor
> (in normal operation) can know the state of VMX.  And given that the only
> in-tree hypervisor (KVM) has its own version of vmxoff, that means there is
> no scenario in which cpu_vmxoff() can safely be used.  Case in point, after
> the next patch there are no users of cpu_vmxoff().
> 
> TL;DR: Just do fixup on cpu_vmxoff().

Personally, I don't care either way, since it fixes the bug either way (and it's inlined, so either way no additional code is generated. I was just being conservative since the cpu_vmxoff() is exported throughout the kernel source, so it might be expected to stay the same (when not in an "emergency"). I'll wait a day or two for any objections to just doing the fix in cpu_vmxoff() by other commenters. WIth no objection, I'll just do it that way.

Sean, are you the one who would get this particular fix pushed into Linus's tree, by the way? The "maintainership" is not clear to me. If you are, happy to take direction from you as the primary input.

> 
>>  }
>>
>>  /* Disable VMX if it is supported and enabled on the current CPU
>> --
>> 2.26.2
>>
> 



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic
  2020-07-07 19:09               ` David P. Reed
@ 2020-07-07 19:24                 ` Sean Christopherson
  2020-07-07 19:52                   ` David P. Reed
  0 siblings, 1 reply; 23+ messages in thread
From: Sean Christopherson @ 2020-07-07 19:24 UTC (permalink / raw)
  To: David P. Reed
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	X86 ML, H. Peter Anvin, Allison Randal, Enrico Weigelt,
	Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML

On Tue, Jul 07, 2020 at 03:09:38PM -0400, David P. Reed wrote:
> 
> On Tuesday, July 7, 2020 1:09am, "Sean Christopherson" <sean.j.christopherson@intel.com> said:
> Sean, are you the one who would get this particular fix pushed into Linus's
> tree, by the way? The "maintainership" is not clear to me.

Nope, I'm just here to complain and nitpick :-)  There's no direct maintainer
for virtext.h so it falls under the higher level arch/x86 umbrella, i.e. I
expect Boris/Thomas/Ingo will pick this up.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic
  2020-07-07 19:24                 ` Sean Christopherson
@ 2020-07-07 19:52                   ` David P. Reed
  0 siblings, 0 replies; 23+ messages in thread
From: David P. Reed @ 2020-07-07 19:52 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	X86 ML, H. Peter Anvin, Allison Randal, Enrico Weigelt,
	Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
	Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
	Jann Horn, Dave Hansen, LKML



On Tuesday, July 7, 2020 3:24pm, "Sean Christopherson" <sean.j.christopherson@intel.com> said:

> On Tue, Jul 07, 2020 at 03:09:38PM -0400, David P. Reed wrote:
>>
>> On Tuesday, July 7, 2020 1:09am, "Sean Christopherson"
>> <sean.j.christopherson@intel.com> said:
>> Sean, are you the one who would get this particular fix pushed into Linus's
>> tree, by the way? The "maintainership" is not clear to me.
> 
> Nope, I'm just here to complain and nitpick :-)  There's no direct maintainer
> for virtext.h so it falls under the higher level arch/x86 umbrella, i.e. I
> expect Boris/Thomas/Ingo will pick this up.
> 
Thanks for your time and effort in helping.


^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2020-07-07 19:52 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-25 14:45 [PATCH v2] Fix undefined operation VMXOFF during reboot and crash David P. Reed
2020-06-25 14:59 ` David P. Reed
2020-06-29 20:54   ` David P. Reed
2020-06-29 21:22     ` Andy Lutomirski
2020-06-29 21:49       ` Sean Christopherson
2020-06-29 22:46         ` David P. Reed
2020-07-04 20:38         ` [PATCH v3 0/3] " David P. Reed
2020-07-04 20:38           ` [PATCH v3 1/3] Correct asm VMXOFF side effects David P. Reed
2020-07-05  5:46             ` Randy Dunlap
2020-07-04 20:38           ` [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic David P. Reed
2020-07-05 18:22             ` Andy Lutomirski
2020-07-05 19:52               ` David P. Reed
2020-07-05 20:55                 ` Andy Lutomirski
2020-07-05 22:07                   ` David P. Reed
2020-07-07  5:09             ` Sean Christopherson
2020-07-07 19:09               ` David P. Reed
2020-07-07 19:24                 ` Sean Christopherson
2020-07-07 19:52                   ` David P. Reed
2020-07-04 20:38           ` [PATCH v3 3/3] Force all cpus to exit VMX root operation on crash/panic reliably David P. Reed
2020-07-05 18:26             ` Andy Lutomirski
2020-07-05 20:00               ` David P. Reed
2020-07-05 20:53                 ` Andy Lutomirski
2020-07-07  5:29                   ` Sean Christopherson

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).