Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 18 Jul 2022 15:10:38 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Maxim Levitsky <mlevitsk@redhat.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/4] KVM: x86: Drop unnecessary goto+label in
 kvm_arch_init()
Message-ID: <YtV37rQaExsfITH3@google.com>
References: <20220715230016.3762909-1-seanjc@google.com>
 <20220715230016.3762909-3-seanjc@google.com>
 <2c8fd8e1179fc63084ec451496df5b68caae2756.camel@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2c8fd8e1179fc63084ec451496df5b68caae2756.camel@redhat.com>
Precedence: bulk

On Mon, Jul 18, 2022, Maxim Levitsky wrote:
> I honestly don't see much value in this change, but I don't mind it either.

Yeah, this particular instance isn't a significant improvement, but I really dislike
the pattern (if the target is a raw return) and want to discourage its use in KVM.

For longer functions, having to scroll down to see that the target is nothing more
than a raw return is quite annoying.  And for more complex usage, the pattern sometimes
leads to setting the return value well ahead of the "goto", which combined with the
scrolling is very unfriendly to readers.

E.g. prior to commit 71a4c30bf0d3 ("KVM: Refactor error handling for setting memory
region"), the memslot code input validation was written as so.  The "r = 0" in the
"Nothing to change" path was especially gross.

        r = -EINVAL;
        as_id = mem->slot >> 16;
        id = (u16)mem->slot;

        /* General sanity checks */
        if (mem->memory_size & (PAGE_SIZE - 1))
                goto out;
        if (mem->guest_phys_addr & (PAGE_SIZE - 1))
                goto out;
        /* We can read the guest memory with __xxx_user() later on. */
        if ((id < KVM_USER_MEM_SLOTS) &&
            ((mem->userspace_addr & (PAGE_SIZE - 1)) ||
             !access_ok((void __user *)(unsigned long)mem->userspace_addr,
                        mem->memory_size)))
                goto out;
        if (as_id >= KVM_ADDRESS_SPACE_NUM || id >= KVM_MEM_SLOTS_NUM)
                goto out;
        if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr)
                goto out;

        slot = id_to_memslot(__kvm_memslots(kvm, as_id), id);
        base_gfn = mem->guest_phys_addr >> PAGE_SHIFT;
        npages = mem->memory_size >> PAGE_SHIFT;

        if (npages > KVM_MEM_MAX_NR_PAGES)
                goto out;

        new = old = *slot;

        new.id = id;
        new.base_gfn = base_gfn;
        new.npages = npages;
        new.flags = mem->flags;
        new.userspace_addr = mem->userspace_addr;

        if (npages) {
                if (!old.npages)
                        change = KVM_MR_CREATE;
                else { /* Modify an existing slot. */
                        if ((new.userspace_addr != old.userspace_addr) ||
                            (npages != old.npages) ||
                            ((new.flags ^ old.flags) & KVM_MEM_READONLY))
                                goto out;

                        if (base_gfn != old.base_gfn)
                                change = KVM_MR_MOVE;
                        else if (new.flags != old.flags)
                                change = KVM_MR_FLAGS_ONLY;
                        else { /* Nothing to change. */
                                r = 0;
                                goto out;
                        }
                }
        } else {
                if (!old.npages)
                        goto out;

                change = KVM_MR_DELETE;
                new.base_gfn = 0;
                new.flags = 0;
        }