Re: [PATCH 10/24] Implement VMPTRLD
On Tue, Jul 06, 2010, Dong, Eddie wrote about "RE: [PATCH 10/24] Implement
VMPTRLD":
> Nadav Har'El wrote:
> > This patch implements the VMPTRLD instruction.
>..
> > +/* Emulate the VMPTRLD instruction */
> > +static int handle_vmptrld(struct kvm_vcpu *vcpu)
> > +{
>..
>
> How about the "Launch" status? Should we get that status from vmcs1x to
> distinguish guest VMLaunch & VMResume?
What do you mean? What does VMPTRLD need to do with the launch status?
The VMCLEAR and VMLAUNCH/VMRESUME emulation need to clear and check the
emulated-VMX launch status (which we called vmcs12->launch_state), and indeed
they already do so (that code appears in other patches, not this specific one).
--
Nadav Har'El|Thursday, Aug 5 2010, 25 Av 5770
n...@math.technion.ac.il |-
Phone +972-523-790466, ICQ 13349191 |I am not a complete idiot - some parts
http://nadav.harel.org.il |are missing.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/24] Implement VMPTRLD
On Mon, Jun 14, 2010, Avi Kivity wrote about "Re: [PATCH 10/24] Implement
VMPTRLD":
> >+if (vmcs_page == NULL)
> >+return 0;
> >
>
> Doesn't seem right.
>...
> >+
> >+if (read_guest_vmcs_gpa(vcpu,&guest_vmcs_addr)) {
> >+set_rflags_to_vmx_fail_invalid(vcpu);
> >
>
> Need to skip_emulated_instruction() in this case.
Thanks. I've "cleaned up my act" regarding error checking, and am now much
more careful to throw the right exception or set the right error code, and
to call skip_emulated_instruction when necessary.
Here is the new version, in case you want to look at it (of course, when I'm
done I'll send the whole patch set again).
Subject: [PATCH 11/26] nVMX: Implement VMPTRLD
This patch implements the VMPTRLD instruction.
Signed-off-by: Nadav Har'El
---
arch/x86/kvm/vmx.c | 64 ++-
1 file changed, 63 insertions(+), 1 deletion(-)
--- .before/arch/x86/kvm/vmx.c 2010-08-05 14:12:24.0 +0300
+++ .after/arch/x86/kvm/vmx.c 2010-08-05 14:12:24.0 +0300
@@ -3882,6 +3882,68 @@ static int handle_vmclear(struct kvm_vcp
return 1;
}
+/* Emulate the VMPTRLD instruction */
+static int handle_vmptrld(struct kvm_vcpu *vcpu)
+{
+ struct vcpu_vmx *vmx = to_vmx(vcpu);
+ gva_t gva;
+ gpa_t vmcs12_addr;
+
+ if (!nested_vmx_check_permission(vcpu))
+ return 1;
+
+ if (get_vmx_mem_address(vcpu, vmcs_readl(EXIT_QUALIFICATION),
+ vmcs_read32(VMX_INSTRUCTION_INFO), &gva))
+ return 1;
+
+ if (kvm_read_guest_virt(gva, &vmcs12_addr, sizeof(vmcs12_addr),
+ vcpu, NULL)) {
+ kvm_queue_exception(vcpu, PF_VECTOR);
+ return 1;
+ }
+
+ if (!IS_ALIGNED(vmcs12_addr, PAGE_SIZE)) {
+ nested_vmx_failValid(vcpu, VMXERR_VMPTRLD_INVALID_ADDRESS);
+ skip_emulated_instruction(vcpu);
+ return 1;
+ }
+
+ if (vmx->nested.current_vmptr != vmcs12_addr) {
+ struct vmcs12 *new_vmcs12;
+ struct page *page;
+ page = nested_get_page(vcpu, vmcs12_addr);
+ if (page == NULL){
+ nested_vmx_failInvalid(vcpu);
+ skip_emulated_instruction(vcpu);
+ return 1;
+ }
+ new_vmcs12 = kmap(page);
+ if (new_vmcs12->revision_id != VMCS12_REVISION){
+ kunmap(page);
+ nested_release_page_clean(page);
+ nested_vmx_failValid(vcpu,
+ VMXERR_VMPTRLD_INCORRECT_VMCS_REVISION_ID);
+ skip_emulated_instruction(vcpu);
+ return 1;
+ }
+ if (vmx->nested.current_vmptr != -1ull){
+ kunmap(vmx->nested.current_vmcs12_page);
+ nested_release_page(vmx->nested.current_vmcs12_page);
+ }
+
+ vmx->nested.current_vmptr = vmcs12_addr;
+ vmx->nested.current_vmcs12 = new_vmcs12;
+ vmx->nested.current_vmcs12_page = page;
+
+ if (nested_create_current_vmcs(vcpu))
+ return -ENOMEM;
+ }
+
+ nested_vmx_succeed(vcpu);
+ skip_emulated_instruction(vcpu);
+ return 1;
+}
+
static int handle_invlpg(struct kvm_vcpu *vcpu)
{
unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
@@ -4166,7 +4228,7 @@ static int (*kvm_vmx_exit_handlers[])(st
[EXIT_REASON_VMCALL] = handle_vmcall,
[EXIT_REASON_VMCLEAR] = handle_vmclear,
[EXIT_REASON_VMLAUNCH]= handle_vmx_insn,
- [EXIT_REASON_VMPTRLD] = handle_vmx_insn,
+ [EXIT_REASON_VMPTRLD] = handle_vmptrld,
[EXIT_REASON_VMPTRST] = handle_vmx_insn,
[EXIT_REASON_VMREAD] = handle_vmx_insn,
[EXIT_REASON_VMRESUME]= handle_vmx_insn,
--
Nadav Har'El|Thursday, Aug 5 2010, 25 Av 5770
n...@math.technion.ac.il |-
Phone +972-523-790466, ICQ 13349191 |We could wipe out world hunger if we knew
http://nadav.harel.org.il |how to make AOL's Free CD's edible!
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 10/24] Implement VMPTRLD
Nadav Har'El wrote:
> This patch implements the VMPTRLD instruction.
>
> Signed-off-by: Nadav Har'El
> ---
> --- .before/arch/x86/kvm/vmx.c2010-06-13 15:01:29.0 +0300
> +++ .after/arch/x86/kvm/vmx.c 2010-06-13 15:01:29.0 +0300
> @@ -3829,6 +3829,26 @@ static int read_guest_vmcs_gpa(struct kv
> return 0;
> }
>
> +static void set_rflags_to_vmx_fail_invalid(struct kvm_vcpu *vcpu)
> +{
> + unsigned long rflags;
> + rflags = vmx_get_rflags(vcpu);
> + rflags |= X86_EFLAGS_CF;
> + rflags &= ~X86_EFLAGS_PF & ~X86_EFLAGS_AF & ~X86_EFLAGS_ZF &
> + ~X86_EFLAGS_SF & ~X86_EFLAGS_OF;
> + vmx_set_rflags(vcpu, rflags);
> +}
> +
> +static void set_rflags_to_vmx_fail_valid(struct kvm_vcpu *vcpu)
> +{
> + unsigned long rflags;
> + rflags = vmx_get_rflags(vcpu);
> + rflags |= X86_EFLAGS_ZF;
> + rflags &= ~X86_EFLAGS_PF & ~X86_EFLAGS_AF & ~X86_EFLAGS_CF &
> + ~X86_EFLAGS_SF & ~X86_EFLAGS_OF;
> + vmx_set_rflags(vcpu, rflags);
> +}
> +
> static void clear_rflags_cf_zf(struct kvm_vcpu *vcpu)
> {
> unsigned long rflags;
> @@ -3869,6 +3889,57 @@ static int handle_vmclear(struct kvm_vcp
> return 1;
> }
>
> +static bool verify_vmcs12_revision(struct kvm_vcpu *vcpu, gpa_t
> guest_vmcs_addr) +{
> + bool ret;
> + struct vmcs12 *vmcs12;
> + struct page *vmcs_page = nested_get_page(vcpu, guest_vmcs_addr);
> + if (vmcs_page == NULL)
> + return 0;
> + vmcs12 = (struct vmcs12 *)kmap_atomic(vmcs_page, KM_USER0);
> + if (vmcs12->revision_id == VMCS12_REVISION)
> + ret = 1;
> + else {
> + set_rflags_to_vmx_fail_valid(vcpu);
> + ret = 0;
> + }
> + kunmap_atomic(vmcs12, KM_USER0);
> + kvm_release_page_dirty(vmcs_page);
> + return ret;
> +}
> +
> +/* Emulate the VMPTRLD instruction */
> +static int handle_vmptrld(struct kvm_vcpu *vcpu)
> +{
> + struct vcpu_vmx *vmx = to_vmx(vcpu);
> + gpa_t guest_vmcs_addr;
> +
> + if (!nested_vmx_check_permission(vcpu))
> + return 1;
> +
> + if (read_guest_vmcs_gpa(vcpu, &guest_vmcs_addr)) {
> + set_rflags_to_vmx_fail_invalid(vcpu);
> + return 1;
> + }
> +
> + if (!verify_vmcs12_revision(vcpu, guest_vmcs_addr))
> + return 1;
> +
> + if (vmx->nested.current_vmptr != guest_vmcs_addr) {
> + vmx->nested.current_vmptr = guest_vmcs_addr;
> +
> + if (nested_create_current_vmcs(vcpu)) {
> + printk(KERN_ERR "%s error could not allocate memory",
> + __func__);
> + return -ENOMEM;
> + }
> + }
> +
> + clear_rflags_cf_zf(vcpu);
> + skip_emulated_instruction(vcpu);
How about the "Launch" status? Should we get that status from vmcs1x to
distinguish guest VMLaunch & VMResume?
> + return 1;
> +}
> +
> static int handle_invlpg(struct kvm_vcpu *vcpu)
> {
> unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
> @@ -4153,7 +4224,7 @@ static int (*kvm_vmx_exit_handlers[])(st
> [EXIT_REASON_VMCALL] = handle_vmcall,
> [EXIT_REASON_VMCLEAR] = handle_vmclear,
> [EXIT_REASON_VMLAUNCH]= handle_vmx_insn,
> - [EXIT_REASON_VMPTRLD] = handle_vmx_insn,
> + [EXIT_REASON_VMPTRLD] = handle_vmptrld,
> [EXIT_REASON_VMPTRST] = handle_vmx_insn,
> [EXIT_REASON_VMREAD] = handle_vmx_insn,
> [EXIT_REASON_VMRESUME]= handle_vmx_insn,
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/24] Implement VMPTRLD
On Sun, Jun 13, 2010 at 03:27:41PM +0300, Nadav Har'El wrote:
> This patch implements the VMPTRLD instruction.
>
> Signed-off-by: Nadav Har'El
> ---
> --- .before/arch/x86/kvm/vmx.c2010-06-13 15:01:29.0 +0300
> +++ .after/arch/x86/kvm/vmx.c 2010-06-13 15:01:29.0 +0300
> @@ -3829,6 +3829,26 @@ static int read_guest_vmcs_gpa(struct kv
> return 0;
> }
>
> +static void set_rflags_to_vmx_fail_invalid(struct kvm_vcpu *vcpu)
> +{
> + unsigned long rflags;
> + rflags = vmx_get_rflags(vcpu);
> + rflags |= X86_EFLAGS_CF;
> + rflags &= ~X86_EFLAGS_PF & ~X86_EFLAGS_AF & ~X86_EFLAGS_ZF &
> + ~X86_EFLAGS_SF & ~X86_EFLAGS_OF;
> + vmx_set_rflags(vcpu, rflags);
> +}
> +
> +static void set_rflags_to_vmx_fail_valid(struct kvm_vcpu *vcpu)
> +{
> + unsigned long rflags;
> + rflags = vmx_get_rflags(vcpu);
> + rflags |= X86_EFLAGS_ZF;
> + rflags &= ~X86_EFLAGS_PF & ~X86_EFLAGS_AF & ~X86_EFLAGS_CF &
> + ~X86_EFLAGS_SF & ~X86_EFLAGS_OF;
> + vmx_set_rflags(vcpu, rflags);
> +}
> +
> static void clear_rflags_cf_zf(struct kvm_vcpu *vcpu)
> {
> unsigned long rflags;
> @@ -3869,6 +3889,57 @@ static int handle_vmclear(struct kvm_vcp
> return 1;
> }
>
> +static bool verify_vmcs12_revision(struct kvm_vcpu *vcpu, gpa_t
> guest_vmcs_addr)
> +{
> + bool ret;
> + struct vmcs12 *vmcs12;
> + struct page *vmcs_page = nested_get_page(vcpu, guest_vmcs_addr);
> + if (vmcs_page == NULL)
> + return 0;
> + vmcs12 = (struct vmcs12 *)kmap_atomic(vmcs_page, KM_USER0);
> + if (vmcs12->revision_id == VMCS12_REVISION)
> + ret = 1;
> + else {
> + set_rflags_to_vmx_fail_valid(vcpu);
Should set VM-Instruction Error Field accordingly.
> + ret = 0;
> + }
> + kunmap_atomic(vmcs12, KM_USER0);
> + kvm_release_page_dirty(vmcs_page);
> + return ret;
> +}
> +
> +/* Emulate the VMPTRLD instruction */
> +static int handle_vmptrld(struct kvm_vcpu *vcpu)
> +{
> + struct vcpu_vmx *vmx = to_vmx(vcpu);
> + gpa_t guest_vmcs_addr;
> +
> + if (!nested_vmx_check_permission(vcpu))
> + return 1;
> +
> + if (read_guest_vmcs_gpa(vcpu, &guest_vmcs_addr)) {
> + set_rflags_to_vmx_fail_invalid(vcpu);
> + return 1;
> + }
> +
> + if (!verify_vmcs12_revision(vcpu, guest_vmcs_addr))
> + return 1;
Should check that guest_vmcs_addr != VMXON address. I think this check
is missing from VMCLEAR too.
> +
> + if (vmx->nested.current_vmptr != guest_vmcs_addr) {
> + vmx->nested.current_vmptr = guest_vmcs_addr;
> +
> + if (nested_create_current_vmcs(vcpu)) {
> + printk(KERN_ERR "%s error could not allocate memory",
> + __func__);
> + return -ENOMEM;
> + }
> + }
> +
> + clear_rflags_cf_zf(vcpu);
> + skip_emulated_instruction(vcpu);
> + return 1;
> +}
> +
> static int handle_invlpg(struct kvm_vcpu *vcpu)
> {
> unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
> @@ -4153,7 +4224,7 @@ static int (*kvm_vmx_exit_handlers[])(st
> [EXIT_REASON_VMCALL] = handle_vmcall,
> [EXIT_REASON_VMCLEAR] = handle_vmclear,
> [EXIT_REASON_VMLAUNCH]= handle_vmx_insn,
> - [EXIT_REASON_VMPTRLD] = handle_vmx_insn,
> + [EXIT_REASON_VMPTRLD] = handle_vmptrld,
> [EXIT_REASON_VMPTRST] = handle_vmx_insn,
> [EXIT_REASON_VMREAD] = handle_vmx_insn,
> [EXIT_REASON_VMRESUME]= handle_vmx_insn,
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/24] Implement VMPTRLD
On 06/13/2010 03:27 PM, Nadav Har'El wrote:
This patch implements the VMPTRLD instruction.
static void clear_rflags_cf_zf(struct kvm_vcpu *vcpu)
{
unsigned long rflags;
@@ -3869,6 +3889,57 @@ static int handle_vmclear(struct kvm_vcp
return 1;
}
+static bool verify_vmcs12_revision(struct kvm_vcpu *vcpu, gpa_t
guest_vmcs_addr)
+{
+ bool ret;
+ struct vmcs12 *vmcs12;
+ struct page *vmcs_page = nested_get_page(vcpu, guest_vmcs_addr);
Blank line so I can catch my breath.
+ if (vmcs_page == NULL)
+ return 0;
Doesn't seem right.
+ vmcs12 = (struct vmcs12 *)kmap_atomic(vmcs_page, KM_USER0);
+ if (vmcs12->revision_id == VMCS12_REVISION)
+ ret = 1;
+ else {
+ set_rflags_to_vmx_fail_valid(vcpu);
+ ret = 0;
+ }
+ kunmap_atomic(vmcs12, KM_USER0);
+ kvm_release_page_dirty(vmcs_page);
Can release a clean page here. But what happened to those mapping helpers?
+ return ret;
+}
+
+/* Emulate the VMPTRLD instruction */
+static int handle_vmptrld(struct kvm_vcpu *vcpu)
+{
+ struct vcpu_vmx *vmx = to_vmx(vcpu);
+ gpa_t guest_vmcs_addr;
+
+ if (!nested_vmx_check_permission(vcpu))
+ return 1;
+
+ if (read_guest_vmcs_gpa(vcpu,&guest_vmcs_addr)) {
+ set_rflags_to_vmx_fail_invalid(vcpu);
Need to skip_emulated_instruction() in this case.
+ return 1;
+ }
+
+ if (!verify_vmcs12_revision(vcpu, guest_vmcs_addr))
+ return 1;
Here too.
+
+ if (vmx->nested.current_vmptr != guest_vmcs_addr) {
+ vmx->nested.current_vmptr = guest_vmcs_addr;
+
+ if (nested_create_current_vmcs(vcpu)) {
+ printk(KERN_ERR "%s error could not allocate memory",
+ __func__);
In general ftrace and the ENOMEM itself are sufficient documentation
that something went wrong.
+ return -ENOMEM;
+ }
+ }
+
+ clear_rflags_cf_zf(vcpu);
+ skip_emulated_instruction(vcpu);
+ return 1;
+}
+
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/24] Implement VMPTRLD
This patch implements the VMPTRLD instruction.
Signed-off-by: Nadav Har'El
---
--- .before/arch/x86/kvm/vmx.c 2010-06-13 15:01:29.0 +0300
+++ .after/arch/x86/kvm/vmx.c 2010-06-13 15:01:29.0 +0300
@@ -3829,6 +3829,26 @@ static int read_guest_vmcs_gpa(struct kv
return 0;
}
+static void set_rflags_to_vmx_fail_invalid(struct kvm_vcpu *vcpu)
+{
+ unsigned long rflags;
+ rflags = vmx_get_rflags(vcpu);
+ rflags |= X86_EFLAGS_CF;
+ rflags &= ~X86_EFLAGS_PF & ~X86_EFLAGS_AF & ~X86_EFLAGS_ZF &
+ ~X86_EFLAGS_SF & ~X86_EFLAGS_OF;
+ vmx_set_rflags(vcpu, rflags);
+}
+
+static void set_rflags_to_vmx_fail_valid(struct kvm_vcpu *vcpu)
+{
+ unsigned long rflags;
+ rflags = vmx_get_rflags(vcpu);
+ rflags |= X86_EFLAGS_ZF;
+ rflags &= ~X86_EFLAGS_PF & ~X86_EFLAGS_AF & ~X86_EFLAGS_CF &
+ ~X86_EFLAGS_SF & ~X86_EFLAGS_OF;
+ vmx_set_rflags(vcpu, rflags);
+}
+
static void clear_rflags_cf_zf(struct kvm_vcpu *vcpu)
{
unsigned long rflags;
@@ -3869,6 +3889,57 @@ static int handle_vmclear(struct kvm_vcp
return 1;
}
+static bool verify_vmcs12_revision(struct kvm_vcpu *vcpu, gpa_t
guest_vmcs_addr)
+{
+ bool ret;
+ struct vmcs12 *vmcs12;
+ struct page *vmcs_page = nested_get_page(vcpu, guest_vmcs_addr);
+ if (vmcs_page == NULL)
+ return 0;
+ vmcs12 = (struct vmcs12 *)kmap_atomic(vmcs_page, KM_USER0);
+ if (vmcs12->revision_id == VMCS12_REVISION)
+ ret = 1;
+ else {
+ set_rflags_to_vmx_fail_valid(vcpu);
+ ret = 0;
+ }
+ kunmap_atomic(vmcs12, KM_USER0);
+ kvm_release_page_dirty(vmcs_page);
+ return ret;
+}
+
+/* Emulate the VMPTRLD instruction */
+static int handle_vmptrld(struct kvm_vcpu *vcpu)
+{
+ struct vcpu_vmx *vmx = to_vmx(vcpu);
+ gpa_t guest_vmcs_addr;
+
+ if (!nested_vmx_check_permission(vcpu))
+ return 1;
+
+ if (read_guest_vmcs_gpa(vcpu, &guest_vmcs_addr)) {
+ set_rflags_to_vmx_fail_invalid(vcpu);
+ return 1;
+ }
+
+ if (!verify_vmcs12_revision(vcpu, guest_vmcs_addr))
+ return 1;
+
+ if (vmx->nested.current_vmptr != guest_vmcs_addr) {
+ vmx->nested.current_vmptr = guest_vmcs_addr;
+
+ if (nested_create_current_vmcs(vcpu)) {
+ printk(KERN_ERR "%s error could not allocate memory",
+ __func__);
+ return -ENOMEM;
+ }
+ }
+
+ clear_rflags_cf_zf(vcpu);
+ skip_emulated_instruction(vcpu);
+ return 1;
+}
+
static int handle_invlpg(struct kvm_vcpu *vcpu)
{
unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
@@ -4153,7 +4224,7 @@ static int (*kvm_vmx_exit_handlers[])(st
[EXIT_REASON_VMCALL] = handle_vmcall,
[EXIT_REASON_VMCLEAR] = handle_vmclear,
[EXIT_REASON_VMLAUNCH]= handle_vmx_insn,
- [EXIT_REASON_VMPTRLD] = handle_vmx_insn,
+ [EXIT_REASON_VMPTRLD] = handle_vmptrld,
[EXIT_REASON_VMPTRST] = handle_vmx_insn,
[EXIT_REASON_VMREAD] = handle_vmx_insn,
[EXIT_REASON_VMRESUME]= handle_vmx_insn,
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
