Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic

[Andy Lutomirski <luto@kernel.org>]

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