This patch-set resolves several emulator bugs. Each fix is independent of the
others. The DR6/7 bug can occur during DR-access exit (regardless to
unrestricted mode, MMIO and SPT).
Thanks for reviewing the patches,
Nadav
Nadav Amit (6):
KVM: x86: bit-ops emulation ignores offset on 64-bit
KVM: x86: Wrong emulation on 'xadd X, X'
KVM: x86: Inter privilage level ret emulation is not implemeneted
KVM: x86: emulation of dword cmov on long-mode should clear [63:32]
KVM: x86: NOP emulation clears (incorrectly) the high 32-bits of RAX
KVM: x86: check DR6/7 high-bits are clear only on long-mode
arch/x86/kvm/emulate.c | 31 ++++++++++++++++++++-----------
arch/x86/kvm/x86.c | 13 +++++++++++--
2 files changed, 31 insertions(+), 13 deletions(-)
--
1.9.1
On long-mode the current NOP (0x90) emulation still writes back to RAX. As a
result, EAX is zero-extended and the high 32-bits of RAX are cleared.
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/emulate.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b354531..eb93eb4 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4663,8 +4663,9 @@ special_insn:
break;
case 0x90 ... 0x97: /* nop / xchg reg, rax */
if (ctxt->dst.addr.reg == reg_rmw(ctxt, VCPU_REGS_RAX))
- break;
- rc = em_xchg(ctxt);
+ ctxt->dst.type = OP_NONE;
+ else
+ rc = em_xchg(ctxt);
break;
case 0x98: /* cbw/cwde/cdqe */
switch (ctxt->op_bytes) {
--
1.9.1
The current emulation of bit operations ignores the offset from the destination
on 64-bit target memory operands. This patch fixes this behavior.
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/emulate.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index e4e833d..f0b0a10 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1220,12 +1220,14 @@ static void fetch_bit_operand(struct x86_emulate_ctxt *ctxt)
long sv = 0, mask;
if (ctxt->dst.type == OP_MEM && ctxt->src.type == OP_REG) {
- mask = ~(ctxt->dst.bytes * 8 - 1);
+ mask = ~((long)ctxt->dst.bytes * 8 - 1);
if (ctxt->src.bytes == 2)
sv = (s16)ctxt->src.val & (s16)mask;
else if (ctxt->src.bytes == 4)
sv = (s32)ctxt->src.val & (s32)mask;
+ else
+ sv = (s64)ctxt->src.val & (s64)mask;
ctxt->dst.addr.mem.ea += (sv >> 3);
}
--
1.9.1
From: Nadav Amit <[email protected]>
When the guest sets DR6 and DR7, KVM asserts the high 32-bits are clear, and
otherwise injects a #GP exception. This exception should only be injected only
if running in long-mode.
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/x86.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 57eac30..71fe841 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -756,6 +756,15 @@ static void kvm_update_dr7(struct kvm_vcpu *vcpu)
vcpu->arch.switch_db_regs |= KVM_DEBUGREG_BP_ENABLED;
}
+static bool is_64_bit_mode(struct kvm_vcpu *vcpu)
+{
+ int cs_db, cs_l;
+ if (!is_long_mode(vcpu))
+ return false;
+ kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
+ return cs_l;
+}
+
static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
{
switch (dr) {
@@ -769,7 +778,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
return 1; /* #UD */
/* fall through */
case 6:
- if (val & 0xffffffff00000000ULL)
+ if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu))
return -1; /* #GP */
vcpu->arch.dr6 = (val & DR6_VOLATILE) | DR6_FIXED_1;
kvm_update_dr6(vcpu);
@@ -779,7 +788,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
return 1; /* #UD */
/* fall through */
default: /* 7 */
- if (val & 0xffffffff00000000ULL)
+ if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu))
return -1; /* #GP */
vcpu->arch.dr7 = (val & DR7_VOLATILE) | DR7_FIXED_1;
kvm_update_dr7(vcpu);
--
1.9.1
The emulator does not emulate the xadd instruction correctly if the two
operands are the same. In this (unlikely) situation the result should be the
sum of X and X (2X) when it is currently X. The solution is to first perform
writeback to the source, before writing to the destination. The only
instruction which should be affected is xadd, as the other instructions that
perform writeback to the source use the extended accumlator (e.g., RAX:RDX).
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/emulate.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f0b0a10..3c8d867 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4711,17 +4711,17 @@ special_insn:
goto done;
writeback:
- if (!(ctxt->d & NoWrite)) {
- rc = writeback(ctxt, &ctxt->dst);
- if (rc != X86EMUL_CONTINUE)
- goto done;
- }
if (ctxt->d & SrcWrite) {
BUG_ON(ctxt->src.type == OP_MEM || ctxt->src.type == OP_MEM_STR);
rc = writeback(ctxt, &ctxt->src);
if (rc != X86EMUL_CONTINUE)
goto done;
}
+ if (!(ctxt->d & NoWrite)) {
+ rc = writeback(ctxt, &ctxt->dst);
+ if (rc != X86EMUL_CONTINUE)
+ goto done;
+ }
/*
* restore dst type in case the decoding will be reused
--
1.9.1
Even if the condition of cmov is not satisfied, bits[63:32] should be cleared.
This is clearly stated in Intel's CMOVcc documentation. The solution is to
reassign the destination onto itself if the condition is unsatisfied. For that
matter the original destination value needs to be read.
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/emulate.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 0183350..b354531 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3905,7 +3905,7 @@ static const struct opcode twobyte_table[256] = {
N, N,
N, N, N, N, N, N, N, N,
/* 0x40 - 0x4F */
- X16(D(DstReg | SrcMem | ModRM | Mov)),
+ X16(D(DstReg | SrcMem | ModRM)),
/* 0x50 - 0x5F */
N, N, N, N, N, N, N, N, N, N, N, N, N, N, N, N,
/* 0x60 - 0x6F */
@@ -4799,8 +4799,10 @@ twobyte_insn:
ops->get_dr(ctxt, ctxt->modrm_reg, &ctxt->dst.val);
break;
case 0x40 ... 0x4f: /* cmov */
- ctxt->dst.val = ctxt->dst.orig_val = ctxt->src.val;
- if (!test_cc(ctxt->b, ctxt->eflags))
+ if (test_cc(ctxt->b, ctxt->eflags))
+ ctxt->dst.val = ctxt->src.val;
+ else if (ctxt->mode != X86EMUL_MODE_PROT64 ||
+ ctxt->op_bytes != 4)
ctxt->dst.type = OP_NONE; /* no writeback */
break;
case 0x80 ... 0x8f: /* jnz rel, etc*/
--
1.9.1
Return unhandlable error on inter-privilage level ret instruction. This is
since the current emulation does not check the privilage level correctly when
loading the CS, and does not pop RSP/SS as needed.
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/emulate.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 3c8d867..0183350 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2019,6 +2019,7 @@ static int em_ret_far(struct x86_emulate_ctxt *ctxt)
{
int rc;
unsigned long cs;
+ int cpl = ctxt->ops->cpl(ctxt);
rc = emulate_pop(ctxt, &ctxt->_eip, ctxt->op_bytes);
if (rc != X86EMUL_CONTINUE)
@@ -2028,6 +2029,9 @@ static int em_ret_far(struct x86_emulate_ctxt *ctxt)
rc = emulate_pop(ctxt, &cs, ctxt->op_bytes);
if (rc != X86EMUL_CONTINUE)
return rc;
+ /* Outer-privilage level return is not implemented */
+ if (ctxt->mode >= X86EMUL_MODE_PROT16 && (cs & 3) > cpl)
+ return X86EMUL_UNHANDLEABLE;
rc = load_segment_descriptor(ctxt, (u16)cs, VCPU_SREG_CS);
return rc;
}
--
1.9.1
Il 15/06/2014 15:13, Nadav Amit ha scritto:
> From: Nadav Amit <[email protected]>
>
> When the guest sets DR6 and DR7, KVM asserts the high 32-bits are clear, and
> otherwise injects a #GP exception. This exception should only be injected only
> if running in long-mode.
>
> Signed-off-by: Nadav Amit <[email protected]>
> ---
> arch/x86/kvm/x86.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 57eac30..71fe841 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -756,6 +756,15 @@ static void kvm_update_dr7(struct kvm_vcpu *vcpu)
> vcpu->arch.switch_db_regs |= KVM_DEBUGREG_BP_ENABLED;
> }
>
> +static bool is_64_bit_mode(struct kvm_vcpu *vcpu)
> +{
> + int cs_db, cs_l;
> + if (!is_long_mode(vcpu))
> + return false;
> + kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
> + return cs_l;
> +}
> +
> static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
> {
> switch (dr) {
> @@ -769,7 +778,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
> return 1; /* #UD */
> /* fall through */
> case 6:
> - if (val & 0xffffffff00000000ULL)
> + if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu))
> return -1; /* #GP */
> vcpu->arch.dr6 = (val & DR6_VOLATILE) | DR6_FIXED_1;
> kvm_update_dr6(vcpu);
> @@ -779,7 +788,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
> return 1; /* #UD */
> /* fall through */
> default: /* 7 */
> - if (val & 0xffffffff00000000ULL)
> + if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu))
> return -1; /* #GP */
> vcpu->arch.dr7 = (val & DR7_VOLATILE) | DR7_FIXED_1;
> kvm_update_dr7(vcpu);
>
Do you get this if the input register has bit 31 set?
Paolo
Il 15/06/2014 15:12, Nadav Amit ha scritto:
> This patch-set resolves several emulator bugs. Each fix is independent of the
> others. The DR6/7 bug can occur during DR-access exit (regardless to
> unrestricted mode, MMIO and SPT).
>
> Thanks for reviewing the patches,
> Nadav
>
> Nadav Amit (6):
> KVM: x86: bit-ops emulation ignores offset on 64-bit
> KVM: x86: Wrong emulation on 'xadd X, X'
> KVM: x86: Inter privilage level ret emulation is not implemeneted
> KVM: x86: emulation of dword cmov on long-mode should clear [63:32]
> KVM: x86: NOP emulation clears (incorrectly) the high 32-bits of RAX
> KVM: x86: check DR6/7 high-bits are clear only on long-mode
>
> arch/x86/kvm/emulate.c | 31 ++++++++++++++++++++-----------
> arch/x86/kvm/x86.c | 13 +++++++++++--
> 2 files changed, 31 insertions(+), 13 deletions(-)
>
I applied these locally. Can you prepare testcases for patches 1, 2, 4
and 5? Perhaps patch 6 too if it's easy to reproduce.
Paolo
On 6/16/14, 1:17 PM, Paolo Bonzini wrote:
> Il 15/06/2014 15:13, Nadav Amit ha scritto:
>> From: Nadav Amit <[email protected]>
>>
>> When the guest sets DR6 and DR7, KVM asserts the high 32-bits are
>> clear, and
>> otherwise injects a #GP exception. This exception should only be
>> injected only
>> if running in long-mode.
>>
>> Signed-off-by: Nadav Amit <[email protected]>
>> ---
>> arch/x86/kvm/x86.c | 13 +++++++++++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 57eac30..71fe841 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -756,6 +756,15 @@ static void kvm_update_dr7(struct kvm_vcpu *vcpu)
>> vcpu->arch.switch_db_regs |= KVM_DEBUGREG_BP_ENABLED;
>> }
>>
>> +static bool is_64_bit_mode(struct kvm_vcpu *vcpu)
>> +{
>> + int cs_db, cs_l;
>> + if (!is_long_mode(vcpu))
>> + return false;
>> + kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
>> + return cs_l;
>> +}
>> +
>> static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long
>> val)
>> {
>> switch (dr) {
>> @@ -769,7 +778,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int
>> dr, unsigned long val)
>> return 1; /* #UD */
>> /* fall through */
>> case 6:
>> - if (val & 0xffffffff00000000ULL)
>> + if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu))
>> return -1; /* #GP */
>> vcpu->arch.dr6 = (val & DR6_VOLATILE) | DR6_FIXED_1;
>> kvm_update_dr6(vcpu);
>> @@ -779,7 +788,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int
>> dr, unsigned long val)
>> return 1; /* #UD */
>> /* fall through */
>> default: /* 7 */
>> - if (val & 0xffffffff00000000ULL)
>> + if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu))
>> return -1; /* #GP */
>> vcpu->arch.dr7 = (val & DR7_VOLATILE) | DR7_FIXED_1;
>> kvm_update_dr7(vcpu);
>>
>
> Do you get this if the input register has bit 31 set?
No. To be frank, the scenario may be considered a bit synthetic: the
guest assigns a value to a general-purpose register in 64-bit mode,
setting the high 32-bits to some non-zero value. Then, later, in 32-bit
mode, the guest performs MOV DR instruction. In between the two
assignments, the general purpose register is unmodified, so the high
32-bits of the general purpose registers are still set.
Note that this scenario does not occur when MOV DR is emulated, but when
handle_dr() is called. In this case, the entire 64-bits of the general
purpose register used for MOV DR are read, regardless to the execution
mode of the guest.
Nadav
Il 16/06/2014 12:33, Nadav Amit ha scritto:
>>
>> Do you get this if the input register has bit 31 set?
> No. To be frank, the scenario may be considered a bit synthetic: the
> guest assigns a value to a general-purpose register in 64-bit mode,
> setting the high 32-bits to some non-zero value. Then, later, in 32-bit
> mode, the guest performs MOV DR instruction. In between the two
> assignments, the general purpose register is unmodified, so the high
> 32-bits of the general purpose registers are still set.
>
> Note that this scenario does not occur when MOV DR is emulated, but when
> handle_dr() is called. In this case, the entire 64-bits of the general
> purpose register used for MOV DR are read, regardless to the execution
> mode of the guest.
I wonder if the same bug happens elsewhere. For example,
kvm_emulate_hypercall doesn't look at CS.L/CS.DB, which is really a
corner case but arguably also a bug. kvm_hv_hypercall instead does it
right.
Perhaps we need a variant of kvm_register_read that (on 64-bit hosts)
checks EFER/CS.L/CS.DB and masks the returned value accordingly. You
could call it kvm_register_readl.
Paolo
On 6/16/14, 2:09 PM, Paolo Bonzini wrote:
> Il 16/06/2014 12:33, Nadav Amit ha scritto:
>>>
>>> Do you get this if the input register has bit 31 set?
>> No. To be frank, the scenario may be considered a bit synthetic: the
>> guest assigns a value to a general-purpose register in 64-bit mode,
>> setting the high 32-bits to some non-zero value. Then, later, in 32-bit
>> mode, the guest performs MOV DR instruction. In between the two
>> assignments, the general purpose register is unmodified, so the high
>> 32-bits of the general purpose registers are still set.
>>
>> Note that this scenario does not occur when MOV DR is emulated, but when
>> handle_dr() is called. In this case, the entire 64-bits of the general
>> purpose register used for MOV DR are read, regardless to the execution
>> mode of the guest.
>
> I wonder if the same bug happens elsewhere. For example,
> kvm_emulate_hypercall doesn't look at CS.L/CS.DB, which is really a
> corner case but arguably also a bug. kvm_hv_hypercall instead does it
> right.
>
> Perhaps we need a variant of kvm_register_read that (on 64-bit hosts)
> checks EFER/CS.L/CS.DB and masks the returned value accordingly. You
> could call it kvm_register_readl.
There are two questions that come in mind:
1. Should we ignore CS.DB? It would make it consistent with
kvm_hv_hypercall and handle_dr. I think this is the proper behavior.
2. Reading CS.L once and masking all the registers (i.e., changing the
is_long_mode in kvm_emulate_hypercall to is_64_bit_mode) is likely to be
more efficient.
Regards,
Nadav
Il 16/06/2014 13:53, Nadav Amit ha scritto:
> On 6/16/14, 2:09 PM, Paolo Bonzini wrote:
>> Il 16/06/2014 12:33, Nadav Amit ha scritto:
>>>>
>>>> Do you get this if the input register has bit 31 set?
>>> No. To be frank, the scenario may be considered a bit synthetic: the
>>> guest assigns a value to a general-purpose register in 64-bit mode,
>>> setting the high 32-bits to some non-zero value. Then, later, in 32-bit
>>> mode, the guest performs MOV DR instruction. In between the two
>>> assignments, the general purpose register is unmodified, so the high
>>> 32-bits of the general purpose registers are still set.
>>>
>>> Note that this scenario does not occur when MOV DR is emulated, but when
>>> handle_dr() is called. In this case, the entire 64-bits of the general
>>> purpose register used for MOV DR are read, regardless to the execution
>>> mode of the guest.
>>
>> I wonder if the same bug happens elsewhere. For example,
>> kvm_emulate_hypercall doesn't look at CS.L/CS.DB, which is really a
>> corner case but arguably also a bug. kvm_hv_hypercall instead does it
>> right.
>>
>> Perhaps we need a variant of kvm_register_read that (on 64-bit hosts)
>> checks EFER/CS.L/CS.DB and masks the returned value accordingly. You
>> could call it kvm_register_readl.
>
> There are two questions that come in mind:
> 1. Should we ignore CS.DB? It would make it consistent with
> kvm_hv_hypercall and handle_dr. I think this is the proper behavior.
It depends on what you're using it for, but as a start yes.
> 2. Reading CS.L once and masking all the registers (i.e., changing the
> is_long_mode in kvm_emulate_hypercall to is_64_bit_mode) is likely to be
> more efficient.
Yes, for the case of kvm_emulate_hypercall. Then you can build
kvm_register_readl on top of is_64bit_mode and fix this bug with that
function. Did you check that handle_cr is unaffected?
Paolo
On 6/16/14, 5:56 PM, Paolo Bonzini wrote:
> Il 16/06/2014 13:53, Nadav Amit ha scritto:
>> On 6/16/14, 2:09 PM, Paolo Bonzini wrote:
>>> Il 16/06/2014 12:33, Nadav Amit ha scritto:
>>>>>
>>>>> Do you get this if the input register has bit 31 set?
>>>> No. To be frank, the scenario may be considered a bit synthetic: the
>>>> guest assigns a value to a general-purpose register in 64-bit mode,
>>>> setting the high 32-bits to some non-zero value. Then, later, in 32-bit
>>>> mode, the guest performs MOV DR instruction. In between the two
>>>> assignments, the general purpose register is unmodified, so the high
>>>> 32-bits of the general purpose registers are still set.
>>>>
>>>> Note that this scenario does not occur when MOV DR is emulated, but
>>>> when
>>>> handle_dr() is called. In this case, the entire 64-bits of the general
>>>> purpose register used for MOV DR are read, regardless to the execution
>>>> mode of the guest.
>>>
>>> I wonder if the same bug happens elsewhere. For example,
>>> kvm_emulate_hypercall doesn't look at CS.L/CS.DB, which is really a
>>> corner case but arguably also a bug. kvm_hv_hypercall instead does it
>>> right.
>>>
>>> Perhaps we need a variant of kvm_register_read that (on 64-bit hosts)
>>> checks EFER/CS.L/CS.DB and masks the returned value accordingly. You
>>> could call it kvm_register_readl.
>>
>> There are two questions that come in mind:
>> 1. Should we ignore CS.DB? It would make it consistent with
>> kvm_hv_hypercall and handle_dr. I think this is the proper behavior.
>
> It depends on what you're using it for, but as a start yes.
>
>> 2. Reading CS.L once and masking all the registers (i.e., changing the
>> is_long_mode in kvm_emulate_hypercall to is_64_bit_mode) is likely to be
>> more efficient.
>
> Yes, for the case of kvm_emulate_hypercall. Then you can build
> kvm_register_readl on top of is_64bit_mode and fix this bug with that
> function. Did you check that handle_cr is unaffected?
handle_cr is affected, and there are some other instances of affected
register reads. Actually, most of the vmx instruction exit handling
routines ignore long-mode and cs.l when considering the register operand
size.
I'll make the necessary changes and resubmit.
Nadav
Nadav Amit <[email protected]> writes:
> The emulator does not emulate the xadd instruction correctly if the two
> operands are the same. In this (unlikely) situation the result should be the
> sum of X and X (2X) when it is currently X. The solution is to first perform
> writeback to the source, before writing to the destination. The only
> instruction which should be affected is xadd, as the other instructions that
> perform writeback to the source use the extended accumlator (e.g., RAX:RDX).
>
> Signed-off-by: Nadav Amit <[email protected]>
> ---
> arch/x86/kvm/emulate.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index f0b0a10..3c8d867 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -4711,17 +4711,17 @@ special_insn:
> goto done;
>
> writeback:
> - if (!(ctxt->d & NoWrite)) {
> - rc = writeback(ctxt, &ctxt->dst);
> - if (rc != X86EMUL_CONTINUE)
> - goto done;
> - }
> if (ctxt->d & SrcWrite) {
> BUG_ON(ctxt->src.type == OP_MEM || ctxt->src.type == OP_MEM_STR);
While we are here, I think we should replace this BUG_ON with a warning
and return X86EMUL_UNHANDLEABLE if the condition is true.
> rc = writeback(ctxt, &ctxt->src);
> if (rc != X86EMUL_CONTINUE)
> goto done;
> }
> + if (!(ctxt->d & NoWrite)) {
> + rc = writeback(ctxt, &ctxt->dst);
> + if (rc != X86EMUL_CONTINUE)
> + goto done;
> + }
>
> /*
> * restore dst type in case the decoding will be reused
Il 16/06/2014 19:38, Bandan Das ha scritto:
> Nadav Amit <[email protected]> writes:
>
>> The emulator does not emulate the xadd instruction correctly if the two
>> operands are the same. In this (unlikely) situation the result should be the
>> sum of X and X (2X) when it is currently X. The solution is to first perform
>> writeback to the source, before writing to the destination. The only
>> instruction which should be affected is xadd, as the other instructions that
>> perform writeback to the source use the extended accumlator (e.g., RAX:RDX).
>>
>> Signed-off-by: Nadav Amit <[email protected]>
>> ---
>> arch/x86/kvm/emulate.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index f0b0a10..3c8d867 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -4711,17 +4711,17 @@ special_insn:
>> goto done;
>>
>> writeback:
>> - if (!(ctxt->d & NoWrite)) {
>> - rc = writeback(ctxt, &ctxt->dst);
>> - if (rc != X86EMUL_CONTINUE)
>> - goto done;
>> - }
>> if (ctxt->d & SrcWrite) {
>> BUG_ON(ctxt->src.type == OP_MEM || ctxt->src.type == OP_MEM_STR);
> While we are here, I think we should replace this BUG_ON with a warning
> and return X86EMUL_UNHANDLEABLE if the condition is true.
Sure, please post a patch and I'll apply it right away.
Paolo
Paolo Bonzini <[email protected]> writes:
> Il 16/06/2014 19:38, Bandan Das ha scritto:
>> Nadav Amit <[email protected]> writes:
>>
>>> The emulator does not emulate the xadd instruction correctly if the two
>>> operands are the same. In this (unlikely) situation the result should be the
>>> sum of X and X (2X) when it is currently X. The solution is to first perform
>>> writeback to the source, before writing to the destination. The only
>>> instruction which should be affected is xadd, as the other instructions that
>>> perform writeback to the source use the extended accumlator (e.g., RAX:RDX).
>>>
>>> Signed-off-by: Nadav Amit <[email protected]>
>>> ---
>>> arch/x86/kvm/emulate.c | 10 +++++-----
>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>>> index f0b0a10..3c8d867 100644
>>> --- a/arch/x86/kvm/emulate.c
>>> +++ b/arch/x86/kvm/emulate.c
>>> @@ -4711,17 +4711,17 @@ special_insn:
>>> goto done;
>>>
>>> writeback:
>>> - if (!(ctxt->d & NoWrite)) {
>>> - rc = writeback(ctxt, &ctxt->dst);
>>> - if (rc != X86EMUL_CONTINUE)
>>> - goto done;
>>> - }
>>> if (ctxt->d & SrcWrite) {
>>> BUG_ON(ctxt->src.type == OP_MEM || ctxt->src.type == OP_MEM_STR);
>> While we are here, I think we should replace this BUG_ON with a warning
>> and return X86EMUL_UNHANDLEABLE if the condition is true.
>
> Sure, please post a patch and I'll apply it right away.
Well, I meant it more as a review and "suggested changes" to this patchset
Nadav posted, but yeah, if you prefer, I can post a change myself. I will
make a pass through other uses of BUG() in the code too.
> Paolo
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 17/06/2014 17:35, Bandan Das ha scritto:
> Well, I meant it more as a review and "suggested changes" to this patchset
> Nadav posted, but yeah, if you prefer, I can post a change myself. I will
> make a pass through other uses of BUG() in the code too.
I'd prefer that, there's no need to make Nadav do even more work.
Paolo
VMX instructions use 32-bit operands in 32-bit mode, and 64-bit operands in
64-bit mode. The current implementation is broken since it does not use the
register operands correctly, and always uses 64-bit for reads and writes.
Moreover, write to memory in vmwrite only considers long-mode, so it ignores
cs.l. This patch fixes this behavior. The field of vmread/vmwrite is kept
intentionally as 64-bit read since if bits [63:32] are not cleared the
instruction should fail, according to Intel SDM.
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/vmx.c | 8 ++++----
arch/x86/kvm/x86.h | 9 +++++++++
2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index cbfbb8b..75dc888 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6397,7 +6397,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
* on the guest's mode (32 or 64 bit), not on the given field's length.
*/
if (vmx_instruction_info & (1u << 10)) {
- kvm_register_write(vcpu, (((vmx_instruction_info) >> 3) & 0xf),
+ kvm_register_writel(vcpu, (((vmx_instruction_info) >> 3) & 0xf),
field_value);
} else {
if (get_vmx_mem_address(vcpu, exit_qualification,
@@ -6434,14 +6434,14 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
return 1;
if (vmx_instruction_info & (1u << 10))
- field_value = kvm_register_read(vcpu,
+ field_value = kvm_register_readl(vcpu,
(((vmx_instruction_info) >> 3) & 0xf));
else {
if (get_vmx_mem_address(vcpu, exit_qualification,
vmx_instruction_info, &gva))
return 1;
if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, gva,
- &field_value, (is_long_mode(vcpu) ? 8 : 4), &e)) {
+ &field_value, (is_64_bit_mode(vcpu) ? 8 : 4), &e)) {
kvm_inject_page_fault(vcpu, &e);
return 1;
}
@@ -6571,7 +6571,7 @@ static int handle_invept(struct kvm_vcpu *vcpu)
}
vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
- type = kvm_register_read(vcpu, (vmx_instruction_info >> 28) & 0xf);
+ type = kvm_register_readl(vcpu, (vmx_instruction_info >> 28) & 0xf);
types = (nested_vmx_ept_caps >> VMX_EPT_EXTENT_SHIFT) & 6;
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index c5b61a7..306a1b7 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -126,6 +126,15 @@ static inline unsigned long kvm_register_readl(struct kvm_vcpu *vcpu,
return is_64_bit_mode(vcpu) ? val : (u32)val;
}
+static inline void kvm_register_writel(struct kvm_vcpu *vcpu,
+ enum kvm_reg reg,
+ unsigned long val)
+{
+ if (!is_64_bit_mode(vcpu))
+ val = (u32)val;
+ return kvm_register_write(vcpu, reg, val);
+}
+
void kvm_before_handle_nmi(struct kvm_vcpu *vcpu);
void kvm_after_handle_nmi(struct kvm_vcpu *vcpu);
int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
--
1.9.1
The current emulation of bit operations ignores the offset from the destination
on 64-bit target memory operands. This patch fixes this behavior.
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/emulate.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index e4e833d..f0b0a10 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1220,12 +1220,14 @@ static void fetch_bit_operand(struct x86_emulate_ctxt *ctxt)
long sv = 0, mask;
if (ctxt->dst.type == OP_MEM && ctxt->src.type == OP_REG) {
- mask = ~(ctxt->dst.bytes * 8 - 1);
+ mask = ~((long)ctxt->dst.bytes * 8 - 1);
if (ctxt->src.bytes == 2)
sv = (s16)ctxt->src.val & (s16)mask;
else if (ctxt->src.bytes == 4)
sv = (s32)ctxt->src.val & (s32)mask;
+ else
+ sv = (s64)ctxt->src.val & (s64)mask;
ctxt->dst.addr.mem.ea += (sv >> 3);
}
--
1.9.1
Currently, the hypercall handling routine only considers LME as an indication
to whether the guest uses 32/64-bit mode. This is incosistent with hyperv
hypercalls handling and against the common sense of considering cs.l as well.
This patch uses is_64_bit_mode instead of is_long_mode for that matter. In
addition, the result is masked in respect to the guest execution mode. Last, it
changes kvm_hv_hypercall to use is_64_bit_mode as well to simplify the code.
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/x86.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f32a025..c6dfe47 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5662,7 +5662,6 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
u64 param, ingpa, outgpa, ret;
uint16_t code, rep_idx, rep_cnt, res = HV_STATUS_SUCCESS, rep_done = 0;
bool fast, longmode;
- int cs_db, cs_l;
/*
* hypercall generates UD from non zero cpl and real mode
@@ -5673,8 +5672,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
return 0;
}
- kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
- longmode = is_long_mode(vcpu) && cs_l == 1;
+ longmode = is_64_bit_mode(vcpu);
if (!longmode) {
param = ((u64)kvm_register_read(vcpu, VCPU_REGS_RDX) << 32) |
@@ -5739,7 +5737,7 @@ static void kvm_pv_kick_cpu_op(struct kvm *kvm, unsigned long flags, int apicid)
int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
{
unsigned long nr, a0, a1, a2, a3, ret;
- int r = 1;
+ int op_64_bit, r = 1;
if (kvm_hv_hypercall_enabled(vcpu->kvm))
return kvm_hv_hypercall(vcpu);
@@ -5752,7 +5750,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
trace_kvm_hypercall(nr, a0, a1, a2, a3);
- if (!is_long_mode(vcpu)) {
+ op_64_bit = is_64_bit_mode(vcpu);
+ if (!op_64_bit) {
nr &= 0xFFFFFFFF;
a0 &= 0xFFFFFFFF;
a1 &= 0xFFFFFFFF;
@@ -5778,6 +5777,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
break;
}
out:
+ if (!op_64_bit)
+ ret = (u32)ret;
kvm_register_write(vcpu, VCPU_REGS_RAX, ret);
++vcpu->stat.hypercalls;
return r;
--
1.9.1
On 32-bit mode only bits [31:0] of the CR should be used for setting the CR
value. Otherwise, the host may incorrectly assume the value is invalid if bits
[63:32] are not zero. Moreover, the CR is currently being read twice when CR8
is used. Last, nested mov-cr exiting is modified to handle the CR value
correctly as well.
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/vmx.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c0f53a0..cbfbb8b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5039,7 +5039,7 @@ static int handle_cr(struct kvm_vcpu *vcpu)
reg = (exit_qualification >> 8) & 15;
switch ((exit_qualification >> 4) & 3) {
case 0: /* mov to cr */
- val = kvm_register_read(vcpu, reg);
+ val = kvm_register_readl(vcpu, reg);
trace_kvm_cr_write(cr, val);
switch (cr) {
case 0:
@@ -5056,7 +5056,7 @@ static int handle_cr(struct kvm_vcpu *vcpu)
return 1;
case 8: {
u8 cr8_prev = kvm_get_cr8(vcpu);
- u8 cr8 = kvm_register_read(vcpu, reg);
+ u8 cr8 = (u8)val;
err = kvm_set_cr8(vcpu, cr8);
kvm_complete_insn_gp(vcpu, err);
if (irqchip_in_kernel(vcpu->kvm))
@@ -6751,7 +6751,7 @@ static bool nested_vmx_exit_handled_cr(struct kvm_vcpu *vcpu,
unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
int cr = exit_qualification & 15;
int reg = (exit_qualification >> 8) & 15;
- unsigned long val = kvm_register_read(vcpu, reg);
+ unsigned long val = kvm_register_readl(vcpu, reg);
switch ((exit_qualification >> 4) & 3) {
case 0: /* mov to cr */
--
1.9.1
From: Nadav Amit <[email protected]>
When the guest sets DR6 and DR7, KVM asserts the high 32-bits are clear, and
otherwise injects a #GP exception. This exception should only be injected only
if running in long-mode.
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/vmx.c | 2 +-
arch/x86/kvm/x86.h | 18 ++++++++++++++++++
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 801332e..c0f53a0 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5165,7 +5165,7 @@ static int handle_dr(struct kvm_vcpu *vcpu)
return 1;
kvm_register_write(vcpu, reg, val);
} else
- if (kvm_set_dr(vcpu, dr, kvm_register_read(vcpu, reg)))
+ if (kvm_set_dr(vcpu, dr, kvm_register_readl(vcpu, reg)))
return 1;
skip_emulated_instruction(vcpu);
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 8c97bac..c5b61a7 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -47,6 +47,16 @@ static inline int is_long_mode(struct kvm_vcpu *vcpu)
#endif
}
+static inline bool is_64_bit_mode(struct kvm_vcpu *vcpu)
+{
+ int cs_db, cs_l;
+
+ if (!is_long_mode(vcpu))
+ return false;
+ kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
+ return cs_l;
+}
+
static inline bool mmu_is_nested(struct kvm_vcpu *vcpu)
{
return vcpu->arch.walk_mmu == &vcpu->arch.nested_mmu;
@@ -108,6 +118,14 @@ static inline bool vcpu_match_mmio_gpa(struct kvm_vcpu *vcpu, gpa_t gpa)
return false;
}
+static inline unsigned long kvm_register_readl(struct kvm_vcpu *vcpu,
+ enum kvm_reg reg)
+{
+ unsigned long val = kvm_register_read(vcpu, reg);
+
+ return is_64_bit_mode(vcpu) ? val : (u32)val;
+}
+
void kvm_before_handle_nmi(struct kvm_vcpu *vcpu);
void kvm_after_handle_nmi(struct kvm_vcpu *vcpu);
int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
--
1.9.1
On long-mode the current NOP (0x90) emulation still writes back to RAX. As a
result, EAX is zero-extended and the high 32-bits of RAX are cleared.
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/emulate.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b354531..eb93eb4 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4663,8 +4663,9 @@ special_insn:
break;
case 0x90 ... 0x97: /* nop / xchg reg, rax */
if (ctxt->dst.addr.reg == reg_rmw(ctxt, VCPU_REGS_RAX))
- break;
- rc = em_xchg(ctxt);
+ ctxt->dst.type = OP_NONE;
+ else
+ rc = em_xchg(ctxt);
break;
case 0x98: /* cbw/cwde/cdqe */
switch (ctxt->op_bytes) {
--
1.9.1
Return unhandlable error on inter-privilage level ret instruction. This is
since the current emulation does not check the privilage level correctly when
loading the CS, and does not pop RSP/SS as needed.
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/emulate.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 3c8d867..0183350 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2019,6 +2019,7 @@ static int em_ret_far(struct x86_emulate_ctxt *ctxt)
{
int rc;
unsigned long cs;
+ int cpl = ctxt->ops->cpl(ctxt);
rc = emulate_pop(ctxt, &ctxt->_eip, ctxt->op_bytes);
if (rc != X86EMUL_CONTINUE)
@@ -2028,6 +2029,9 @@ static int em_ret_far(struct x86_emulate_ctxt *ctxt)
rc = emulate_pop(ctxt, &cs, ctxt->op_bytes);
if (rc != X86EMUL_CONTINUE)
return rc;
+ /* Outer-privilage level return is not implemented */
+ if (ctxt->mode >= X86EMUL_MODE_PROT16 && (cs & 3) > cpl)
+ return X86EMUL_UNHANDLEABLE;
rc = load_segment_descriptor(ctxt, (u16)cs, VCPU_SREG_CS);
return rc;
}
--
1.9.1
The emulator does not emulate the xadd instruction correctly if the two
operands are the same. In this (unlikely) situation the result should be the
sum of X and X (2X) when it is currently X. The solution is to first perform
writeback to the source, before writing to the destination. The only
instruction which should be affected is xadd, as the other instructions that
perform writeback to the source use the extended accumlator (e.g., RAX:RDX).
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/emulate.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f0b0a10..3c8d867 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4711,17 +4711,17 @@ special_insn:
goto done;
writeback:
- if (!(ctxt->d & NoWrite)) {
- rc = writeback(ctxt, &ctxt->dst);
- if (rc != X86EMUL_CONTINUE)
- goto done;
- }
if (ctxt->d & SrcWrite) {
BUG_ON(ctxt->src.type == OP_MEM || ctxt->src.type == OP_MEM_STR);
rc = writeback(ctxt, &ctxt->src);
if (rc != X86EMUL_CONTINUE)
goto done;
}
+ if (!(ctxt->d & NoWrite)) {
+ rc = writeback(ctxt, &ctxt->dst);
+ if (rc != X86EMUL_CONTINUE)
+ goto done;
+ }
/*
* restore dst type in case the decoding will be reused
--
1.9.1
Even if the condition of cmov is not satisfied, bits[63:32] should be cleared.
This is clearly stated in Intel's CMOVcc documentation. The solution is to
reassign the destination onto itself if the condition is unsatisfied. For that
matter the original destination value needs to be read.
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/emulate.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 0183350..b354531 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3905,7 +3905,7 @@ static const struct opcode twobyte_table[256] = {
N, N,
N, N, N, N, N, N, N, N,
/* 0x40 - 0x4F */
- X16(D(DstReg | SrcMem | ModRM | Mov)),
+ X16(D(DstReg | SrcMem | ModRM)),
/* 0x50 - 0x5F */
N, N, N, N, N, N, N, N, N, N, N, N, N, N, N, N,
/* 0x60 - 0x6F */
@@ -4799,8 +4799,10 @@ twobyte_insn:
ops->get_dr(ctxt, ctxt->modrm_reg, &ctxt->dst.val);
break;
case 0x40 ... 0x4f: /* cmov */
- ctxt->dst.val = ctxt->dst.orig_val = ctxt->src.val;
- if (!test_cc(ctxt->b, ctxt->eflags))
+ if (test_cc(ctxt->b, ctxt->eflags))
+ ctxt->dst.val = ctxt->src.val;
+ else if (ctxt->mode != X86EMUL_MODE_PROT64 ||
+ ctxt->op_bytes != 4)
ctxt->dst.type = OP_NONE; /* no writeback */
break;
case 0x80 ... 0x8f: /* jnz rel, etc*/
--
1.9.1
This patch-set resolves several emulator bugs. Each fix is independent of the
others. The DR6 bug can occur during DR-access exit (regardless to
unrestricted mode, MMIO and SPT).
Changes in v2:
Introduced kvm_register_readl and kvm_register_writel which consider long-mode
and cs.l when reading the registers.
Fixing the register read to respect 32/64 bit in hypercall handling, CR exit
handling and VMX instructions handling.
Thanks for re-reviewing the patch
Nadav Amit (9):
KVM: x86: bit-ops emulation ignores offset on 64-bit
KVM: x86: Wrong emulation on 'xadd X, X'
KVM: x86: Inter privilage level ret emulation is not implemeneted
KVM: x86: emulation of dword cmov on long-mode should clear [63:32]
KVM: x86: NOP emulation clears (incorrectly) the high 32-bits of RAX
KVM: x86: check DR6/7 high-bits are clear only on long-mode
KVM: x86: Hypercall handling does not considers opsize correctly
KVM: vmx: handle_cr ignores 32/64-bit mode
KVM: vmx: vmx instructions handling does not consider cs.l
arch/x86/kvm/emulate.c | 31 ++++++++++++++++++++-----------
arch/x86/kvm/vmx.c | 16 ++++++++--------
arch/x86/kvm/x86.c | 11 ++++++-----
arch/x86/kvm/x86.h | 27 +++++++++++++++++++++++++++
4 files changed, 61 insertions(+), 24 deletions(-)
--
1.9.1
Il 18/06/2014 16:19, Nadav Amit ha scritto:
> VMX instructions use 32-bit operands in 32-bit mode, and 64-bit operands in
> 64-bit mode. The current implementation is broken since it does not use the
> register operands correctly, and always uses 64-bit for reads and writes.
> Moreover, write to memory in vmwrite only considers long-mode, so it ignores
> cs.l. This patch fixes this behavior. The field of vmread/vmwrite is kept
> intentionally as 64-bit read since if bits [63:32] are not cleared the
> instruction should fail, according to Intel SDM.
This is not how I read the SDM:
"These instructions fail if given, in 64-bit mode, an operand that sets
an encoding bit beyond bit 32." (Section 24.11.1.2)
"Outside IA-32e mode, the source operand has 32 bits, regardless of the
value of CS.D. In 64-bit mode, the source operand has 64 bits; however,
if bits 63:32 of the source operand are not zero, VMREAD will fail due
to an attempt to access an unsupported VMCS component (see operation
section)." (Description of VMREAD in Chapter 30).
I'll fix up the patch myself.
Paolo
Il 18/06/2014 16:19, Nadav Amit ha scritto:
> This patch-set resolves several emulator bugs. Each fix is independent of the
> others. The DR6 bug can occur during DR-access exit (regardless to
> unrestricted mode, MMIO and SPT).
>
>
> Changes in v2:
>
> Introduced kvm_register_readl and kvm_register_writel which consider long-mode
> and cs.l when reading the registers.
>
> Fixing the register read to respect 32/64 bit in hypercall handling, CR exit
> handling and VMX instructions handling.
>
> Thanks for re-reviewing the patch
>
> Nadav Amit (9):
> KVM: x86: bit-ops emulation ignores offset on 64-bit
> KVM: x86: Wrong emulation on 'xadd X, X'
> KVM: x86: Inter privilage level ret emulation is not implemeneted
> KVM: x86: emulation of dword cmov on long-mode should clear [63:32]
> KVM: x86: NOP emulation clears (incorrectly) the high 32-bits of RAX
> KVM: x86: check DR6/7 high-bits are clear only on long-mode
> KVM: x86: Hypercall handling does not considers opsize correctly
> KVM: vmx: handle_cr ignores 32/64-bit mode
> KVM: vmx: vmx instructions handling does not consider cs.l
>
> arch/x86/kvm/emulate.c | 31 ++++++++++++++++++++-----------
> arch/x86/kvm/vmx.c | 16 ++++++++--------
> arch/x86/kvm/x86.c | 11 ++++++-----
> arch/x86/kvm/x86.h | 27 +++++++++++++++++++++++++++
> 4 files changed, 61 insertions(+), 24 deletions(-)
>
Thanks, looks good.
Paolo
On 6/18/14, 6:41 PM, Paolo Bonzini wrote:
> Il 18/06/2014 16:19, Nadav Amit ha scritto:
>> VMX instructions use 32-bit operands in 32-bit mode, and 64-bit
>> operands in
>> 64-bit mode. The current implementation is broken since it does not
>> use the
>> register operands correctly, and always uses 64-bit for reads and writes.
>> Moreover, write to memory in vmwrite only considers long-mode, so it
>> ignores
>> cs.l. This patch fixes this behavior. The field of vmread/vmwrite is
>> kept
>> intentionally as 64-bit read since if bits [63:32] are not cleared the
>> instruction should fail, according to Intel SDM.
>
> This is not how I read the SDM:
>
> "These instructions fail if given, in 64-bit mode, an operand that sets
> an encoding bit beyond bit 32." (Section 24.11.1.2)
>
> "Outside IA-32e mode, the source operand has 32 bits, regardless of the
> value of CS.D. In 64-bit mode, the source operand has 64 bits; however,
> if bits 63:32 of the source operand are not zero, VMREAD will fail due
> to an attempt to access an unsupported VMCS component (see operation
> section)." (Description of VMREAD in Chapter 30).
>
> I'll fix up the patch myself.
>
Perhaps I am missing something, but I don't see where my mistake is.
The VMREAD source operand is always read as 64-bits and I made no
changes there. Therefore, if bits 63:32 are not zero, the instruction
should fail when attempting to access the field.
The value in the source operand of VMWRITE which represents the value
which should be written is zero-extended outside 64-bit mode.
Quoting: "The effective size of the primary source operand, which may be
a register or in memory, is always 32 bits outside IA-32e mode (the
setting of CS.D is ignored with respect to operand size) and 64 bits in
64-bit mode." (Description of VMWRITE in chapter 30).
Regards,
Nadav
Il 18/06/2014 18:01, Nadav Amit ha scritto:
>>
>
> Perhaps I am missing something, but I don't see where my mistake is.
> The VMREAD source operand is always read as 64-bits and I made no
> changes there. Therefore, if bits 63:32 are not zero, the instruction
> should fail when attempting to access the field.
In 32-bit mode, it should be read as 32 bits: "Outside IA-32e mode, the
source operand has 32 bits, regardless of the value of CS.D" (so it's
never 16 bits, but it's also never 64 bits).
Paolo
On Jun 18, 2014, at 7:06 PM, Paolo Bonzini <[email protected]> wrote:
> Il 18/06/2014 18:01, Nadav Amit ha scritto:
>>>
>>
>> Perhaps I am missing something, but I don't see where my mistake is.
>> The VMREAD source operand is always read as 64-bits and I made no
>> changes there. Therefore, if bits 63:32 are not zero, the instruction
>> should fail when attempting to access the field.
>
> In 32-bit mode, it should be read as 32 bits: "Outside IA-32e mode, the source operand has 32 bits, regardless of the value of CS.D" (so it's never 16 bits, but it's also never 64 bits).
Oh. I now get it. I misunderstood what the SDM said, as I was thinking that 62:32 will lead to failure also on 32-bit mode.
If you fix it, please fix both VMREAD and VMWRITE. If not, I would resubmit.
Thanks,
Nadav-
Il 18/06/2014 19:51, Nadav Amit ha scritto:
> If you fix it, please fix both VMREAD and VMWRITE. If not, I would resubmit.
Yes, I'm fixing it myself.
Paolo