Hi Paolo,
On 12/13/2016 11:09 AM, Paolo Bonzini wrote:
On 12/12/2016 18:51, Brijesh Singh wrote:
As per the AMD BKDG [1] Section 2.7.1, we should not be using any of
these instruction for MMIO access, the behavior is undefined.
The question is, do we really need to add logic to detect the cross-page
MMIO accesses and push/pop mem operations so that we pass the
kvm-unit-test or we should update the unit test? Like you said
cross-page MMIO access detection is going to be a bit tricky.
Actually there is a nice trick you can do to support cross-page
MMIO access detection:
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 37cd31645d45..754d251dc611 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4549,6 +4549,7 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
*/
if (vcpu->arch.gpa_available &&
!emulator_is_string_op(ctxt) &&
+ (addr & ~PAGE_MASK) == (exception->address & ~PAGE_MASK) &&
vcpu_is_mmio_gpa(vcpu, addr, exception->address, write)) {
gpa = exception->address;
goto mmio;
It fixes the testcase for push/pop with two memory ops too,
but it's not reliable, so your change for TwoMemOp is still
necessary. Feel free to include it in your patch!
Regarding the replacement of emulator_is_string_op with
emulator_is_two_memory_op, what about REP prefixes? In that
case I think that you do need to reject string ops. So the
function would have to reject all TwoMemOps, and REP-prefixed
String operations.
Since now we are going to perform multiple conditional checks before
concluding that its safe to use HW provided GPA. How about if we add two
functions "emulator_is_rep_string_op" and "emulator_is_two_mem_op" into
emulator.c and use these functions inside the x86.c to determine if its
safe to use HW provided gpa?
Please let me know if you are okay with this approach.
diff --git a/arch/x86/include/asm/kvm_emulate.h
b/arch/x86/include/asm/kvm_emulate.h
index 777eea2..29e44cb 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -441,6 +441,7 @@ int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
int emulate_int_real(struct x86_emulate_ctxt *ctxt, int irq);
void emulator_invalidate_register_cache(struct x86_emulate_ctxt *ctxt);
void emulator_writeback_register_cache(struct x86_emulate_ctxt *ctxt);
-bool emulator_is_string_op(struct x86_emulate_ctxt *ctxt);
+bool emulator_is_rep_string_op(struct x86_emulate_ctxt *ctxt);
+bool emulator_is_two_mem_op(struct x86_emulate_ctxt *ctxt);
#endif /* _ASM_X86_KVM_X86_EMULATE_H */
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 8e7d09f..16149ad 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -5485,10 +5485,12 @@ void emulator_writeback_register_cache(struct
x86_emulate_ctxt *ctxt)
writeback_registers(ctxt);
}
-bool emulator_is_string_op(struct x86_emulate_ctxt *ctxt)
+bool emulator_is_rep_string_op(struct x86_emulate_ctxt *ctxt)
{
- if (ctxt->d & String)
- return true;
+ return ctxt->rep_prefix && (ctxt->d & String) ? true: false;
+}
- return false;
+bool emulator_is_two_mem_op(struct x86_emulate_ctxt *ctxt)
+{
+ return ctxt->d & TwoMemOp ? true : false;
}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 640527b..0bc814a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4548,6 +4548,12 @@ static const struct read_write_emulator_ops
write_emultor = {
.write = true,
};
+static bool emulator_can_use_gpa(struct x86_emulate_ctxt *ctxt)
+{
+ return emulator_is_rep_string_op(ctxt) &&
+ emulator_is_two_mem_op(ctxt) ? true : false;
+}
+
static int emulator_read_write_onepage(unsigned long addr, void *val,
unsigned int bytes,
struct x86_exception *exception,
@@ -4568,7 +4574,7 @@ static int emulator_read_write_onepage(unsigned
long addr, void *val,
* occurred.
*/
if (vcpu->arch.gpa_available &&
- !emulator_is_string_op(ctxt) &&
+ !emulator_can_use_gpa(ctxt) &&
vcpu_is_mmio_gpa(vcpu, addr, exception->address, write) &&
(addr & ~PAGE_MASK) == (exception->address & ~PAGE_MASK)) {
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
