| Message ID | 20220107210012.4091153-6-farosas@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: PPC: MMIO fixes | expand |
Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am: > If MMIO emulation fails we don't want to crash the whole guest by > returning to userspace. > > The original commit bbf45ba57eae ("KVM: ppc: PowerPC 440 KVM > implementation") added a todo: > > /* XXX Deliver Program interrupt to guest. */ > > and later the commit d69614a295ae ("KVM: PPC: Separate loadstore > emulation from priv emulation") added the Program interrupt injection > but in another file, so I'm assuming it was missed that this block > needed to be altered. > > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> > Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > arch/powerpc/kvm/powerpc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c > index 6daeea4a7de1..56b0faab7a5f 100644 > --- a/arch/powerpc/kvm/powerpc.c > +++ b/arch/powerpc/kvm/powerpc.c > @@ -309,7 +309,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu) > kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst); > kvmppc_core_queue_program(vcpu, 0); > pr_info("%s: emulation failed (%08x)\n", __func__, last_inst); > - r = RESUME_HOST; > + r = RESUME_GUEST; So at this point can the pr_info just go away? I wonder if this shouldn't be a DSI rather than a program check. DSI with DSISR[37] looks a bit more expected. Not that Linux probably does much with it but at least it would give a SIGBUS rather than SIGILL. Thanks, Nick
On 1/10/22 18:36, Nicholas Piggin wrote: > Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am: >> If MMIO emulation fails we don't want to crash the whole guest by >> returning to userspace. >> >> The original commit bbf45ba57eae ("KVM: ppc: PowerPC 440 KVM >> implementation") added a todo: >> >> /* XXX Deliver Program interrupt to guest. */ >> >> and later the commit d69614a295ae ("KVM: PPC: Separate loadstore >> emulation from priv emulation") added the Program interrupt injection >> but in another file, so I'm assuming it was missed that this block >> needed to be altered. >> >> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> >> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> arch/powerpc/kvm/powerpc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c >> index 6daeea4a7de1..56b0faab7a5f 100644 >> --- a/arch/powerpc/kvm/powerpc.c >> +++ b/arch/powerpc/kvm/powerpc.c >> @@ -309,7 +309,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu) >> kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst); >> kvmppc_core_queue_program(vcpu, 0); >> pr_info("%s: emulation failed (%08x)\n", __func__, last_inst); >> - r = RESUME_HOST; >> + r = RESUME_GUEST; > > So at this point can the pr_info just go away? > > I wonder if this shouldn't be a DSI rather than a program check. > DSI with DSISR[37] looks a bit more expected. Not that Linux > probably does much with it but at least it would give a SIGBUS > rather than SIGILL. It does not like it is more expected to me, it is not about wrong memory attributes, it is the instruction itself which cannot execute. DSISR[37]: Set to 1 if the access is due to a lq, stq, lwat, ldat, lbarx, lharx, lwarx, ldarx, lqarx, stwat, stdat, stbcx., sthcx., stwcx., stdcx., or stqcx. instruction that addresses storage that is Write Through Required or Caching Inhibited; or if the access is due to a copy or paste. instruction that addresses storage that is Caching Inhibited; or if the access is due to a lwat, ldat, stwat, or stdat instruction that addresses storage that is Guarded; otherwise set to 0.
Excerpts from Alexey Kardashevskiy's message of January 11, 2022 9:51 am: > > > On 1/10/22 18:36, Nicholas Piggin wrote: >> Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am: >>> If MMIO emulation fails we don't want to crash the whole guest by >>> returning to userspace. >>> >>> The original commit bbf45ba57eae ("KVM: ppc: PowerPC 440 KVM >>> implementation") added a todo: >>> >>> /* XXX Deliver Program interrupt to guest. */ >>> >>> and later the commit d69614a295ae ("KVM: PPC: Separate loadstore >>> emulation from priv emulation") added the Program interrupt injection >>> but in another file, so I'm assuming it was missed that this block >>> needed to be altered. >>> >>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> >>> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >>> arch/powerpc/kvm/powerpc.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c >>> index 6daeea4a7de1..56b0faab7a5f 100644 >>> --- a/arch/powerpc/kvm/powerpc.c >>> +++ b/arch/powerpc/kvm/powerpc.c >>> @@ -309,7 +309,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu) >>> kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst); >>> kvmppc_core_queue_program(vcpu, 0); >>> pr_info("%s: emulation failed (%08x)\n", __func__, last_inst); >>> - r = RESUME_HOST; >>> + r = RESUME_GUEST; >> >> So at this point can the pr_info just go away? >> >> I wonder if this shouldn't be a DSI rather than a program check. >> DSI with DSISR[37] looks a bit more expected. Not that Linux >> probably does much with it but at least it would give a SIGBUS >> rather than SIGILL. > > It does not like it is more expected to me, it is not about wrong memory > attributes, it is the instruction itself which cannot execute. It's not an illegal instruction though, it can't execute because of the nature of the data / address it is operating on. That says d-side to me. DSISR[37] isn't perfect but if you squint it's not terrible. It's about certain instructions that have restrictions operating on other than normal cacheable mappings. Thanks, Nick > > DSISR[37]: > Set to 1 if the access is due to a lq, stq, lwat, ldat, lbarx, lharx, > lwarx, ldarx, lqarx, stwat, > stdat, stbcx., sthcx., stwcx., stdcx., or stqcx. instruction that > addresses storage that is Write > Through Required or Caching Inhibited; or if the access is due to a copy > or paste. instruction > that addresses storage that is Caching Inhibited; or if the access is > due to a lwat, ldat, stwat, or > stdat instruction that addresses storage that is Guarded; otherwise set > to 0. >
Nicholas Piggin <npiggin@gmail.com> writes: > Excerpts from Alexey Kardashevskiy's message of January 11, 2022 9:51 am: >> >> >> On 1/10/22 18:36, Nicholas Piggin wrote: >>> Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am: >>>> If MMIO emulation fails we don't want to crash the whole guest by >>>> returning to userspace. >>>> >>>> The original commit bbf45ba57eae ("KVM: ppc: PowerPC 440 KVM >>>> implementation") added a todo: >>>> >>>> /* XXX Deliver Program interrupt to guest. */ >>>> >>>> and later the commit d69614a295ae ("KVM: PPC: Separate loadstore >>>> emulation from priv emulation") added the Program interrupt injection >>>> but in another file, so I'm assuming it was missed that this block >>>> needed to be altered. >>>> >>>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> >>>> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>>> --- >>>> arch/powerpc/kvm/powerpc.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c >>>> index 6daeea4a7de1..56b0faab7a5f 100644 >>>> --- a/arch/powerpc/kvm/powerpc.c >>>> +++ b/arch/powerpc/kvm/powerpc.c >>>> @@ -309,7 +309,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu) >>>> kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst); >>>> kvmppc_core_queue_program(vcpu, 0); >>>> pr_info("%s: emulation failed (%08x)\n", __func__, last_inst); >>>> - r = RESUME_HOST; >>>> + r = RESUME_GUEST; >>> >>> So at this point can the pr_info just go away? >>> >>> I wonder if this shouldn't be a DSI rather than a program check. >>> DSI with DSISR[37] looks a bit more expected. Not that Linux >>> probably does much with it but at least it would give a SIGBUS >>> rather than SIGILL. >> >> It does not like it is more expected to me, it is not about wrong memory >> attributes, it is the instruction itself which cannot execute. > > It's not an illegal instruction though, it can't execute because of the > nature of the data / address it is operating on. That says d-side to me. > > DSISR[37] isn't perfect but if you squint it's not terrible. It's about > certain instructions that have restrictions operating on other than > normal cacheable mappings. I think I agree with Nick on this one. At least the DSISR gives _some_ information while the Program is maybe too generic. I would probably be staring at the opcode wondering what is wrong with it.
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index 6daeea4a7de1..56b0faab7a5f 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -309,7 +309,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu) kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst); kvmppc_core_queue_program(vcpu, 0); pr_info("%s: emulation failed (%08x)\n", __func__, last_inst); - r = RESUME_HOST; + r = RESUME_GUEST; break; } default:
If MMIO emulation fails we don't want to crash the whole guest by returning to userspace. The original commit bbf45ba57eae ("KVM: ppc: PowerPC 440 KVM implementation") added a todo: /* XXX Deliver Program interrupt to guest. */ and later the commit d69614a295ae ("KVM: PPC: Separate loadstore emulation from priv emulation") added the Program interrupt injection but in another file, so I'm assuming it was missed that this block needed to be altered. Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- arch/powerpc/kvm/powerpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)