| Message ID | 20220125215655.1026224-6-farosas@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | KVM: PPC: MMIO fixes | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
Excerpts from Fabiano Rosas's message of January 26, 2022 7:56 am: > MMIO emulation can fail if the guest uses an instruction that we are > not prepared to emulate. Since these instructions can be and most > likely are valid ones, this is (slightly) closer to an access fault > than to an illegal instruction, so deliver a Data Storage interrupt > instead of a Program interrupt. > > BookE ignores bad faults, so it will keep using a Program interrupt > because a DSI would cause a fault loop in the guest. > > Suggested-by: Nicholas Piggin <npiggin@gmail.com> > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> Thanks this looks good to me. (And thanks for updating patch 4/5 with the kvm debug print helper.) Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > --- > arch/powerpc/kvm/emulate_loadstore.c | 10 +++------- > arch/powerpc/kvm/powerpc.c | 22 ++++++++++++++++++++++ > 2 files changed, 25 insertions(+), 7 deletions(-) > > diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c > index 48272a9b9c30..cfc9114b87d0 100644 > --- a/arch/powerpc/kvm/emulate_loadstore.c > +++ b/arch/powerpc/kvm/emulate_loadstore.c > @@ -73,7 +73,6 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu) > { > u32 inst; > enum emulation_result emulated = EMULATE_FAIL; > - int advance = 1; > struct instruction_op op; > > /* this default type might be overwritten by subcategories */ > @@ -98,6 +97,8 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu) > int type = op.type & INSTR_TYPE_MASK; > int size = GETSIZE(op.type); > > + vcpu->mmio_is_write = OP_IS_STORE(type); > + > switch (type) { > case LOAD: { > int instr_byte_swap = op.type & BYTEREV; > @@ -355,15 +356,10 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu) > } > } > > - if (emulated == EMULATE_FAIL) { > - advance = 0; > - kvmppc_core_queue_program(vcpu, 0); > - } > - > trace_kvm_ppc_instr(inst, kvmppc_get_pc(vcpu), emulated); > > /* Advance past emulated instruction. */ > - if (advance) > + if (emulated != EMULATE_FAIL) > kvmppc_set_pc(vcpu, kvmppc_get_pc(vcpu) + 4); > > return emulated; > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c > index acb0d2a4bdb9..82d889db2b6b 100644 > --- a/arch/powerpc/kvm/powerpc.c > +++ b/arch/powerpc/kvm/powerpc.c > @@ -309,6 +309,28 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu) > kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst); > kvm_debug_ratelimited("Guest access to device memory using unsupported instruction (opcode: %#08x)\n", > last_inst); > + > + /* > + * Injecting a Data Storage here is a bit more > + * accurate since the instruction that caused the > + * access could still be a valid one. > + */ > + if (!IS_ENABLED(CONFIG_BOOKE)) { > + ulong dsisr = DSISR_BADACCESS; > + > + if (vcpu->mmio_is_write) > + dsisr |= DSISR_ISSTORE; > + > + kvmppc_core_queue_data_storage(vcpu, vcpu->arch.vaddr_accessed, dsisr); > + } else { > + /* > + * BookE does not send a SIGBUS on a bad > + * fault, so use a Program interrupt instead > + * to avoid a fault loop. > + */ > + kvmppc_core_queue_program(vcpu, 0); > + } > + > r = RESUME_GUEST; > break; > } > -- > 2.34.1 > >
diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c index 48272a9b9c30..cfc9114b87d0 100644 --- a/arch/powerpc/kvm/emulate_loadstore.c +++ b/arch/powerpc/kvm/emulate_loadstore.c @@ -73,7 +73,6 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu) { u32 inst; enum emulation_result emulated = EMULATE_FAIL; - int advance = 1; struct instruction_op op; /* this default type might be overwritten by subcategories */ @@ -98,6 +97,8 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu) int type = op.type & INSTR_TYPE_MASK; int size = GETSIZE(op.type); + vcpu->mmio_is_write = OP_IS_STORE(type); + switch (type) { case LOAD: { int instr_byte_swap = op.type & BYTEREV; @@ -355,15 +356,10 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu) } } - if (emulated == EMULATE_FAIL) { - advance = 0; - kvmppc_core_queue_program(vcpu, 0); - } - trace_kvm_ppc_instr(inst, kvmppc_get_pc(vcpu), emulated); /* Advance past emulated instruction. */ - if (advance) + if (emulated != EMULATE_FAIL) kvmppc_set_pc(vcpu, kvmppc_get_pc(vcpu) + 4); return emulated; diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index acb0d2a4bdb9..82d889db2b6b 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -309,6 +309,28 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu) kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst); kvm_debug_ratelimited("Guest access to device memory using unsupported instruction (opcode: %#08x)\n", last_inst); + + /* + * Injecting a Data Storage here is a bit more + * accurate since the instruction that caused the + * access could still be a valid one. + */ + if (!IS_ENABLED(CONFIG_BOOKE)) { + ulong dsisr = DSISR_BADACCESS; + + if (vcpu->mmio_is_write) + dsisr |= DSISR_ISSTORE; + + kvmppc_core_queue_data_storage(vcpu, vcpu->arch.vaddr_accessed, dsisr); + } else { + /* + * BookE does not send a SIGBUS on a bad + * fault, so use a Program interrupt instead + * to avoid a fault loop. + */ + kvmppc_core_queue_program(vcpu, 0); + } + r = RESUME_GUEST; break; }
MMIO emulation can fail if the guest uses an instruction that we are not prepared to emulate. Since these instructions can be and most likely are valid ones, this is (slightly) closer to an access fault than to an illegal instruction, so deliver a Data Storage interrupt instead of a Program interrupt. BookE ignores bad faults, so it will keep using a Program interrupt because a DSI would cause a fault loop in the guest. Suggested-by: Nicholas Piggin <npiggin@gmail.com> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> --- arch/powerpc/kvm/emulate_loadstore.c | 10 +++------- arch/powerpc/kvm/powerpc.c | 22 ++++++++++++++++++++++ 2 files changed, 25 insertions(+), 7 deletions(-)