Re: [WIP Patch v2 11/14] KVM: arm64: Allow user_mem_abort to return 0 to signal a 'normal' exit

[Oliver Upton <oliver.upton@linux.dev>]

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@google.com>
> ---
>  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