| Message ID | 20220121222626.972495-6-farosas@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | KVM: PPC: MMIO fixes | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
Excerpts from Fabiano Rosas's message of January 22, 2022 8:26 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. > > 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 | 12 ++++++++++++ > 2 files changed, 15 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 214602c58f13..9befb121dddb 100644 > --- a/arch/powerpc/kvm/powerpc.c > +++ b/arch/powerpc/kvm/powerpc.c > @@ -305,10 +305,22 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu) > case EMULATE_FAIL: > { > u32 last_inst; > + ulong store_bit = DSISR_ISSTORE; > + ulong cause = DSISR_BADACCESS; > > +#ifdef CONFIG_BOOKE > + store_bit = ESR_ST; > + cause = 0; > +#endif BookE can not cause a bad page fault in the guest with ESR bits AFAIKS, so it would cause an infinite fault loop here. Maybe stick with the program interrupt for BookE with a comment about that here. And if it could use if (IS_ENABLED()) would be good? Otherwise looks good, it should do the right thing on BookS. Thanks, Nick > kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst); > pr_info_ratelimited("KVM: guest access to device memory using unsupported instruction (PID: %d opcode: %#08x)\n", > current->pid, last_inst); > + > + if (vcpu->mmio_is_write) > + cause |= store_bit; > + > + kvmppc_core_queue_data_storage(vcpu, vcpu->arch.vaddr_accessed, > + cause); > 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 214602c58f13..9befb121dddb 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -305,10 +305,22 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu) case EMULATE_FAIL: { u32 last_inst; + ulong store_bit = DSISR_ISSTORE; + ulong cause = DSISR_BADACCESS; +#ifdef CONFIG_BOOKE + store_bit = ESR_ST; + cause = 0; +#endif kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst); pr_info_ratelimited("KVM: guest access to device memory using unsupported instruction (PID: %d opcode: %#08x)\n", current->pid, last_inst); + + if (vcpu->mmio_is_write) + cause |= store_bit; + + kvmppc_core_queue_data_storage(vcpu, vcpu->arch.vaddr_accessed, + cause); 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. 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 | 12 ++++++++++++ 2 files changed, 15 insertions(+), 7 deletions(-)