Re: [PATCH 5/5] Nested VMX patch 5 implements vmlaunch and vmresume

[Avi Kivity <avi@redhat.com>]

On 10/15/2009 11:41 PM, oritw@il.ibm.com wrote:
> From: Orit Wasserman<oritw@il.ibm.com>
>
> ---
>   arch/x86/kvm/vmx.c | 1173 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>   1 files changed, 1148 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 6a4c252..e814029 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -209,6 +209,7 @@ struct __attribute__ ((__packed__)) level_state {
>   	struct vmcs *vmcs;
>   	int cpu;
>   	int launched;
> +	bool first_launch;
>   };
>
>   struct nested_vmx {
> @@ -216,6 +217,12 @@ struct nested_vmx {
>   	bool vmxon;
>   	/* What is the location of the  vmcs l1 keeps for l2? (in level1 gpa) */
>   	u64 vmptr;
> +	/* Are we running nested guest */
> +	bool nested_mode;
> +	/* L1 requested VMLAUNCH or VMRESUME but we didn't run L2 yet */
> +	bool nested_run_pending;
> +	/* flag indicating if there was a valid IDT after exiting from l2 */
> +	bool nested_valid_idt;
>    

Did you mean valid_idt_vectoring_info?

No need to prefix everything with nested_ inside nested_vmx.

> +void prepare_vmcs_12(struct kvm_vcpu *vcpu)
> +{
> +	struct shadow_vmcs *l2_shadow_vmcs =
> +		get_shadow_vmcs(vcpu);
> +
> +	l2_shadow_vmcs->guest_es_selector = vmcs_read16(GUEST_ES_SELECTOR);
> +	l2_shadow_vmcs->guest_cs_selector = vmcs_read16(GUEST_CS_SELECTOR);
> +	l2_shadow_vmcs->guest_ss_selector = vmcs_read16(GUEST_SS_SELECTOR);
> +	l2_shadow_vmcs->guest_ds_selector = vmcs_read16(GUEST_DS_SELECTOR);
> +	l2_shadow_vmcs->guest_fs_selector = vmcs_read16(GUEST_FS_SELECTOR);
> +	l2_shadow_vmcs->guest_gs_selector = vmcs_read16(GUEST_GS_SELECTOR);
> +	l2_shadow_vmcs->guest_ldtr_selector = vmcs_read16(GUEST_LDTR_SELECTOR);
> +	l2_shadow_vmcs->guest_tr_selector = vmcs_read16(GUEST_TR_SELECTOR);
> +
> +	l2_shadow_vmcs->tsc_offset = vmcs_read64(TSC_OFFSET);
> +	l2_shadow_vmcs->guest_physical_address =
> +		vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> +	l2_shadow_vmcs->vmcs_link_pointer = vmcs_read64(VMCS_LINK_POINTER);
>    

Physical addresses need translation,  no?

> +	l2_shadow_vmcs->guest_cr0 = vmcs_readl(GUEST_CR0);
> +
> +	l2_shadow_vmcs->guest_cr4 = vmcs_readl(GUEST_CR4);
>    

We don't allow the guest to modify these, so no need to read them.  If 
you do, you need to remove the bits that we modify.

> +
> +int load_vmcs_common(struct shadow_vmcs *src)
> +{
> +
> +	vmcs_write64(VMCS_LINK_POINTER, src->vmcs_link_pointer);
>    

Why load this?

> +	vmcs_write64(GUEST_IA32_DEBUGCTL, src->guest_ia32_debugctl);
>    

I think some features there are dangerous.

> +	vmcs_write32(VM_ENTRY_MSR_LOAD_COUNT, src->vm_entry_msr_load_count);
>    

Need to verify?  Also need to validate the loaded MSRs and run them 
through kvm_set_msr() instead of letting the cpu do it.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.