| Message ID | 1403101166-23616-10-git-send-email-namit@cs.technion.ac.il (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Jun 18, 2014, at 7:06 PM, Paolo Bonzini <pbonzini@redhat.com> 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-- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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 <namit@cs.technion.ac.il> --- arch/x86/kvm/vmx.c | 8 ++++---- arch/x86/kvm/x86.h | 9 +++++++++ 2 files changed, 13 insertions(+), 4 deletions(-)