On Wed, Mar 15, 2023 at 02:17:35AM +0000, Anish Moorthy wrote:
> kvm_handle_guest_abort currently just returns 1 if user_mem_abort
> returns 0. Since 1 is the "resume the guest" code, user_mem_abort is
> essentially incapable of triggering a "normal" exit: it can only trigger
> exits by returning a negative value, which indicates an error.
>
> Remove the "if (ret == 0) ret = 1;" statement from
> kvm_handle_guest_abort and refactor user_mem_abort slightly to allow it
> to trigger 'normal' exits by returning 0.
You should append '()' to function names, as it makes it abundantly obvious to
the reader that the symbols you describe are indeed functions.
I find the changelog a bit too mechanical and doesn't capture the nuance.
Generally, in the context of a vCPU exit, a return value of 1 is used
to indicate KVM should return to the guest and 0 is used to complete a
'normal' exit to userspace. user_mem_abort() deviates from this
slightly, using 0 to return to the guest.
Just return 1 from user_mem_abort() to return to the guest and drop
the return code conversion from kvm_handle_guest_abort(). It is now
possible to do a 'normal' exit to userspace from user_mem_abort(),
which will be used in a later change.
> Signed-off-by: Anish Moorthy <amoorthy@xxxxxxxxxx>
> ---
> arch/arm64/kvm/mmu.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 7113587222ffe..735044859eb25 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1190,7 +1190,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> struct kvm_memory_slot *memslot, unsigned long hva,
> unsigned long fault_status)
> {
> - int ret = 0;
> + int ret = 1;
> bool write_fault, writable, force_pte = false;
> bool exec_fault;
> bool device = false;
> @@ -1281,8 +1281,10 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> (logging_active && write_fault)) {
> ret = kvm_mmu_topup_memory_cache(memcache,
> kvm_mmu_cache_min_pages(kvm));
> - if (ret)
> + if (ret < 0)
There's no need to change this condition.
> return ret;
> + else
> + ret = 1;
I'd prefer if you set 'ret' close to where it is actually used, which I
believe is only if mmu_invalidate_retry():
if (mmu_invalidate_retry(kvm, mmu_seq)) {
ret = 1;
goto out_unlock;
}
Otherwise ret gets written to before exiting.
--
Thanks,
Oliver
