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

[Nicholas Piggin <npiggin@gmail.com>]

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.
>