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: <e08c8c46-2aa8-3cb4-dce6-61d64c6835cb@intel.com>
Date:   Fri, 2 Sep 2022 08:09:29 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] KVM: SVM: Replace kmap_atomic() with kmap_local_page()
Content-Language: en-US
To:     Zhao Liu <zhao1.liu@linux.intel.com>,
        Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H . Peter Anvin" <hpa@zytor.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     Ira Weiny <ira.weiny@intel.com>,
        "Fabio M . De Francesco" <fmdefrancesco@gmail.com>,
        Zhenyu Wang <zhenyu.z.wang@intel.com>,
        Zhao Liu <zhao1.liu@intel.com>
References: <20220902090811.2430228-1-zhao1.liu@linux.intel.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <20220902090811.2430228-1-zhao1.liu@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/2/22 02:08, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1].
> 
> In arch/x86/kvm/svm/sev.c, the function sev_clflush_pages() doesn't
> need to disable pagefaults and preemption in kmap_atomic(). It can
> simply use kmap_local_page() / kunmap_local() that can instead do the
> mapping / unmapping regardless of the context.
> 
> With kmap_local_page(), the mapping is per thread, CPU local and not
> globally visible. Therefore, sev_clflush_pages() is a function where
> the use of kmap_local_page() in place of kmap_atomic() is correctly
> suited.

This changelog is a little on the weak side.  You could literally take
any arbitrary call-site and file for kmap_atomic() and slap that
changelog on it.  For instance:

	In drivers/target/target_core_sbc.c, the function
	sbc_dif_copy_prot() doesn't need to disable pagefaults and
	preemption in kmap_atomic(). It can simply use kmap_local_page()
	/ kunmap_local() that can instead do the mapping / unmapping
	regardless of the context.

	With kmap_local_page(), the mapping is per thread, CPU local and
	not globally visible. Therefore, sbc_dif_copy_prot() is a
	function where the use of kmap_local_page() in place of
	kmap_atomic() is correctly suited.

That's all valid English and there's nothing incorrect in it.  But, it
doesn't indicate that any actual analysis was performed.  It's utterly
generic.  It could literally have been generated by a pretty trivial script.

It would be great to add at least a small, call-site-specific human
touch to these changelogs.

In this case, saying something about how global the cache flush is would
be a great addition.