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
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
