Re: patch for review: kvm: implement guest rebooting

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Leonard Norrgard wrote:
> I've been looking at the guest reboot problem, which currently reboots
> the host on svm when a Linux guest issues "# shutdown -r now".
>   

Obviously a major issue, which we've neglected.  Thanks for addressing it.

> The patch below stops the rebooting of the host and handles the request
> to reboot the guest in an orderly fashion, so it's a big step forward,
> but it doesn't yet succeed in rebooting of the guest, instead the VM exits.
>
> The basic approach is to intercept the shutdown. The patch includes some
> possible alternatives to reboot, while searching for the best approach:
>
> 1) Call kvm_qemu_destroy(); and immediately after kvm_qemu_create_context();
>   

I don't like this approach.  It means memory needs to be deallocated and 
reallocated.  There's some likelyhood of the reallocation failing, so a 
reboot can turn into a dead VM.  Memory contents is not preserved, while 
a real reset does preserve memory.

More fundamentally, a real cpu can undergo a reset, and so should a 
virtual cpu.


> 2) New ioctl to reset a vcpu
>   

Definitely the way to go.  It'll possibly be needed for guest smp too.

> 3) I also tried calling init_vmcb from shutdown_intercept, but that had
> no noticable effect
>   

I think "shutdown" in this context is processor shutdown, not a vcpu 
reset.  IIRC a processor shutdown can be caused by invalid state or 
hardware errors.

We should intercept it, but it's not that important IMO.  (We should 
also find out what it means exactly)

> Method 1 seems promising, but currently results in kvm exiting due to
> "do_interrupt: unexpect" errors.
>
> Method 2 currently results in vmrun returning KVM_EXIT_TYPE_FAIL_ENTRY.
>
>   

Looks like not everything was initialized correctly...

> - Both of methods 1 and 2 need an API version bump
> - This code is safe to run without having to fear a host reboot
> - SVM only, at the moment
> - svm_reset_vcpu() was initially just the one-line call to init_vmcb(),
> there's some leftover cruft now...
> - A printf has been added to cpu_reset(), if this prints, qemu has
> called all registered reset handlers.
>
> Please comment. The Patch is relative to kvm-11.
>
>
>   
> ------------------------------------------------------------------------
>
> Index: kernel/kvm_main.c
> ===================================================================
> --- kernel/kvm_main.c	(revision 2211)
> +++ kernel/kvm_main.c	(working copy)
> @@ -1764,6 +1764,23 @@
>  	return r;
>  }
>  
> +static int kvm_dev_ioctl_reset_vcpu(struct kvm *kvm, struct kvm_reset_vcpu *slot)
> +{
> +	struct kvm_vcpu *vcpu;
> +
> +	if (!valid_vcpu(slot->vcpu))
> +		return -EINVAL;
> +	vcpu = vcpu_load(kvm, slot->vcpu);
> +	if (!vcpu)
> +		return -ENOENT;
> +
> +	kvm_arch_ops->reset_vcpu(vcpu);
> +
> +	vcpu_put(vcpu);
> +
> +	return 0;
> +}
> +
>  static long kvm_dev_ioctl(struct file *filp,
>  			  unsigned int ioctl, unsigned long arg)
>  {
> @@ -1780,6 +1797,13 @@
>  			goto out;
>  		break;
>  	}
> +	case KVM_RESET_VCPU: {
> +		struct kvm_reset_vcpu kvm_reset_vcpu;
>   

Empty line here.

> +		if (copy_from_user(&kvm_reset_vcpu, (void *)arg, sizeof kvm_reset_vcpu))
> +			goto out;
> +		r = kvm_dev_ioctl_reset_vcpu(kvm, &kvm_reset_vcpu);
> +		break;
> +	}
>  	case KVM_RUN: {
>  		struct kvm_run kvm_run;
>  
> Index: kernel/include/linux/kvm.h
> ===================================================================
> --- kernel/include/linux/kvm.h	(revision 2211)
> +++ kernel/include/linux/kvm.h	(working copy)
> @@ -11,7 +11,7 @@
>  #include <asm/types.h>
>  #include <linux/ioctl.h>
>  
> -#define KVM_API_VERSION 2
> +#define KVM_API_VERSION 3
>  
>  /*
>   * Architectural interrupt line count, and the size of the bitmap needed
> @@ -46,6 +46,7 @@
>  	KVM_EXIT_HLT              = 5,
>  	KVM_EXIT_MMIO             = 6,
>  	KVM_EXIT_IRQ_WINDOW_OPEN  = 7,
> +	KVM_EXIT_SHUTDOWN         = 8,
>  };
>  
>  /* for KVM_RUN */
> @@ -218,6 +219,12 @@
>  	};
>  };
>  
> +/* for KVM_RESET_VCPU */
> +struct kvm_reset_vcpu {
> +	/* in */
> +	__u32 vcpu;
> +};
> +
>   

Why define the structure?  Since ioctl() passes one argument, it can be 
the vcpu number.


>  #define KVMIO 0xAE
>  
>  #define KVM_GET_API_VERSION       _IO(KVMIO, 1)
> @@ -235,5 +242,6 @@
>  #define KVM_GET_MSRS              _IOWR(KVMIO, 13, struct kvm_msrs)
>  #define KVM_SET_MSRS              _IOWR(KVMIO, 14, struct kvm_msrs)
>  #define KVM_GET_MSR_INDEX_LIST    _IOWR(KVMIO, 15, struct kvm_msr_list)
> +#define KVM_RESET_VCPU            _IO(KVMIO, 16)
>   

If you're passing a structure, this needs to be _IOR(KVMIO, 16, struct 
kvm_reset_vcpu).


> @@ -582,6 +583,21 @@
>  	return r;
>  }
>  
> +static void svm_reset_vcpu(struct kvm_vcpu *vcpu)
> +{
> +	printk("....svm_reset_vcpu\n");
> +	memset(vcpu->svm->vmcb, 0, PAGE_SIZE);
> +	//vcpu->svm->vmcb_pa = page_to_pfn(page) << PAGE_SHIFT;
> +	vcpu->svm->cr0 = 0x00000010;
> +	vcpu->svm->asid_generation = 0;
> +	memset(vcpu->svm->db_regs, 0, sizeof(vcpu->svm->db_regs));
> +	init_vmcb(vcpu->svm->vmcb);
> +
> +	fx_init(vcpu);
> +
> +	init_vmcb(vcpu->svm->vmcb);
> +}
>   

You will need to reset the mmu context as well, as we're transitioning 
from protected mode to real mode.  Look at the set_sregs ioctl for an 
example.  This should be done from common code.

Perhaps init_vmcb relies on the save area being cleared.  Try zeroing 
vmcb->save in init_vmcb().

>  
> @@ -1690,6 +1715,8 @@
>  	.run = svm_vcpu_run,
>  	.skip_emulated_instruction = skip_emulated_instruction,
>  	.vcpu_setup = svm_vcpu_setup,
> +
> +	.reset_vcpu = svm_reset_vcpu,
>  };
>   

It would be nice to call reset_vcpu() as part of the normal vcpu 
creating sequence, to avoid having a rarely-used code path.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV