On Mon, Oct 27, 2008 at 11:13:41AM +0200, Avi Kivity wrote:
> Eric W. Biederman wrote:
>>> NMI IPIs are already used on x86 native_machine_crash_shutdown(), so
>>> it wouldn't get more messy that it is currently. We just need to add
>>> another bit of code to the code that already runs on an NMI handler.
>>>
>>
>> Yes. And handling of those NMIs is best effort. Nothing fails if
>> they don't actually run.
>>
>>
>
> Unless someone can come up with another way to disable vmx remotely,
> that's going to change if you have vmx enabled.
>
>> Well we could fairly easily have a non-modular function that does.
>> if (vmx_present && vmx_enabled) {
>> turn_off_vmx();
>> }
>>
>> Which at first skim looks like it is all of about 10-20 machine
>> instructions.
>>
>>
>
> There's no way to query whether vmx is enabled or disabled, AFAICT. So
> we have to execute vmxoff and ignore possible #UDs.
Oops. This means the notifier my patches add would break, if vmx is
disabled on any CPU.
Can't we just set a flag when we are about to enable vmx, so we run vmxoff
only when know it's enabled? There will be a tiny window between setting
this flag and and actually running vmxon where things could go wrong,
but this doesn't look that bad.
Having to handle #UD would make things more messy, in my opinion.
BTW, is this problem vmx-specific? Do we need to do something similar
for svm?
>
> If we trust the exception handlers, there's no problem. Otherwise we
> need to replace the current #UD handler with an iret (perhaps switching
> temporarily to another IDT).
I think we can't fully trust anything if we are on the crash dump path,
so the less code we depend on, the better.
>
>> There are a few real places where we need code on the kdump
>> path because there it is not possible to do the work any
>> other way. However we need to think long and hard about
>> that because placing the code anywhere besides in a broken
>> and failing kernel is going to be easier to maintain and
>> more reliable.
>>
>
> vmx blocking INITs makes it impossible to leave this to the new kernel.
>
>> I oppose an atomic notifier because it makes the review
>> essentially impossible. If any module can come in and register
>> a notifier we can't know what code is running on that code
>> path and we can't be certain the code is safe in an abnormal
>> case to run on that code path.
>>
>
> What if it's a specialized notifier for kexec? Or even kexec_crash?
The patches I've sent to the kvm mailing list added a notifier interface
specific for kexec_crash, using raw_notifier_*().
IMO, if a notifier registration interface was acceptable, the raw
notifiers would be good enough for that. But Eric seems to think that
adding a notifier registration interface for the crash handler path
wouldn't be a good idea, and I am starting to agree with him.
>
> That said, I have no issue with static code at the call site.
>
>> Right now we only need to support vmx on the kdump path because
>> of what appears to be a hardware design bug. Enabling vmx
>> apparently disables standard functions like an INIT IPI. Things
>> like this do happen but they should be rare.
>>
>
> The general kexec path also wants this fixed.
When I've tested it, kexec called the kvm reboot notifier, so
everything worked fine.
--
Eduardo
