Re: kernel device reset support

Dong, Eddie
2007-10-09T01:58:22Z

Avi Kivity wrote:
> Dong, Eddie wrote:
>> Kernel side patch to introduce a new API for kernel device reset and
>> force vcpu mp_state to UNINATIALIZED state if it is reset.
>> thx,eddie
>>
>>
>>
>> +
>> +/*
>> + * Reset VM.
>> + *
>> + */
>> +int kvm_vm_reset(struct kvm *kvm)
>> +{
>> + struct kvm_vcpu *vcpu;
>> + int i;
>> +
>> + kvm_reset_devices(kvm);
>> + for (i = 0; i < KVM_MAX_VCPUS; i++) {
>> + vcpu = kvm->vcpus[i];
>> + if (!vcpu)
>> + continue;
>> + /* active VCPU */
>> + if (vcpu->vcpu_id)
>> + vcpu->mp_state = VCPU_MP_STATE_UNINITIALIZED;
>> + else {
>> + vcpu->mp_state = VCPU_MP_STATE_RUNNABLE;
>> + kvm_lapic_reset(vcpu);
>>
>
> Why is the handling here different?

In native, RESET signal force every processor enter "RESET" status, and
then immediately after RESET signal is removed, all CPUs will compete
for BSP role, the winner continue execution, and failor will be blocked
till INIT/SIPI/SIPI.

>
>> + }
>> + vcpu->force_to_quit = 1;
>> + kvm_vcpu_kick(vcpu);
>> + }
>> + return 0;
>> +}
>> +
>> static long kvm_vm_ioctl(struct file *filp,
>> unsigned int ioctl, unsigned long arg)
>> {
>> @@ -3163,6 +3200,12 @@ static long kvm_vm_ioctl(struct file *filp,
>> else goto out;
>> break;
>> + case KVM_RESET:
>> + r = -EFAULT;
>>
>
> -EINVAL. Or better accept it always and only reset the vcpu if
> !irqchip_in_kernel().

Fixed.

>
>> + if (!irqchip_in_kernel(kvm))
>> + goto out;
>> + r = kvm_vm_reset(kvm);
>> + break;
>> case KVM_IRQ_LINE: {
>> struct kvm_irq_level irq_event;
>>
>> diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
>> index 3d8e01e..0799aad 100644
>> --- a/drivers/kvm/vmx.c
>> +++ b/drivers/kvm/vmx.c
>> @@ -1860,6 +1860,10 @@ static int handle_external_interrupt(struct
>> kvm_vcpu *vcpu, struct kvm_run
*kvm_run)
>> {
>> ++vcpu->stat.irq_exits;
>> + if (vcpu->force_to_quit) {
>> + vcpu->force_to_quit = 0;
>> + return -EINTR;
>> + }
>> return 1;
>> }
>>
>
> Why is this needed?

For a graceful reboot, this one is not needed since every APs are
already brought to HALT status before BSP issue RESET signal. But in
case of non-graceful reboot, it is possible the VCPUs are still
executing guest instruction, so we kick the VCPU and then use this logic
to force the exception handler to be a heavy VM Exit and execute
following code at beginning of kvm_vcpu_ioctl_run. (Let
VCPU_MP_STATE_UNINITIALIZED take effective)

if (unlikely(vcpu->mp_state == VCPU_MP_STATE_UNINITIALIZED)) {
if (irqchip_in_kernel(vcpu->kvm) && vcpu->apic)
kvm_lapic_reset(vcpu);
kvm_vcpu_block(vcpu);
vcpu_put(vcpu);
return -EAGAIN;
}

Whether we need to handle those un-graceful reboot case is up to you :-)
If not, then those code can be removed.

>
> Userspace can always force an exit by sending a signal.
>
>> +/*
>> + * Reset VM.
>> + *
>> + */
>> +int kvm_vm_reset(struct kvm *kvm)
>> +{
>> + struct kvm_vcpu *vcpu;
>> + int i;
>> +
>> + kvm_reset_devices(kvm);
>>
>
> Need to take kvm->lock around this.

Mmm, I will move this to be after VCPU reset.
If a VCPU is still accessing (write) devices register, we always have
problem.
So move after all the processors enetring frozen state will be simple
and safer.

Any opnion? Will post after your new comments.

thx,eddie