Re: [PATCH 0/2] kvm: disable virtualization on kdump

[Eduardo Habkost <ehabkost-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

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