KVM: X86: remove read buffer for mmio read
PATCH 1/2 KVM: X86: remove read buffer for mmio read
After commit f78146b0f9230765c6315b2e14f56112513389ad:
KVM: Fix page-crossing MMIO
MMIO that are split across a page boundary are currently broken - the code does not expect to be aborted by the exit to userspace for the first MMIO fragment.
This patch fixes the problem by generalizing the current code for handling 16-byte MMIOs to handle a number of "fragments", and changes the MMIO code to create those fragments.
Multiple MMIO reads can be merged into mmio_fragments, the read buffer is not needed anymore
--- arch/x86/include/asm/kvm_emulate.h | 1 - arch/x86/kvm/emulate.c | 43 ++++------------------------------- arch/x86/kvm/x86.c | 2 - 3 files changed, 5 insertions(+), 41 deletions(-)
arch/x86/include/asm/kvm_emulate.h +0 -1
@@ -286,7 +286,6 @@ struct x86_emulate_ctxt {struct operand *memopp;struct fetch_cache fetch;struct read_cache io_read; -struct read_cache mem_read; };/* Repeat String Operation Prefix */
arch/x86/kvm/emulate.c +5 -38
68 lines not on this page · raw message
arch/x86/kvm/x86.c +0 -2
@@ -4399,8 +4399,6 @@ static void init_decode_cache(struct x86_emulate_ctxt *ctxt, ctxt->fetch.end =0 ; ctxt->io_read.pos =0 ; ctxt->io_read.end =0 ; - ctxt->mem_read.pos =0 ; - ctxt->mem_read.end =0 ; }static void init_emulate_ctxt (struct kvm_vcpu *vcpu)
Signature · 1 line
1.7.7.6
PATCH 2/2 KVM: X86: introduce set_mmio_exit_info
Introduce set_mmio_exit_info to cleanup the common code
--- arch/x86/kvm/x86.c | 33 +++++++++++++++++---------------- 1 files changed, 17 insertions(+), 16 deletions(-)
arch/x86/kvm/x86.c +17 -16
@@ -3755,9 +3755,6 @@ static int read_exit_mmio(struct kvm_vcpu *vcpu, gpa_t gpa,static int write_exit_mmio (struct kvm_vcpu *vcpu,gpa_t gpa,void *val,int bytes) { -struct kvm_mmio_fragment *frag = &vcpu->mmio_fragments[0 ]; - -memcpy (vcpu->run->mmio.data, frag->data, frag->len);return X86EMUL_CONTINUE ; }
@@ -3825,6 +3822,20 @@ mmio:return X86EMUL_CONTINUE ; } +static void set_mmio_exit_info (struct kvm_vcpu *vcpu, +struct kvm_mmio_fragment *frag,bool write) +{ +struct kvm_run *run = vcpu->run; + + run->exit_reason =KVM_EXIT_MMIO ; + run->mmio.phys_addr = frag->gpa; + run->mmio.len = frag->len; + run->mmio.is_write = vcpu->mmio_is_write = write; + +if (write) +memcpy (run->mmio.data, frag->data, frag->len); +} + int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr, void *val, unsigned int bytes, struct x86_exception *exception,
@@ -3864,14 +3875,10 @@ int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,return rc; gpa = vcpu->mmio_fragments[0 ].gpa; - vcpu->mmio_needed =1 ; vcpu->mmio_cur_fragment =0 ; - vcpu->run->mmio.len = vcpu->mmio_fragments[0 ].len; - vcpu->run->mmio.is_write = vcpu->mmio_is_write = ops->write; - vcpu->run->exit_reason =KVM_EXIT_MMIO ; - vcpu->run->mmio.phys_addr = gpa; +set_mmio_exit_info (vcpu, &vcpu->mmio_fragments[0 ], ops->write);return ops->read_write_exit_mmio (vcpu, gpa, val, bytes); }
@@ -5490,7 +5497,6 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)*/ static int complete_mmio (struct kvm_vcpu *vcpu) { -struct kvm_run *run = vcpu->run;struct kvm_mmio_fragment *frag;int r;
@@ -5501,7 +5507,7 @@ static int complete_mmio(struct kvm_vcpu *vcpu)/* Complete previous fragment */ frag = &vcpu->mmio_fragments[vcpu->mmio_cur_fragment++];if (!vcpu->mmio_is_write) -memcpy (frag->data, run->mmio.data, frag->len); +memcpy (frag->data, vcpu->run->mmio.data, frag->len);if (vcpu->mmio_cur_fragment == vcpu->mmio_nr_fragments) { vcpu->mmio_needed =0 ;if (vcpu->mmio_is_write)
@@ -5511,12 +5517,7 @@ static int complete_mmio(struct kvm_vcpu *vcpu) }/* Initiate next fragment */ ++frag; - run->exit_reason =KVM_EXIT_MMIO ; - run->mmio.phys_addr = frag->gpa; -if (vcpu->mmio_is_write) -memcpy (run->mmio.data, frag->data, frag->len); - run->mmio.len = frag->len; - run->mmio.is_write = vcpu->mmio_is_write; +set_mmio_exit_info (vcpu, frag, vcpu->mmio_is_write);return 0 ; }
Signature · 1 line
1.7.7.6
Quoted 30 lines · Xiao Guangrong
After commit
f78146b0f9230765c6315b2e14f56112513389ad:KVM: Fix page-crossing MMIO
MMIO that are split across a page boundary are currently broken - the code does not expect to be aborted by the exit to userspace for the first MMIO fragment.
This patch fixes the problem by generalizing the current code for handling 16-byte MMIOs to handle a number of "fragments", and changes the MMIO code to create those fragments.
Signed-off-by Avi Kivity <[email protected]>Signed-off-by Marcelo Tosatti <[email protected]>Multiple MMIO reads can be merged into
mmio_fragments, the read buffer is not needed anymoreSigned-off-by Xiao Guangrong <[email protected]>--- arch/x86/include/asm/kvm_emulate.h | 1 - arch/x86/kvm/emulate.c | 43 ++++------------------------------- arch/x86/kvm/x86.c | 2 - 3 files changed, 5 insertions(+), 41 deletions(-)arch/x86/include/asm/kvm_emulate.h +0
-0
arch/x86/include/asm/kvm_emulate.h struct x86_emulate_ctxt { · line 290
Quoted 6 lines
arch/x86/include/asm/kvm_emulate.h +0
-1@@ -286,7 +286,6 @@ struct x86_emulate_ctxt {struct operand *memopp;struct fetch_cache fetch;struct read_cache io_read; -struct read_cache mem_read; };
Suppose we have a RMW instruction. On the first entry to x86_emulate_insn() we'll drop to userspace and perform the read, and the seconds we'll read from the cache and complete the write.
Without the read cache this cannot work.
kvm-unit-tests.git has a test for xchg to mmio. Does it still work?
I agree this code has to go, but it needs to be replaced by something. Maybe a .valid flag in struct operand.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 42 lines · Avi Kivity
Quoted 30 lines · Xiao Guangrong
After commit
f78146b0f9230765c6315b2e14f56112513389ad:KVM: Fix page-crossing MMIO
MMIO that are split across a page boundary are currently broken - the code does not expect to be aborted by the exit to userspace for the first MMIO fragment.
This patch fixes the problem by generalizing the current code for handling 16-byte MMIOs to handle a number of "fragments", and changes the MMIO code to create those fragments.
Signed-off-by Avi Kivity <[email protected]>Signed-off-by Marcelo Tosatti <[email protected]>Multiple MMIO reads can be merged into
mmio_fragments, the read buffer is not needed anymoreSigned-off-by Xiao Guangrong <[email protected]>--- arch/x86/include/asm/kvm_emulate.h | 1 - arch/x86/kvm/emulate.c | 43 ++++------------------------------- arch/x86/kvm/x86.c | 2 - 3 files changed, 5 insertions(+), 41 deletions(-)arch/x86/include/asm/kvm_emulate.h +0
-0arch/x86/include/asm/kvm_emulate.h struct x86_emulate_ctxt { · line 290
Quoted 6 lines
arch/x86/include/asm/kvm_emulate.h +0
-1@@ -286,7 +286,6 @@ struct x86_emulate_ctxt {struct operand *memopp;struct fetch_cache fetch;struct read_cache io_read; -struct read_cache mem_read; };Suppose we have a RMW instruction. On the first entry to
x86_emulate_insn()we'll drop to userspace and perform the read, and the seconds we'll read from the cache and complete the write.
Quoted 2 lines
Without the read cache this cannot work.
Cache is needed to emulate instructions that need more than one read that can go to MMIO.
Quoted 5 lines
kvm-unit-tests.git has a test for xchg to mmio. Does it still work?
I agree this code has to go, but it needs to be replaced by something. Maybe a .valid flag in struct operand.
Valid will not enough for that.
--
Gleb.
Quoted 7 lines · Gleb Natapov
Quoted 6 lines
kvm-unit-tests.git has a test for xchg to mmio. Does it still work?
I agree this code has to go, but it needs to be replaced by something. Maybe a .valid flag in struct operand.
Valid will not enough for that.
If me make everything go through operands, why not?
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 7 lines · Gleb Natapov
Quoted 6 lines
kvm-unit-tests.git has a test for xchg to mmio. Does it still work?
I agree this code has to go, but it needs to be replaced by something. Maybe a .valid flag in struct operand.
Valid will not enough for that.
If we make everything go through operands, any reason why not?
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 11 lines · Avi Kivity
Quoted 7 lines · Gleb Natapov
Quoted 6 lines
kvm-unit-tests.git has a test for xchg to mmio. Does it still work?
I agree this code has to go, but it needs to be replaced by something. Maybe a .valid flag in struct operand.
Valid will not enough for that.
If we make everything go through operands, any reason why not?
I noticed some instructions need to read ESP for many times (e.g, iret_real), maybe .valid flag is not enough for this case if the stack is in MMIO, yes?
IIUC, I also noticed ESP is not reset back if it is emulated fail (mmio is needed). If the stack located in mmio region, this kind of instruct will be broken, i know no guest will use mmio as stack but SDM does not limit it, is it valid?
Quoted 13 lines · Xiao Guangrong
Quoted 11 lines · Avi Kivity
Quoted 7 lines · Gleb Natapov
Quoted 6 lines
kvm-unit-tests.git has a test for xchg to mmio. Does it still work?
I agree this code has to go, but it needs to be replaced by something. Maybe a .valid flag in struct operand.
Valid will not enough for that.
If we make everything go through operands, any reason why not?
Quoted 7 lines
I noticed some instructions need to read ESP for many times (e.g,
iret_real), maybe .valid flag is not enough for this case if the stack is in MMIO, yes?IIUC, I also noticed ESP is not reset back if it is emulated fail (mmio is needed). If the stack located in mmio region, this kind of instruct will be broken, i know no guest will use mmio as stack but SDM does not limit it, is it valid?
Good point about MMIO stack too.
--
Gleb.
Quoted 8 lines · Xiao Guangrong
Quoted 7 lines · Avi Kivity
Quoted 6 lines · Gleb Natapov
Quoted 6 lines
kvm-unit-tests.git has a test for xchg to mmio. Does it still work?
I agree this code has to go, but it needs to be replaced by something. Maybe a .valid flag in struct operand.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Valid will not enough for that.
If we make everything go through operands, any reason why not?
I noticed some instructions need to read ESP for many times (e.g,
iret_real), maybe .valid flag is not enough for this case if the stack is in MMIO, yes?
Good catch. We either have to fix it or to restrict stack operations to regular memory (->read_std).
Quoted 3 lines
IIUC, I also noticed ESP is not reset back if it is emulated fail (mmio is needed). If the stack located in mmio region, this kind of instruct will be broken, i know no guest will use mmio as stack but SDM does not limit it, is it valid?
Stack in mmio (or task switch in mmio) is architecturally valid. We don't have to support it if no guests do it.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 20 lines · Avi Kivity
Quoted 8 lines · Xiao Guangrong
Quoted 7 lines · Avi Kivity
Quoted 6 lines · Gleb Natapov
Quoted 6 lines
kvm-unit-tests.git has a test for xchg to mmio. Does it still work?
I agree this code has to go, but it needs to be replaced by something. Maybe a .valid flag in struct operand.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Valid will not enough for that.
If we make everything go through operands, any reason why not?
I noticed some instructions need to read ESP for many times (e.g,
iret_real), maybe .valid flag is not enough for this case if the stack is in MMIO, yes?Good catch. We either have to fix it or to restrict stack operations to regular memory (->read_std).
Quoted 7 lines
Quoted 3 lines
IIUC, I also noticed ESP is not reset back if it is emulated fail (mmio is needed). If the stack located in mmio region, this kind of instruct will be broken, i know no guest will use mmio as stack but SDM does not limit it, is it valid?
Stack in mmio (or task switch in mmio) is architecturally valid. We don't have to support it if no guests do it.
But the code is already here, why drop it?
--
Gleb.
Quoted 21 lines · Gleb Natapov
Quoted 20 lines · Avi Kivity
Quoted 8 lines · Xiao Guangrong
Quoted 7 lines · Avi Kivity
Quoted 6 lines · Gleb Natapov
Quoted 6 lines
kvm-unit-tests.git has a test for xchg to mmio. Does it still work?
I agree this code has to go, but it needs to be replaced by something. Maybe a .valid flag in struct operand.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Valid will not enough for that.
If we make everything go through operands, any reason why not?
I noticed some instructions need to read ESP for many times (e.g,
iret_real), maybe .valid flag is not enough for this case if the stack is in MMIO, yes?Good catch. We either have to fix it or to restrict stack operations to regular memory (->read_std).
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
IIUC, I also noticed ESP is not reset back if it is emulated fail (mmio is needed). If the stack located in mmio region, this kind of instruct will be broken, i know no guest will use mmio as stack but SDM does not limit it, is it valid?
Stack in mmio (or task switch in mmio) is architecturally valid. We don't have to support it if no guests do it.
But the code is already here, why drop it?
The read cache is not effective for multiple disjunct reads. The splitting into 8-byte groups is unneeded.
Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
Quoted 26 lines · Avi Kivity
Quoted 25 lines · Gleb Natapov
Quoted 24 lines · Avi Kivity
Quoted 8 lines · Xiao Guangrong
Quoted 7 lines · Avi Kivity
Quoted 6 lines · Gleb Natapov
Quoted 6 lines
kvm-unit-tests.git has a test for xchg to mmio. Does it still work?
I agree this code has to go, but it needs to be replaced by something. Maybe a .valid flag in struct operand.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Valid will not enough for that.
If we make everything go through operands, any reason why not?
I noticed some instructions need to read ESP for many times (e.g,
iret_real), maybe .valid flag is not enough for this case if the stack is in MMIO, yes?Good catch. We either have to fix it or to restrict stack operations to regular memory (->read_std).
Quoted 3 lines
IIUC, I also noticed ESP is not reset back if it is emulated fail (mmio is needed). If the stack located in mmio region, this kind of instruct will be broken, i know no guest will use mmio as stack but SDM does not limit it, is it valid?
Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
Stack in mmio (or task switch in mmio) is architecturally valid. We don't have to support it if no guests do it.
But the code is already here, why drop it?
The read cache is not effective for multiple disjunct reads.
What do you mean?
Quoted 3 lines
The
splitting into 8-byte groups is unneeded.
Agree. Easy to fix.
--
Gleb.
Quoted 5 lines · Gleb Natapov
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
But the code is already here, why drop it?
The read cache is not effective for multiple disjunct reads.
What do you mean?
If an instruction reads from several sources in mmio, then the first read will be flushed from the cache by the second read. So if we need a third read, we'll have to exit for the first again.
Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
Quoted 11 lines · Avi Kivity
Quoted 5 lines · Gleb Natapov
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
But the code is already here, why drop it?
The read cache is not effective for multiple disjunct reads.
What do you mean?
If an instruction reads from several sources in mmio, then the first read will be flushed from the cache by the second read. So if we need a third read, we'll have to exit for the first again.
The cache is flashed only before instruction decode, never during emulation.
--
Gleb.
Quoted 6 lines · Gleb Natapov
Quoted 5 lines · Avi Kivity
Quoted 4 lines · Gleb Natapov
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
But the code is already here, why drop it?
The read cache is not effective for multiple disjunct reads.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
What do you mean?
If an instruction reads from several sources in mmio, then the first read will be flushed from the cache by the second read. So if we need a third read, we'll have to exit for the first again.
The cache is flashed only before instruction decode, never during emulation.
Oh, I misread the code.
Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
Quoted 11 lines · Avi Kivity
Quoted 7 lines · Gleb Natapov
Quoted 6 lines
kvm-unit-tests.git has a test for xchg to mmio. Does it still work?
I agree this code has to go, but it needs to be replaced by something. Maybe a .valid flag in struct operand.
Valid will not enough for that.
If we make everything go through operands, any reason why not?
Ah I missed operand part. Thought about adding valid to x86.c mmio read buffer. What about doing more complex things from MMIO, like task switch?
--
Gleb.