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;
Message-ID: <fd8fab58-2871-c2d4-f620-e454c7ac653e@redhat.com>
Date:   Wed, 28 Sep 2022 19:04:36 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.1
Content-Language: en-US
To:     Emanuele Giuseppe Esposito <eesposit@redhat.com>,
        kvm@vger.kernel.org
Cc:     Sean Christopherson <seanjc@google.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        David Hildenbrand <david@redhat.com>,
        Maxim Levitsky <mlevitsk@redhat.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org
References: <20220909104506.738478-1-eesposit@redhat.com>
 <20220909104506.738478-5-eesposit@redhat.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [RFC PATCH 4/9] kvm_main.c: split logic in kvm_set_memslots
In-Reply-To: <20220909104506.738478-5-eesposit@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/9/22 12:45, Emanuele Giuseppe Esposito wrote:
> +/*
> + * Takes kvm->slots_arch_lock, and releases it only if
> + * invalid_slot allocation or kvm_prepare_memory_region failed.
> +*/

Much simpler: "kvm->slots_arch_lock is taken on a successful return."

This is a small change in phrasing, but it makes a lot more sense: on 
success you are preparing for the final commit operation, otherwise you 
just want the caller to return your errno value.

[...]

> +/* Must be called with kvm->slots_arch_lock held, but releases it. */
s/but/and/.  Even better, "and releases it before returning".  "But" 
tells the reader that something strange is going on, "and" tells it that 
something simple is going on.

I would also rename the functions along the lines of my review to patch 
3, to highlight that these function prepare/commit a *change* to a memslot.

> +static void kvm_finish_memslot(struct kvm *kvm,
> +			       struct kvm_internal_memory_region_list *batch)
> +{
> +	struct kvm_memory_slot *invalid_slot = batch->invalid;
> +	struct kvm_memory_slot *old = batch->old;
> +	struct kvm_memory_slot *new = batch->new;
> +	enum kvm_mr_change change = batch->change;

lockdep_assert_held(&kvm->slots_arch_lock);

>   
>   	/*
>   	 * For DELETE and MOVE, the working slot is now active as the INVALID
> @@ -1883,6 +1898,18 @@ static int kvm_set_memslot(struct kvm *kvm,
>   	 * responsible for knowing that new->arch may be stale.
>   	 */
>   	kvm_commit_memory_region(kvm, batch);
> +}
> +
> +static int kvm_set_memslot(struct kvm *kvm,
> +			   struct kvm_internal_memory_region_list *batch)
> +{
> +	int r;
> +
> +	r = kvm_prepare_memslot(kvm, batch);
> +	if (r)
> +		return r;
> +
> +	kvm_finish_memslot(kvm, batch);
>   
>   	return 0;

Ok, these are the functions I hinted at in the review of the previous 
patch, so we're not far away.  You should be able to move the 
kvm_set_memslot call to kvm_set_memory_region in patch 3, and then 
replace it with the two calls here directly in kvm_set_memory_region.

Paolo