Re: [PATCH v4 07/16] KVM: Simplify error handling in __gfn_to_pfn_memslot()

[Sean Christopherson <seanjc@google.com>]

On Fri, Jun 02, 2023, Anish Moorthy wrote:
> KVM_HVA_ERR_RO_BAD satisfies kvm_is_error_hva(), so there's no need to
> duplicate the "if (writable)" block. Fix this by bringing all
> kvm_is_error_hva() cases under one conditional.
> 
> Signed-off-by: Anish Moorthy <amoorthy@google.com>
> ---
>  virt/kvm/kvm_main.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index b9d2606f9251..05d6e7e3994d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2711,16 +2711,14 @@ kvm_pfn_t __gfn_to_pfn_memslot(const struct kvm_memory_slot *slot, gfn_t gfn,
>  	if (hva)
>  		*hva = addr;
>  
> -	if (addr == KVM_HVA_ERR_RO_BAD) {
> -		if (writable)
> -			*writable = false;
> -		return KVM_PFN_ERR_RO_FAULT;
> -	}
> -
>  	if (kvm_is_error_hva(addr)) {
>  		if (writable)
>  			*writable = false;
> -		return KVM_PFN_NOSLOT;
> +
> +		if (addr == KVM_HVA_ERR_RO_BAD)
> +			return KVM_PFN_ERR_RO_FAULT;
> +		else
> +			return KVM_PFN_NOSLOT;

Similar to an earlier patch, preferred style for terminal if-statements is

		if (addr == KVM_HVA_ERR_RO_BAD)
			return KVM_PFN_ERR_RO_FAULT;

		return KVM_PFN_NOSLOT;

Again, there are reasons for the style/rule.  In this case, it will yield a smaller
diff (obviously not a huge deal, but helpful), and it makes it more obvious that
the taken path of "if (kvm_is_error_hva(addr))" is itself terminal.

Alternatively, a ternary operator is often used for these types of things, though
in this case I much prefer the above, as I find the below hard to read.

		return addr == KVM_HVA_ERR_RO_BAD ? KVM_PFN_ERR_RO_FAULT :
						    KVM_PFN_NOSLOT;