Re: [PATCH 6/9] Add VMRUN handler v5

[Muli Ben-Yehuda <muli@il.ibm.com>]

On Mon, Oct 20, 2008 at 07:04:47PM +0200, Alexander Graf wrote:

> +static int nested_svm_vmrun(struct vcpu_svm *svm, void *arg1,
> +			    void *arg2, void *opaque)
> +{
> +	struct vmcb *nested_vmcb = (struct vmcb *)arg1;
> +	struct vmcb *hsave = svm->hsave;
> +
> +	/* nested_vmcb is our indicator if nested SVM is activated */
> +	svm->nested_vmcb = svm->vmcb->save.rax;
> +
> +	/* Clear internal status */
> +	svm->vcpu.arch.exception.pending = false;
> +
> +	/* Save the old vmcb, so we don't need to pick what we save, but
> +	   can restore everything when a VMEXIT occurs */
> +	memcpy(hsave, svm->vmcb, sizeof(struct vmcb));
> +	/* We need to remember the original CR3 in the SPT case */
> +	if (!npt_enabled)
> +		hsave->save.cr3 = svm->vcpu.arch.cr3;
> +	hsave->save.rip = svm->next_rip;
> +
> +	if (svm->vmcb->save.rflags & X86_EFLAGS_IF)
> +		svm->vcpu.arch.hflags |= HF_HIF_MASK;
> +	else
> +		svm->vcpu.arch.hflags &= ~HF_HIF_MASK;
> +
> +	/* Load the nested guest state */
> +	svm->vmcb->save.es = nested_vmcb->save.es;
> +	svm->vmcb->save.cs = nested_vmcb->save.cs;
> +	svm->vmcb->save.ss = nested_vmcb->save.ss;
> +	svm->vmcb->save.ds = nested_vmcb->save.ds;
> +	svm->vmcb->save.gdtr = nested_vmcb->save.gdtr;
> +	svm->vmcb->save.idtr = nested_vmcb->save.idtr;
> +	svm->vmcb->save.rflags = nested_vmcb->save.rflags;
> +	svm_set_efer(&svm->vcpu, nested_vmcb->save.efer);
> +	svm_set_cr0(&svm->vcpu, nested_vmcb->save.cr0);
> +	svm_set_cr4(&svm->vcpu, nested_vmcb->save.cr4);
> +	if (npt_enabled) {
> +		svm->vmcb->save.cr3 = nested_vmcb->save.cr3;
> +		svm->vcpu.arch.cr3 = nested_vmcb->save.cr3;
> +	} else {
> +		kvm_set_cr3(&svm->vcpu, nested_vmcb->save.cr3);
> +		kvm_mmu_reset_context(&svm->vcpu);
> +	}
> +	svm->vmcb->save.cr2 = nested_vmcb->save.cr2;
> +	kvm_register_write(&svm->vcpu, VCPU_REGS_RAX, nested_vmcb->save.rax);
> +	kvm_register_write(&svm->vcpu, VCPU_REGS_RSP, nested_vmcb->save.rsp);
> +	kvm_register_write(&svm->vcpu, VCPU_REGS_RIP, nested_vmcb->save.rip);
> +	/* In case we don't even reach vcpu_run, the fields are not updated */
> +	svm->vmcb->save.rax = nested_vmcb->save.rax;
> +	svm->vmcb->save.rsp = nested_vmcb->save.rsp;
> +	svm->vmcb->save.rip = nested_vmcb->save.rip;
> +	svm->vmcb->save.dr7 = nested_vmcb->save.dr7;
> +	svm->vmcb->save.dr6 = nested_vmcb->save.dr6;
> +	svm->vmcb->save.cpl = nested_vmcb->save.cpl;
> +
> +	/* We don't want a nested guest to be more powerful than the guest,
> +	   so all intercepts are ORed */
> +	svm->vmcb->control.intercept_cr_read |=
> +		nested_vmcb->control.intercept_cr_read;
> +	svm->vmcb->control.intercept_cr_write |=
> +		nested_vmcb->control.intercept_cr_write;
> +	svm->vmcb->control.intercept_dr_read |=
> +		nested_vmcb->control.intercept_dr_read;
> +	svm->vmcb->control.intercept_dr_write |=
> +		nested_vmcb->control.intercept_dr_write;
> +	svm->vmcb->control.intercept_exceptions |=
> +		nested_vmcb->control.intercept_exceptions;
> +
> +	svm->vmcb->control.intercept |= nested_vmcb->control.intercept;
> +
> +	svm->nested_vmcb_msrpm = nested_vmcb->control.msrpm_base_pa;
> +
> +	force_new_asid(&svm->vcpu);
> +	svm->vmcb->control.exit_int_info = nested_vmcb->control.exit_int_info;
> +	svm->vmcb->control.exit_int_info_err = nested_vmcb->control.exit_int_info_err;
> +	svm->vmcb->control.int_ctl = nested_vmcb->control.int_ctl | V_INTR_MASKING_MASK;
> +	if (nested_vmcb->control.int_ctl & V_IRQ_MASK) {
> +		nsvm_printk("nSVM Injecting Interrupt: 0x%x\n",
> +				nested_vmcb->control.int_ctl);
> +	}
> +	if (nested_vmcb->control.int_ctl & V_INTR_MASKING_MASK)
> +		svm->vcpu.arch.hflags |= HF_VINTR_MASK;
> +	else
> +		svm->vcpu.arch.hflags &= ~HF_VINTR_MASK;
> +
> +	nsvm_printk("nSVM exit_int_info: 0x%x | int_state: 0x%x\n",
> +			nested_vmcb->control.exit_int_info,
> +			nested_vmcb->control.int_state);
> +
> +	svm->vmcb->control.int_vector = nested_vmcb->control.int_vector;
> +	svm->vmcb->control.int_state = nested_vmcb->control.int_state;
> +	svm->vmcb->control.tsc_offset += nested_vmcb->control.tsc_offset;
> +	if (nested_vmcb->control.event_inj & SVM_EVTINJ_VALID)
> +		nsvm_printk("Injecting Event: 0x%x\n",
> +				nested_vmcb->control.event_inj);
> +	svm->vmcb->control.event_inj = nested_vmcb->control.event_inj;
> +	svm->vmcb->control.event_inj_err = nested_vmcb->control.event_inj_err;
> +
> +	svm->vcpu.arch.hflags |= HF_GIF_MASK;
> +
> +	return 1;

I think there's a small bug here...

> +static int vmrun_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
> +{
> +	nsvm_printk("VMrun\n");
> +	if (nested_svm_check_permissions(svm))
> +		return 1;
> +
> +	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
> +	skip_emulated_instruction(&svm->vcpu);
> +
> +	if (nested_svm_do(svm, svm->vmcb->save.rax, 0,
> +			  NULL, nested_svm_vmrun))
> +		return 1;
> +

... which manifests here. nested_svm_run always returns 1, which will
cause us to return here rather than sync the msrpm's.

Cheers,
Muli
-- 
The First Workshop on I/O Virtualization (WIOV '08)
Dec 2008, San Diego, CA, http://www.usenix.org/wiov08/
                       <->
SYSTOR 2009---The Israeli Experimental Systems Conference
http://www.haifa.il.ibm.com/conferences/systor2009/