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

From: Alexey Kardashevskiy <aik@ozlabs.ru>
To: Nicholas Piggin <npiggin@gmail.com>,
	Fabiano Rosas <farosas@linux.ibm.com>,
	kvm-ppc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v3 5/6] KVM: PPC: mmio: Return to guest after emulation failure
Date: Tue, 11 Jan 2022 10:51:02 +1100	[thread overview]
Message-ID: <bf61f021-15b3-7093-f991-cdcda93d059d@ozlabs.ru> (raw)
In-Reply-To: <1641799578.6dxlxsaaos.astroid@bobo.none>

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.