Re: [PATCH v3 5/6] KVM: PPC: mmio: Return to guest after emulation failure

Nicholas Piggin <[email protected]> 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 <[email protected]>
>>>> Reviewed-by: Alexey Kardashevskiy <[email protected]>
>>>> ---
>>>>   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.