Re: [kvm-devel] [1-5/17] Patch 1-10 of kvm/ia64 V2

Re: [kvm-devel] [1-5/17] Patch 1-10 of kvm/ia64 V2

[Akio Takebe]

Hi, Xiantao

I got reading kvm-ia64 started. :)
Which member is p2m table?

> [2/17] Add kvm-ia64.c to support kvm module on Itanium processors.
Some trivial comments.

+int kvm_arch_init(void *opaque)
+{
+	int r;
+	struct kvm_vmm_info *vmm_info = (struct kvm_vmm_info *)opaque;
+
+	if (kvm_vmm_info) {
+		printk(KERN_ERR "kvm: already loaded the other module!\n");
+		r = -EEXIST;
+		goto out;
+	}
+
+	r =  -ENOMEM;
extra white space.

+	kvm_vmm_info = kzalloc(sizeof(struct kvm_vmm_info), GFP_KERNEL);
+	if (!kvm_vmm_info)
+		goto out;
+
Can we get the message of shortage of memory by qemu?

+	if (!vti_cpu_has_kvm_support()) {
+		printk(KERN_ERR "kvm: no hardware support\n");
+		r = -EOPNOTSUPP;
+		goto out_free0;
+	}
+
We had better check vti support at the first.

+	kvm_hack_insert_tr();
+
+	if (kvm_alloc_vmm_area())
+		goto out_free0;
+
r = kvm_alloc_vmm_area() is better.

+	r = kvm_relocate_vmm(vmm_info, vmm_info->module);
+	if (r)
+		goto out_free1;
+
+	return 0;
+
+out_free1:
+	kvm_free_vmm_area();
+out_free0:
+	kfree(kvm_vmm_info);
+out:
+	return r;
+}
+
+void kvm_arch_exit(void)
+{
+	kvm_free_vmm_area();
+	kfree(kvm_vmm_info);
+	kvm_vmm_info = 0;
+}
kvm_vmm_info = (void *)0?

Best Regards,

Akio Takebe

Re: [kvm-devel] [1-5/17] Patch 1-10 of kvm/ia64 V2

[Akio Takebe]

Hi, Xiantao

>> [3/17] Add firmware virtualization support.

+void kvm_sal_emul(struct kvm_vcpu *vcpu)
+{
+
+	struct sal_ret_values result;
+	u64 index, in1, in2, in3, in4, in5, in6, in7;
+
+	kvm_get_sal_call_data(vcpu, &index, &in1, &in2,
+			&in3, &in4, &in5, &in6, &in7);
+	if (index) {
+		result = sal_emulator(index, in1, in2, in3,
+						in4, in5, in6, in7);
+		set_sal_result(vcpu, result);
+	} else
+		printk(KERN_WARNING"kvm:Unsupported sal call called,"
+					" index:%d!\n", index);
+}
The printk always show index:0.
And set_sal_result() doesn't work at the time of index=0.
So I think the following code is better.

+void kvm_sal_emul(struct kvm_vcpu *vcpu)
+{
+
+	struct sal_ret_values result;
+	u64 index, in1, in2, in3, in4, in5, in6, in7;
+
+	kvm_get_sal_call_data(vcpu, &index, &in1, &in2,
+			&in3, &in4, &in5, &in6, &in7);
+	result = sal_emulator(index, in1, in2, in3,
+					in4, in5, in6, in7);
+	set_sal_result(vcpu, result);
+}

Best Regards,

Akio Takebe

RE: [kvm-devel] [1-5/17] Patch 1-10 of kvm/ia64 V2

[Zhang, Xiantao]

Hi Akio,
	Thank you for your comments.
Xiantao
Akio Takebe wrote:
> Hi, Xiantao
m: already loaded the other module!\n");
> +		r = -EEXIST;
> +		goto out;
> +	}
> +
> +	r =  -ENOMEM;
> extra white space.

Yes.  One more white space here.

> +	kvm_vmm_info = kzalloc(sizeof(struct kvm_vmm_info), GFP_KERNEL);
> +	if (!kvm_vmm_info)
> +		goto out;
> +
> Can we get the message of shortage of memory by qemu?

The memory allocation here is just for vmm module initialization, and no
qemu/vm created so far. 
Memory shortage here, will leads to module insert fail, and this is our
expectable result.  

> +	if (!vti_cpu_has_kvm_support()) {
> +		printk(KERN_ERR "kvm: no hardware support\n");
> +		r = -EOPNOTSUPP;
> +		goto out_free0;
> +	}
> +
> We had better check vti support at the first.

OK,  I will move it to first in arch_init. :)

> +}
> kvm_vmm_info = (void *)0?

Maybe NULL is better :)

> 
>
------------------------------------------------------------------------
-
> This SF.net email is sponsored by: Microsoft
> Defy all challenges. Microsoft(R) Visual Studio 2005.
> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kvm-devel

RE: [kvm-devel] [1-5/17] Patch 1-10 of kvm/ia64 V2

[Zhang, Xiantao]

Akio Takebe wrote:
> Hi, Xiantao
> 
>>> [3/17] Add firmware virtualization support.
> 
> +void kvm_sal_emul(struct kvm_vcpu *vcpu)
> +{
> +
> +	struct sal_ret_values result;
> +	u64 index, in1, in2, in3, in4, in5, in6, in7;
> +
> +	kvm_get_sal_call_data(vcpu, &index, &in1, &in2,
> +			&in3, &in4, &in5, &in6, &in7);
> +	if (index) {
> +		result = sal_emulator(index, in1, in2, in3,
> +						in4, in5, in6, in7);
> +		set_sal_result(vcpu, result);
> +	} else
> +		printk(KERN_WARNING"kvm:Unsupported sal call called,"
> +					" index:%d!\n", index);
> +}
> The printk always show index:0.
> And set_sal_result() doesn't work at the time of index=0.
> So I think the following code is better.

Good catch! Originally, I want to use it for kvm_get_sal_call debug, but
forget to remove it. 
Thank you! :)

> +void kvm_sal_emul(struct kvm_vcpu *vcpu)
> +{
> +
> +	struct sal_ret_values result;
> +	u64 index, in1, in2, in3, in4, in5, in6, in7;
> +
> +	kvm_get_sal_call_data(vcpu, &index, &in1, &in2,
> +			&in3, &in4, &in5, &in6, &in7);
> +	result = sal_emulator(index, in1, in2, in3,
> +					in4, in5, in6, in7);
> +	set_sal_result(vcpu, result);
> +}
> 
> Best Regards,
> 
> Akio Takebe