Re: [PATCH 3/7] Nested VMX patch 3 implements vmptrld and vmptrst

[Avi Kivity <avi@redhat.com>]

On 12/10/2009 08:38 PM, oritw@il.ibm.com wrote:
>
> +
> +
>   struct __attribute__ ((__packed__)) level_state {
>   	/* Has the level1 guest done vmclear? */
>   	bool vmclear;
> +
> +	u64 io_bitmap_a;
> +	u64 io_bitmap_b;
> +	u64 msr_bitmap;
> +
> +	bool first_launch;
>   };
>    

Please keep things naturally aligned.

>   /*
> @@ -122,6 +255,8 @@ struct nested_vmx {
>   	gpa_t current_vmptr;
>   	/* Level 1 state for switching to level 2 and back */
>   	struct level_state *l1_state;
> +	/* Level 1 shadow vmcs for switching to level 2 and back */
> +	struct shadow_vmcs *l1_shadow_vmcs;
>   	/* list of vmcs for each l2 guest created by l1 */
>   	struct list_head l2_vmcs_list;
>   	/* l2 page corresponding to the current vmcs set by l1 */
> @@ -187,10 +322,7 @@ static struct page *nested_get_page(struct kvm_vcpu *vcpu,
>   {
>   	struct page *vmcs_page = NULL;
>
> -	down_read(&current->mm->mmap_sem);
>   	vmcs_page = gfn_to_page(vcpu->kvm, vmcs_addr>>  PAGE_SHIFT);
> -	up_read(&current->mm->mmap_sem);
>    

Fold this into the patch that introduced the problem.

> -
>   	if (is_error_page(vmcs_page)) {
>   		printk(KERN_ERR "%s error allocating page 0x%llx\n",
>   		       __func__, vmcs_addr);
> @@ -832,13 +964,14 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>
>   	if (per_cpu(current_vmcs, cpu) != vmx->vmcs) {
>   		u8 error;
> -
>   		per_cpu(current_vmcs, cpu) = vmx->vmcs;
> +
>    

Please avoid pointless whitespace changes.

>   		asm volatile (__ex(ASM_VMX_VMPTRLD_RAX) "; setna %0"
>   			      : "=g"(error) : "a"(&phys_addr), "m"(phys_addr)
>   			      : "cc");
> +
>   		if (error)
> -			printk(KERN_ERR "kvm: vmptrld %p/%llx fail\n",
> +			printk(KERN_ERR "kvm: vmptrld %p/%llx failed\n",
>   			       vmx->vmcs, phys_addr);
>    

Fold.

> +
>   static int create_l1_state(struct kvm_vcpu *vcpu)
>   {
>   	struct vcpu_vmx *vmx = to_vmx(vcpu);
> @@ -1441,10 +1587,75 @@ static int create_l1_state(struct kvm_vcpu *vcpu)
>   	} else
>   		return 0;
>
> +	vmx->nested.l1_shadow_vmcs = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!vmx->nested.l1_shadow_vmcs) {
> +		printk(KERN_INFO "%s could not allocate memory for l1_shadow vmcs\n",
> +		       __func__);
> +		kfree(vmx->nested.l1_state);
> +		return -ENOMEM;
> +	}
> +
>   	INIT_LIST_HEAD(&(vmx->nested.l2_vmcs_list));
>   	return 0;
>   }
>
> +static struct vmcs *alloc_vmcs(void);
> +int create_l2_state(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +	struct vmcs *l2_vmcs;
> +
> +	if (!nested_map_current(vcpu)) {
> +		printk(KERN_ERR "%s error mapping  level 2 page", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	l2_vmcs = nested_get_current_vmcs(vcpu);
> +	if (!l2_vmcs) {
> +		struct nested_vmcs_list *new_l2_guest =
> +			(struct nested_vmcs_list *)
> +			kmalloc(sizeof(struct nested_vmcs_list), GFP_KERNEL);
> +
> +		if (!new_l2_guest) {
> +			printk(KERN_ERR "%s error could not allocate memory for a new l2 guest list item",
> +			       __func__);
> +			nested_unmap_current(vcpu);
> +			return -ENOMEM;
> +		}
>    

Can the list grow without bounds?

> +
> +		l2_vmcs = alloc_vmcs();
> +
> +		if (!l2_vmcs) {
> +			printk(KERN_ERR "%s error could not allocate memory for l2_vmcs",
> +			       __func__);
> +			kfree(new_l2_guest);
> +			nested_unmap_current(vcpu);
> +			return -ENOMEM;
> +		}
> +
> +		new_l2_guest->vmcs_addr = vmx->nested.current_vmptr;
> +		new_l2_guest->l2_vmcs   = l2_vmcs;
> +		list_add(&(new_l2_guest->list),&(vmx->nested.l2_vmcs_list));
> +	}
> +
> +	if (cpu_has_vmx_msr_bitmap())
> +		vmx->nested.current_l2_page->l2_state.msr_bitmap =
> +			vmcs_read64(MSR_BITMAP);
> +	else
> +		vmx->nested.current_l2_page->l2_state.msr_bitmap = 0;
> +
> +	vmx->nested.current_l2_page->l2_state.io_bitmap_a =
> +		vmcs_read64(IO_BITMAP_A);
> +	vmx->nested.current_l2_page->l2_state.io_bitmap_b =
> +		vmcs_read64(IO_BITMAP_B);
>    

Don't understand why these reads are needed.

> +
> +	vmx->nested.current_l2_page->l2_state.first_launch = true;
> +
> +	nested_unmap_current(vcpu);
> +
> +	return 0;
> +}
> +
>   

> @@ -3633,8 +3849,9 @@ static int handle_vmon(struct kvm_vcpu *vcpu)
>   		return 1;
>   	}
>
> -	if (create_l1_state(vcpu)) {
> -		printk(KERN_ERR "%s create_l1_state failed\n", __func__);
> +	r = create_l1_state(vcpu);
> +	if (r) {
> +		printk(KERN_ERR "%s create_l1_state failed: %d\n", __func__, r);
>    

Move this to the original patch.

>   		kvm_queue_exception(vcpu, UD_VECTOR);
>   		return 1;
>   	}
> @@ -3645,6 +3862,63 @@ static int handle_vmon(struct kvm_vcpu *vcpu)
>   	return 1;
>   }
>
> +static int handle_vmptrld(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +	u64 guest_vmcs_addr;
> +	gva_t vmcs_gva;
> +	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
> +	u32 vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
> +	int r = 0;
> +
> +	if (!nested_vmx_check_permission(vcpu))
> +		return 1;
> +
> +	vmcs_gva = get_vmx_mem_address(vcpu, exit_qualification,
> +				       vmx_instruction_info);
> +
> +	if (read_guest_vmcs_gpa(vcpu, vmcs_gva,&guest_vmcs_addr))
> +		return 1;
> +
> +	if (vmx->nested.current_vmptr != guest_vmcs_addr) {
> +		vmx->nested.current_vmptr = guest_vmcs_addr;
> +		r = create_l2_state(vcpu);
> +		if (r) {
> +			printk(KERN_ERR "%s create_l2_state failed: %d\n",
> +			       __func__, r);
> +			return 1;
> +		}
> +	}
> +
> +	clear_rflags_cf_zf(vcpu);
> +	skip_emulated_instruction(vcpu);
> +	return 1;
>    

No set_rflags() on error?

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