Re: [PATCH 19/27] nVMX: Exiting from L2 to L1

[Avi Kivity <avi@redhat.com>]

  On 10/17/2010 12:13 PM, Nadav Har'El wrote:
> This patch implements nested_vmx_vmexit(), called when the nested L2 guest
> exits and we want to run its L1 parent and let it handle this exit.
>
> Note that this will not necessarily be called on every L2 exit. L0 may decide
> to handle a particular exit on its own, without L1's involvement; In that
> case, L0 will handle the exit, and resume running L2, without running L1 and
> without calling nested_vmx_vmexit(). The logic for deciding whether to handle
> a particular exit in L1 or in L0, i.e., whether to call nested_vmx_vmexit(),
> will appear in the next patch.
>
> Signed-off-by: Nadav Har'El<nyh@il.ibm.com>
> ---
>   arch/x86/kvm/vmx.c |  235 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 235 insertions(+)
>
> --- .before/arch/x86/kvm/vmx.c	2010-10-17 11:52:02.000000000 +0200
> +++ .after/arch/x86/kvm/vmx.c	2010-10-17 11:52:02.000000000 +0200
> @@ -5085,6 +5085,8 @@ static void __vmx_complete_interrupts(st
>
>   static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
>   {
> +	if (vmx->nested.nested_mode)
> +		return;
>   	__vmx_complete_interrupts(vmx, vmx->idt_vectoring_info,
>   				  VM_EXIT_INSTRUCTION_LEN,
>   				  IDT_VECTORING_ERROR_CODE);
> @@ -5981,6 +5983,239 @@ static int nested_vmx_run(struct kvm_vcp
>   	return 1;
>   }
>
> +/*
> + * On a nested exit from L2 to L1, vmcs12.guest_cr0 might not be up-to-date
> + * because L2 may have changed some cr0 bits directly (see CRO_GUEST_HOST_MASK)
> + * without L0 trapping the change and updating vmcs12.
> + * This function returns the value we should put in vmcs12.guest_cr0. It's not
> + * enough to just return the current (vmcs02) GUEST_CR0. This may not be the
> + * guest cr0 that L1 thought it was giving its L2 guest - it is possible that
> + * L1 wished to allow its guest to set a cr0 bit directly, but we (L0) asked
> + * to trap this change and instead set just the read shadow. If this is the
> + * case, we need to copy these read-shadow bits back to vmcs12.guest_cr0, where
> + * L1 believes they already are.
> + */
> +static inline unsigned long
> +vmcs12_guest_cr0(struct kvm_vcpu *vcpu, struct vmcs_fields *vmcs12)
> +{
> +	unsigned long guest_cr0_bits =
> +		vcpu->arch.cr0_guest_owned_bits | vmcs12->cr0_guest_host_mask;
> +	return (vmcs_readl(GUEST_CR0)&  guest_cr0_bits) |
> +		(vmcs_readl(CR0_READ_SHADOW)&  ~guest_cr0_bits);
> +}

I think it's easier to keep cr0_guest_owned_bits up to date, and use 
kvm_read_cr0().  In fact, I think you have to do it so kvm_read_cr0() 
works correctly while in nested guest context.

> +
> +static inline unsigned long
> +vmcs12_guest_cr4(struct kvm_vcpu *vcpu, struct vmcs_fields *vmcs12)
> +{
> +	unsigned long guest_cr4_bits =
> +		vcpu->arch.cr4_guest_owned_bits | vmcs12->cr4_guest_host_mask;
> +	return (vmcs_readl(GUEST_CR4)&  guest_cr4_bits) |
> +		(vmcs_readl(CR4_READ_SHADOW)&  ~guest_cr4_bits);
> +}

Ditto.

> +
> +/*
> + * prepare_vmcs12 is called when the nested L2 guest exits and we want to
> + * prepare to run its L1 parent. L1 keeps a vmcs for L2 (vmcs12), and this
> + * function updates it to reflect the changes to the guest state while L2 was
> + * running (and perhaps made some exits which were handled directly by L0
> + * without going back to L1), and to reflect the exit reason.
> + * Note that we do not have to copy here all VMCS fields, just those that
> + * could have changed by the L2 guest or the exit - i.e., the guest-state and
> + * exit-information fields only. Other fields are modified by L1 with VMWRITE,
> + * which already writes to vmcs12 directly.
> + */
> +void prepare_vmcs12(struct kvm_vcpu *vcpu)
> +{
> +	struct vmcs_fields *vmcs12 = get_vmcs12_fields(vcpu);
> +
> +	/* update guest state fields: */
> +	vmcs12->guest_cr0 = vmcs12_guest_cr0(vcpu, vmcs12);
> +	vmcs12->guest_cr4 = vmcs12_guest_cr4(vcpu, vmcs12);
> +
> +	vmcs12->guest_dr7 = vmcs_readl(GUEST_DR7);
> +	vmcs12->guest_rsp = vmcs_readl(GUEST_RSP);
> +	vmcs12->guest_rip = vmcs_readl(GUEST_RIP);

kvm_register_read(), kvm_rip_read(), kvm_get_dr().

> +	vmcs12->guest_rflags = vmcs_readl(GUEST_RFLAGS);
> +

> +
> +	if(enable_ept){
> +		vmcs12->guest_physical_address =
> +			vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> +		vmcs12->guest_linear_address = vmcs_readl(GUEST_LINEAR_ADDRESS);
> +	}

Drop please.

> +
> +static int nested_vmx_vmexit(struct kvm_vcpu *vcpu, bool is_interrupt)
> +{
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +	int efer_offset;
> +	struct vmcs_fields *vmcs01 = vmx->nested.vmcs01_fields;
> +
> +	if (!vmx->nested.nested_mode) {
> +		printk(KERN_INFO "WARNING: %s called but not in nested mode\n",
> +		       __func__);
> +		return 0;
> +	}
> +
> +	sync_cached_regs_to_vmcs(vcpu);

If you use kvm_rip_read() etc, this isn't needed.

> +
> +	prepare_vmcs12(vcpu);
> +	if (is_interrupt)
> +		get_vmcs12_fields(vcpu)->vm_exit_reason =
> +			EXIT_REASON_EXTERNAL_INTERRUPT;

Somewhat strange that a field is updated conditionally.

> +
> +	vmx->nested.current_vmcs12->launched = vmx->launched;
> +	vmx->nested.current_vmcs12->cpu = vcpu->cpu;
> +
> +	vmx->vmcs = vmx->nested.vmcs01;
> +	vcpu->cpu = vmx->nested.l1_state.cpu;
> +	vmx->launched = vmx->nested.l1_state.launched;
> +
> +	vmx->nested.nested_mode = false;
> +
> +	vmx_vcpu_load(vcpu, get_cpu());
> +	put_cpu();

Again need to extend the preempt disable region, probably to before you 
assign vmx->vmcs.

> +
> +	vcpu->arch.efer = vmx->nested.l1_state.efer;
> +	if ((vcpu->arch.efer&  EFER_LMA)&&
> +	    !(vcpu->arch.efer&  EFER_SCE))
> +		vcpu->arch.efer |= EFER_SCE;
> +
> +	efer_offset = __find_msr_index(vmx, MSR_EFER);
> +	if (update_transition_efer(vmx, efer_offset))
> +		wrmsrl(MSR_EFER, vmx->guest_msrs[efer_offset].data);

Use kvm_set_efer().  Just take the existing efer and use "host address 
space vm-exit control bit" for LMA and LME.

You may need to enter_lmode() or exit_lmode() manually.

> +	
> +	/*
> +	 * L2 perhaps switched to real mode and set vmx->rmode, but we're back
> +	 * in L1 and as it is running VMX, it can't be in real mode.
> +	 */
> +	vmx->rmode.vm86_active = 0;
> +

L2 cannot be in real mode, (vmx non-root mode does not support it).  L1 
cannot be in real mode (vmx root operation does not support it).  So no 
need for the assignment.

> +	/*
> +	 * We're running a regular L1 guest again, so we do the regular KVM
> +	 * thing: run vmx_set_cr0 with the cr0 bits the guest thinks it has.
> +	 * vmx_set_cr0 might use slightly different bits on the new guest_cr0
> +	 * it sets, e.g., add TS when !fpu_active.
> +	 * Note that vmx_set_cr0 refers to rmode and efer set above.
> +	 */
> +	vmx_set_cr0(vcpu, guest_readable_cr0(vmcs01));

Should be vmcs12->host_cr0.

> +	/*
> +	 * If we did fpu_activate()/fpu_deactive() during l2's run, we need to
> +	 * apply the same changes to l1's vmcs. We just set cr0 correctly, but
> +	 * now we need to also update cr0_guest_host_mask and exception_bitmap.
> +	 */
> +	vmcs_write32(EXCEPTION_BITMAP,
> +		(vmcs01->exception_bitmap&  ~(1u<<NM_VECTOR)) |
> +			(vcpu->fpu_active ? 0 : (1u<<NM_VECTOR)));

update_exception_bitmap()

> +	vcpu->arch.cr0_guest_owned_bits = (vcpu->fpu_active ? X86_CR0_TS : 0);

&= ~X86_CR0_TS removes the inside information about fpu_active.

> +	vmcs_writel(CR0_GUEST_HOST_MASK, ~vcpu->arch.cr0_guest_owned_bits);


> +
> +
> +	vmx_set_cr4(vcpu, vmx->nested.l1_state.cr4);
> +
> +	if (enable_ept) {
> +		vcpu->arch.cr3 = vmcs01->guest_cr3;
> +		vmcs_write32(GUEST_CR3, vmcs01->guest_cr3);
> +		vmcs_write64(EPT_POINTER, vmcs01->ept_pointer);
> +		vmcs_write64(GUEST_PDPTR0, vmcs01->guest_pdptr0);
> +		vmcs_write64(GUEST_PDPTR1, vmcs01->guest_pdptr1);
> +		vmcs_write64(GUEST_PDPTR2, vmcs01->guest_pdptr2);
> +		vmcs_write64(GUEST_PDPTR3, vmcs01->guest_pdptr3);

vmexits do not reload PDPTRs from a cache.  Instead, use kvm_set_cr3() 
unconditionally.

> +	} else {
> +		kvm_set_cr3(vcpu, vmx->nested.l1_state.cr3);
> +	}
> +
> +	kvm_register_write(vcpu, VCPU_REGS_RSP, vmcs01->guest_rsp);
> +	kvm_register_write(vcpu, VCPU_REGS_RIP, vmcs01->guest_rip);
> +
> +	kvm_mmu_reset_context(vcpu);
> +	kvm_mmu_load(vcpu);

kvm_mmu_load() is unnecessary, next guest entry will do it automatically 
IIRC.

> +
> +	if (unlikely(vmx->fail)) {
> +		/*
> +		 * When L1 launches L2 and then we (L0) fail to launch L2,
> +		 * we nested_vmx_vmexit back to L1, but now should let it know
> +		 * that the VMLAUNCH failed - with the same error that we
> +		 * got when launching L2.
> +		 */
> +		vmx->fail = 0;
> +		nested_vmx_failValid(vcpu, vmcs_read32(VM_INSTRUCTION_ERROR));
> +	} else
> +		nested_vmx_succeed(vcpu);
> +
> +	return 0;
> +}
> +
>   static struct kvm_x86_ops vmx_x86_ops = {
>   	.cpu_has_kvm_support = cpu_has_kvm_support,
>   	.disabled_by_bios = vmx_disabled_by_bios,


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