From: Zhao Liu <[email protected]>
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.
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
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/[email protected]
[2]: https://lore.kernel.org/lkml/[email protected]/
Suggested-by: Dave Hansen <[email protected]>
Suggested-by: Ira Weiny <[email protected]>
Suggested-by: Fabio M. De Francesco <[email protected]>
Signed-off-by: Zhao Liu <[email protected]>
---
Suggested by credits:
Dave: Referred to his explanation about cache flush and usage of
page_address().
Ira: Referred to his task document, review comments and explanation about
cache flush.
Fabio: Referred to his boiler plate commit message.
---
Changes since v1:
* Add the explanation of global cache flush for sev_clflush_pages() in commit
message.
* Add the explanation why not use page_address() directly.
---
arch/x86/kvm/svm/sev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 28064060413a..12747c7bda4e 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -465,9 +465,9 @@ static void sev_clflush_pages(struct page *pages[], unsigned long npages)
return;
for (i = 0; i < npages; i++) {
- page_virtual = kmap_atomic(pages[i]);
+ page_virtual = kmap_local_page(pages[i]);
clflush_cache_range(page_virtual, PAGE_SIZE);
- kunmap_atomic(page_virtual);
+ kunmap_local(page_virtual);
cond_resched();
}
}
--
2.34.1
On Wed, Sep 28, 2022, Zhao Liu wrote:
> From: Zhao Liu <[email protected]>
>
> 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/[email protected]
> [2]: https://lore.kernel.org/lkml/[email protected]/
>
> Suggested-by: Dave Hansen <[email protected]>
> Suggested-by: Ira Weiny <[email protected]>
> Suggested-by: Fabio M. De Francesco <[email protected]>
> Signed-off-by: Zhao Liu <[email protected]>
> ---
No need to send a v3, the above are all the nittiest of nits.
Reviewed-by: Sean Christopherson <[email protected]>
On Thu, Oct 06, 2022 at 12:14:17AM +0000, Sean Christopherson wrote:
> Date: Thu, 6 Oct 2022 00:14:17 +0000
> From: Sean Christopherson <[email protected]>
> Subject: Re: [PATCH v2] KVM: SVM: Replace kmap_atomic() with
> kmap_local_page()
>
> On Wed, Sep 28, 2022, Zhao Liu wrote:
> > From: Zhao Liu <[email protected]>
> >
> > 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/[email protected]
> > [2]: https://lore.kernel.org/lkml/[email protected]/
> >
> > Suggested-by: Dave Hansen <[email protected]>
> > Suggested-by: Ira Weiny <[email protected]>
> > Suggested-by: Fabio M. De Francesco <[email protected]>
> > Signed-off-by: Zhao Liu <[email protected]>
> > ---
>
> No need to send a v3, the above are all the nittiest of nits.
Thanks Sean! I'll pay more attention to these next time.
Zhao
>
> Reviewed-by: Sean Christopherson <[email protected]>
On Wed, Sep 28, 2022, Zhao Liu wrote:
> From: Zhao Liu <[email protected]>
>
> 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.
>
> 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
> 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/[email protected]
> [2]: https://lore.kernel.org/lkml/[email protected]/
>
> Suggested-by: Dave Hansen <[email protected]>
> Suggested-by: Ira Weiny <[email protected]>
> Suggested-by: Fabio M. De Francesco <[email protected]>
> Signed-off-by: Zhao Liu <[email protected]>
> ---
Merged to kvm/queue, thanks!
https://lore.kernel.org/all/[email protected]