linux-kernel - Re: [PATCH v2] KVM: SVM: Replace kmap_atomic() with kmap_local

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 6 Oct 2022 00:14:17 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Zhao Liu <zhao1.liu@...ux.intel.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H . Peter Anvin" <hpa@...or.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Ira Weiny <ira.weiny@...el.com>,
        "Fabio M . De Francesco" <fmdefrancesco@...il.com>,
        Zhenyu Wang <zhenyu.z.wang@...el.com>,
        Zhao Liu <zhao1.liu@...el.com>,
        Dave Hansen <dave.hansen@...el.com>
Subject: Re: [PATCH v2] KVM: SVM: Replace kmap_atomic() with kmap_local_page()

On Wed, Sep 28, 2022, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@...el.com>
> 
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1].
> 
> The main difference between kmap_atomic() and kmap_local_page() is the
> latter allows pagefaults and preemption.

Uber nit, I would phrase this as saying that local mappings don't disable
page faults and preemption, which is slightly different than stating that they
allow pagefaults/preemption.  E.g. if preemption is already disabled.

> There're 2 reasons we can use kmap_local_page() here:
> 1. SEV is 64-bit only and kmap_locla_page() doesn't disable migration in

Nit, s/kmap_locla_page/kmap_local_page

For future reference, even better would be to use human language after "introducing"
the functions, e.g.

  The main difference between atomic and local mappings is that local
  mappings don't disable page faults or preemption.

Obviously that doesn't magically prevent typos, but it does make the changelog
easier to read (IMO).

> this case, but here the function clflush_cache_range() uses CLFLUSHOPT
> instruction to flush, and on x86 CLFLUSHOPT is not CPU-local and flushes
> the page out of the entire cache hierarchy on all CPUs (APM volume 3,
> chapter 3, CLFLUSHOPT). So there's no need to disable preemption to ensure
> CPU-local.
> 2. clflush_cache_range() doesn't need to disable pagefault and the mapping
> is still valid even if sleeps. This is also true for sched out/in when
> preempted.
> 
> In addition, though kmap_local_page() is a thin wrapper around
> page_address() on 64-bit, kmap_local_page() should still be used here in
> preference to page_address() since page_address() isn't suitable to be used
> in a generic function (like sev_clflush_pages()) where the page passed in
> is not easy to determine the source of allocation. Keeping the kmap* API in
> place means it can be used for things other than highmem mappings[2].
> 
> Therefore, sev_clflush_pages() is a function that should use
> kmap_local_page() in place of kmap_atomic().
> 
> Convert the calls of kmap_atomic() / kunmap_atomic() to kmap_local_page() /
> kunmap_local().
> 
> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
> [2]: https://lore.kernel.org/lkml/5d667258-b58b-3d28-3609-e7914c99b31b@intel.com/
> 
> Suggested-by: Dave Hansen <dave.hansen@...el.com>
> Suggested-by: Ira Weiny <ira.weiny@...el.com>
> Suggested-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> Signed-off-by: Zhao Liu <zhao1.liu@...el.com>
> ---

No need to send a v3, the above are all the nittiest of nits.

Reviewed-by: Sean Christopherson <seanjc@...gle.com>