This series resurrects Christoffer Dall's series[1] to provide a common
MMU memory cache implementation that can be shared by x86, arm64 and MIPS.
It also picks up a suggested change from Ben Gardon[2] to clear shadow
page tables during initial allocation so as to avoid clearing entire
pages while holding mmu_lock.
The front half of the patches do house cleaning on x86's memory cache
implementation in preparation for moving it to common code, along with a
fair bit of cleanup on the usage. The middle chunk moves the patches to
common KVM, and the last two chunks convert arm64 and MIPS to the common
implementation.
Cleanup aside, the notable difference from Christoffer and Ben's proposed
patches is to make __GFP_ZERO optional, e.g. to allow x86 to skip zeroing
for its gfns array and to provide line of sight for my
cannot-yet-be-discussed-in-detail use case for non-zero initialized shadow
page tables[3].
Tested on x86 only, no testing whatsoever on arm64 or MIPS.
[1] https://lkml.kernel.org/r/[email protected]
[2] https://lkml.kernel.org/r/[email protected]
[3] https://lkml.kernel.org/r/[email protected]
Sean Christopherson (21):
KVM: x86/mmu: Track the associated kmem_cache in the MMU caches
KVM: x86/mmu: Consolidate "page" variant of memory cache helpers
KVM: x86/mmu: Use consistent "mc" name for kvm_mmu_memory_cache locals
KVM: x86/mmu: Remove superfluous gotos from mmu_topup_memory_caches()
KVM: x86/mmu: Try to avoid crashing KVM if a MMU memory cache is empty
KVM: x86/mmu: Move fast_page_fault() call above
mmu_topup_memory_caches()
KVM: x86/mmu: Topup memory caches after walking GVA->GPA
KVM: x86/mmu: Clean up the gorilla math in mmu_topup_memory_caches()
KVM: x86/mmu: Separate the memory caches for shadow pages and gfn
arrays
KVM: x86/mmu: Make __GFP_ZERO a property of the memory cache
KVM: x86/mmu: Zero allocate shadow pages (outside of mmu_lock)
KVM: x86/mmu: Skip filling the gfn cache for guaranteed direct MMU
topups
KVM: x86/mmu: Prepend "kvm_" to memory cache helpers that will be
global
KVM: Move x86's version of struct kvm_mmu_memory_cache to common code
KVM: Move x86's MMU memory cache helpers to common KVM code
KVM: arm64: Drop @max param from mmu_topup_memory_cache()
KVM: arm64: Use common code's approach for __GFP_ZERO with memory
caches
KVM: arm64: Use common KVM implementation of MMU memory caches
KVM: MIPS: Drop @max param from mmu_topup_memory_cache()
KVM: MIPS: Account pages used for GPA page tables
KVM: MIPS: Use common KVM implementation of MMU memory caches
arch/arm64/include/asm/kvm_host.h | 11 ---
arch/arm64/include/asm/kvm_types.h | 8 ++
arch/arm64/kvm/arm.c | 2 +
arch/arm64/kvm/mmu.c | 54 +++--------
arch/mips/include/asm/kvm_host.h | 11 ---
arch/mips/include/asm/kvm_types.h | 7 ++
arch/mips/kvm/mmu.c | 44 ++-------
arch/powerpc/include/asm/kvm_types.h | 5 ++
arch/s390/include/asm/kvm_types.h | 5 ++
arch/x86/include/asm/kvm_host.h | 14 +--
arch/x86/include/asm/kvm_types.h | 7 ++
arch/x86/kvm/mmu/mmu.c | 129 +++++++++------------------
arch/x86/kvm/mmu/paging_tmpl.h | 10 +--
include/linux/kvm_host.h | 7 ++
include/linux/kvm_types.h | 19 ++++
virt/kvm/kvm_main.c | 55 ++++++++++++
16 files changed, 178 insertions(+), 210 deletions(-)
create mode 100644 arch/arm64/include/asm/kvm_types.h
create mode 100644 arch/mips/include/asm/kvm_types.h
create mode 100644 arch/powerpc/include/asm/kvm_types.h
create mode 100644 arch/s390/include/asm/kvm_types.h
create mode 100644 arch/x86/include/asm/kvm_types.h
--
2.26.0
Add a "gfp_zero" member to arm64's 'struct kvm_mmu_memory_cache' to make
the struct and its usage compatible with the common 'struct
kvm_mmu_memory_cache' in linux/kvm_host.h. This will minimize code
churn when arm64 moves to the common implementation in a future patch, at
the cost of temporarily having somewhat silly code.
No functional change intended.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/arm64/include/asm/kvm_host.h | 1 +
arch/arm64/kvm/arm.c | 2 ++
arch/arm64/kvm/mmu.c | 5 +++--
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index abbdf9703e20..2385dede96e0 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -105,6 +105,7 @@ struct kvm_arch {
*/
struct kvm_mmu_memory_cache {
int nobjs;
+ gfp_t gfp_zero;
void *objects[KVM_NR_MEM_OBJS];
};
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 45276ed50dd6..4c98c6b4d850 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -270,6 +270,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
vcpu->arch.target = -1;
bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
+ vcpu->arch.mmu_page_cache.gfp_zero = __GFP_ZERO;
+
/* Set up the timer */
kvm_timer_vcpu_init(vcpu);
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 9398b66f8a87..688213ef34f0 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -131,7 +131,8 @@ static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache, int min)
if (cache->nobjs >= min)
return 0;
while (cache->nobjs < ARRAY_SIZE(cache->objects)) {
- page = (void *)__get_free_page(GFP_PGTABLE_USER);
+ page = (void *)__get_free_page(GFP_KERNEL_ACCOUNT |
+ cache->gfp_zero);
if (!page)
return -ENOMEM;
cache->objects[cache->nobjs++] = page;
@@ -1342,7 +1343,7 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
phys_addr_t addr, end;
int ret = 0;
unsigned long pfn;
- struct kvm_mmu_memory_cache cache = { 0, };
+ struct kvm_mmu_memory_cache cache = { 0, __GFP_ZERO, };
end = (guest_ipa + size + PAGE_SIZE - 1) & PAGE_MASK;
pfn = __phys_to_pfn(pa);
--
2.26.0
Set __GFP_ZERO for the shadow page memory cache and drop the explicit
clear_page() from kvm_mmu_get_page(). This moves the cost of zeroing a
page to the allocation time of the physical page, i.e. when topping up
the memory caches, and thus avoids having to zero out an entire page
while holding mmu_lock.
Cc: Peter Feiner <[email protected]>
Cc: Peter Shier <[email protected]>
Cc: Junaid Shahid <[email protected]>
Cc: Jim Mattson <[email protected]>
Suggested-by: Ben Gardon <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/mmu/mmu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 6b0ec9060786..a8f8eebf67df 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2545,7 +2545,6 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
if (level > PG_LEVEL_4K && need_sync)
flush |= kvm_sync_pages(vcpu, gfn, &invalid_list);
}
- clear_page(sp->spt);
trace_kvm_mmu_get_page(sp, true);
kvm_mmu_flush_or_zap(vcpu, &invalid_list, false, flush);
@@ -5687,6 +5686,8 @@ int kvm_mmu_create(struct kvm_vcpu *vcpu)
vcpu->arch.mmu_page_header_cache.kmem_cache = mmu_page_header_cache;
vcpu->arch.mmu_page_header_cache.gfp_zero = __GFP_ZERO;
+ vcpu->arch.mmu_shadow_page_cache.gfp_zero = __GFP_ZERO;
+
vcpu->arch.mmu = &vcpu->arch.root_mmu;
vcpu->arch.walk_mmu = &vcpu->arch.root_mmu;
--
2.26.0
Replace the @max param in mmu_topup_memory_cache() and instead use
ARRAY_SIZE() to terminate the loop to fill the cache. This removes a
BUG_ON() and sets the stage for moving arm64 to the common memory cache
implementation.
No functional change intended.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/arm64/kvm/mmu.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index a1f6bc70c4e4..9398b66f8a87 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -124,15 +124,13 @@ static void stage2_dissolve_pud(struct kvm *kvm, phys_addr_t addr, pud_t *pudp)
put_page(virt_to_page(pudp));
}
-static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
- int min, int max)
+static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache, int min)
{
void *page;
- BUG_ON(max > KVM_NR_MEM_OBJS);
if (cache->nobjs >= min)
return 0;
- while (cache->nobjs < max) {
+ while (cache->nobjs < ARRAY_SIZE(cache->objects)) {
page = (void *)__get_free_page(GFP_PGTABLE_USER);
if (!page)
return -ENOMEM;
@@ -1356,8 +1354,7 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
pte = kvm_s2pte_mkwrite(pte);
ret = mmu_topup_memory_cache(&cache,
- kvm_mmu_cache_min_pages(kvm),
- KVM_NR_MEM_OBJS);
+ kvm_mmu_cache_min_pages(kvm));
if (ret)
goto out;
spin_lock(&kvm->mmu_lock);
@@ -1737,8 +1734,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
up_read(¤t->mm->mmap_sem);
/* We need minimum second+third level pages */
- ret = mmu_topup_memory_cache(memcache, kvm_mmu_cache_min_pages(kvm),
- KVM_NR_MEM_OBJS);
+ ret = mmu_topup_memory_cache(memcache, kvm_mmu_cache_min_pages(kvm));
if (ret)
return ret;
--
2.26.0
On Fri, Jun 5, 2020 at 2:39 PM Sean Christopherson
<[email protected]> wrote:
>
> Set __GFP_ZERO for the shadow page memory cache and drop the explicit
> clear_page() from kvm_mmu_get_page(). This moves the cost of zeroing a
> page to the allocation time of the physical page, i.e. when topping up
> the memory caches, and thus avoids having to zero out an entire page
> while holding mmu_lock.
>
> Cc: Peter Feiner <[email protected]>
> Cc: Peter Shier <[email protected]>
> Cc: Junaid Shahid <[email protected]>
> Cc: Jim Mattson <[email protected]>
> Suggested-by: Ben Gardon <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
Reviewed-by: Ben Gardon <[email protected]>
> ---
> arch/x86/kvm/mmu/mmu.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 6b0ec9060786..a8f8eebf67df 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -2545,7 +2545,6 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> if (level > PG_LEVEL_4K && need_sync)
> flush |= kvm_sync_pages(vcpu, gfn, &invalid_list);
> }
> - clear_page(sp->spt);
> trace_kvm_mmu_get_page(sp, true);
>
> kvm_mmu_flush_or_zap(vcpu, &invalid_list, false, flush);
> @@ -5687,6 +5686,8 @@ int kvm_mmu_create(struct kvm_vcpu *vcpu)
> vcpu->arch.mmu_page_header_cache.kmem_cache = mmu_page_header_cache;
> vcpu->arch.mmu_page_header_cache.gfp_zero = __GFP_ZERO;
>
> + vcpu->arch.mmu_shadow_page_cache.gfp_zero = __GFP_ZERO;
> +
> vcpu->arch.mmu = &vcpu->arch.root_mmu;
> vcpu->arch.walk_mmu = &vcpu->arch.root_mmu;
>
> --
> 2.26.0
>
On Fri, Jun 5, 2020 at 2:39 PM Sean Christopherson
<[email protected]> wrote:
>
> Replace the @max param in mmu_topup_memory_cache() and instead use
> ARRAY_SIZE() to terminate the loop to fill the cache. This removes a
> BUG_ON() and sets the stage for moving arm64 to the common memory cache
> implementation.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/arm64/kvm/mmu.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index a1f6bc70c4e4..9398b66f8a87 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -124,15 +124,13 @@ static void stage2_dissolve_pud(struct kvm *kvm, phys_addr_t addr, pud_t *pudp)
> put_page(virt_to_page(pudp));
> }
>
> -static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
> - int min, int max)
> +static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache, int min)
> {
> void *page;
>
> - BUG_ON(max > KVM_NR_MEM_OBJS);
KVM_NR_MEM_OBJS should be undefined as of patch 14 in this series. I'd
recommend changing this to use the new constant you defined in that
patch.
> if (cache->nobjs >= min)
> return 0;
> - while (cache->nobjs < max) {
> + while (cache->nobjs < ARRAY_SIZE(cache->objects)) {
> page = (void *)__get_free_page(GFP_PGTABLE_USER);
> if (!page)
> return -ENOMEM;
> @@ -1356,8 +1354,7 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> pte = kvm_s2pte_mkwrite(pte);
>
> ret = mmu_topup_memory_cache(&cache,
> - kvm_mmu_cache_min_pages(kvm),
> - KVM_NR_MEM_OBJS);
See above, KVM_NR_MEM_OBJS is undefined as of patch 14.
> + kvm_mmu_cache_min_pages(kvm));
> if (ret)
> goto out;
> spin_lock(&kvm->mmu_lock);
> @@ -1737,8 +1734,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> up_read(¤t->mm->mmap_sem);
>
> /* We need minimum second+third level pages */
> - ret = mmu_topup_memory_cache(memcache, kvm_mmu_cache_min_pages(kvm),
> - KVM_NR_MEM_OBJS);
See above, KVM_NR_MEM_OBJS is undefined as of patch 14.
> + ret = mmu_topup_memory_cache(memcache, kvm_mmu_cache_min_pages(kvm));
> if (ret)
> return ret;
>
> --
> 2.26.0
>
Hi Sean,
On 2020-06-05 22:38, Sean Christopherson wrote:
> Add a "gfp_zero" member to arm64's 'struct kvm_mmu_memory_cache' to
> make
> the struct and its usage compatible with the common 'struct
> kvm_mmu_memory_cache' in linux/kvm_host.h. This will minimize code
> churn when arm64 moves to the common implementation in a future patch,
> at
> the cost of temporarily having somewhat silly code.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/arm64/include/asm/kvm_host.h | 1 +
> arch/arm64/kvm/arm.c | 2 ++
> arch/arm64/kvm/mmu.c | 5 +++--
> 3 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index abbdf9703e20..2385dede96e0 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -105,6 +105,7 @@ struct kvm_arch {
> */
> struct kvm_mmu_memory_cache {
> int nobjs;
> + gfp_t gfp_zero;
> void *objects[KVM_NR_MEM_OBJS];
> };
>
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 45276ed50dd6..4c98c6b4d850 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -270,6 +270,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> vcpu->arch.target = -1;
> bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
>
> + vcpu->arch.mmu_page_cache.gfp_zero = __GFP_ZERO;
> +
> /* Set up the timer */
> kvm_timer_vcpu_init(vcpu);
>
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 9398b66f8a87..688213ef34f0 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -131,7 +131,8 @@ static int mmu_topup_memory_cache(struct
> kvm_mmu_memory_cache *cache, int min)
> if (cache->nobjs >= min)
> return 0;
> while (cache->nobjs < ARRAY_SIZE(cache->objects)) {
> - page = (void *)__get_free_page(GFP_PGTABLE_USER);
> + page = (void *)__get_free_page(GFP_KERNEL_ACCOUNT |
This is definitely a change in the way we account for guest
page tables allocation, although I find it bizarre that not
all architectures account for it the same way.
It seems logical to me that nested page tables would be accounted
against userspace, but I'm willing to be educated on the matter.
Another possibility is that depending on the context, some allocations
should be accounted on either the kernel or userspace (NV on arm64
could definitely do something like that). If that was the case,
maybe moving most of the GFP_* flags into the per-cache flags,
and have the renaming that Ben suggested earlier.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Hi Sean,
On 2020-06-05 22:38, Sean Christopherson wrote:
> This series resurrects Christoffer Dall's series[1] to provide a common
> MMU memory cache implementation that can be shared by x86, arm64 and
> MIPS.
>
> It also picks up a suggested change from Ben Gardon[2] to clear shadow
> page tables during initial allocation so as to avoid clearing entire
> pages while holding mmu_lock.
>
> The front half of the patches do house cleaning on x86's memory cache
> implementation in preparation for moving it to common code, along with
> a
> fair bit of cleanup on the usage. The middle chunk moves the patches
> to
> common KVM, and the last two chunks convert arm64 and MIPS to the
> common
> implementation.
>
> Cleanup aside, the notable difference from Christoffer and Ben's
> proposed
> patches is to make __GFP_ZERO optional, e.g. to allow x86 to skip
> zeroing
> for its gfns array and to provide line of sight for my
> cannot-yet-be-discussed-in-detail use case for non-zero initialized
> shadow
> page tables[3].
>
> Tested on x86 only, no testing whatsoever on arm64 or MIPS.
I've given it a go on a small bunch of arm64 boxes, and nothing caught
fire!
As Ben noticed, the series isn't bisectable (easily fixed) and there is
some
nagging conflicts with the current state of mainline.
Overall, a very welcome cleanup. The only point of contention is the
change
in allocation accounting on arm64, but there is an easy fix for that.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On Thu, Jun 11, 2020 at 08:59:05AM +0100, Marc Zyngier wrote:
> >diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> >index 9398b66f8a87..688213ef34f0 100644
> >--- a/arch/arm64/kvm/mmu.c
> >+++ b/arch/arm64/kvm/mmu.c
> >@@ -131,7 +131,8 @@ static int mmu_topup_memory_cache(struct
> >kvm_mmu_memory_cache *cache, int min)
> > if (cache->nobjs >= min)
> > return 0;
> > while (cache->nobjs < ARRAY_SIZE(cache->objects)) {
> >- page = (void *)__get_free_page(GFP_PGTABLE_USER);
> >+ page = (void *)__get_free_page(GFP_KERNEL_ACCOUNT |
>
> This is definitely a change in the way we account for guest
> page tables allocation, although I find it bizarre that not
> all architectures account for it the same way.
It's not intended to be a functional change, i.e. the allocations should
still be accounted:
#define GFP_PGTABLE_USER (GFP_PGTABLE_KERNEL | __GFP_ACCOUNT)
|
-> #define GFP_PGTABLE_KERNEL (GFP_KERNEL | __GFP_ZERO)
== GFP_KERNEL | __GFP_ACCOUNT | __GFP_ZERO
versus
#define GFP_KERNEL_ACCOUNT (GFP_KERNEL | __GFP_ACCOUNT)
with __GFP_ZERO explicitly OR'd in
== GFP_KERNEL | __GFP_ACCOUNT | __GFP_ZERO
I can put the above in the changelog, unless of course it's wrong and I've
missed something.
> It seems logical to me that nested page tables would be accounted
> against userspace, but I'm willing to be educated on the matter.
>
> Another possibility is that depending on the context, some allocations
> should be accounted on either the kernel or userspace (NV on arm64
> could definitely do something like that). If that was the case,
> maybe moving most of the GFP_* flags into the per-cache flags,
> and have the renaming that Ben suggested earlier.
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
On Wed, Jun 10, 2020 at 03:00:47PM -0700, Ben Gardon wrote:
> On Fri, Jun 5, 2020 at 2:39 PM Sean Christopherson
> <[email protected]> wrote:
> >
> > Replace the @max param in mmu_topup_memory_cache() and instead use
> > ARRAY_SIZE() to terminate the loop to fill the cache. This removes a
> > BUG_ON() and sets the stage for moving arm64 to the common memory cache
> > implementation.
> >
> > No functional change intended.
> >
> > Signed-off-by: Sean Christopherson <[email protected]>
> > ---
> > arch/arm64/kvm/mmu.c | 12 ++++--------
> > 1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index a1f6bc70c4e4..9398b66f8a87 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -124,15 +124,13 @@ static void stage2_dissolve_pud(struct kvm *kvm, phys_addr_t addr, pud_t *pudp)
> > put_page(virt_to_page(pudp));
> > }
> >
> > -static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
> > - int min, int max)
> > +static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache, int min)
> > {
> > void *page;
> >
> > - BUG_ON(max > KVM_NR_MEM_OBJS);
> KVM_NR_MEM_OBJS should be undefined as of patch 14 in this series. I'd
> recommend changing this to use the new constant you defined in that
> patch.
My intent was to leave KVM_NR_MEM_OBJS defined by arm64 and MIPS until they
move to the common implementation, e.g. this should be defined in
arch/arm64/include/asm/kvm_host.h until patch 18. I'll get cross-compiling
setup so I can properly test bisection before sending v2.
> > if (cache->nobjs >= min)
> > return 0;
> > - while (cache->nobjs < max) {
> > + while (cache->nobjs < ARRAY_SIZE(cache->objects)) {
> > page = (void *)__get_free_page(GFP_PGTABLE_USER);
> > if (!page)
> > return -ENOMEM;
> > @@ -1356,8 +1354,7 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> > pte = kvm_s2pte_mkwrite(pte);
> >
> > ret = mmu_topup_memory_cache(&cache,
> > - kvm_mmu_cache_min_pages(kvm),
> > - KVM_NR_MEM_OBJS);
> See above, KVM_NR_MEM_OBJS is undefined as of patch 14.
> > + kvm_mmu_cache_min_pages(kvm));
> > if (ret)
> > goto out;
> > spin_lock(&kvm->mmu_lock);
> > @@ -1737,8 +1734,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> > up_read(¤t->mm->mmap_sem);
> >
> > /* We need minimum second+third level pages */
> > - ret = mmu_topup_memory_cache(memcache, kvm_mmu_cache_min_pages(kvm),
> > - KVM_NR_MEM_OBJS);
> See above, KVM_NR_MEM_OBJS is undefined as of patch 14.
> > + ret = mmu_topup_memory_cache(memcache, kvm_mmu_cache_min_pages(kvm));
> > if (ret)
> > return ret;
> >
> > --
> > 2.26.0
> >
On 2020-06-11 16:43, Sean Christopherson wrote:
> On Thu, Jun 11, 2020 at 08:59:05AM +0100, Marc Zyngier wrote:
>> >diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>> >index 9398b66f8a87..688213ef34f0 100644
>> >--- a/arch/arm64/kvm/mmu.c
>> >+++ b/arch/arm64/kvm/mmu.c
>> >@@ -131,7 +131,8 @@ static int mmu_topup_memory_cache(struct
>> >kvm_mmu_memory_cache *cache, int min)
>> > if (cache->nobjs >= min)
>> > return 0;
>> > while (cache->nobjs < ARRAY_SIZE(cache->objects)) {
>> >- page = (void *)__get_free_page(GFP_PGTABLE_USER);
>> >+ page = (void *)__get_free_page(GFP_KERNEL_ACCOUNT |
>>
>> This is definitely a change in the way we account for guest
>> page tables allocation, although I find it bizarre that not
>> all architectures account for it the same way.
>
> It's not intended to be a functional change, i.e. the allocations
> should
> still be accounted:
>
> #define GFP_PGTABLE_USER (GFP_PGTABLE_KERNEL | __GFP_ACCOUNT)
> |
> -> #define GFP_PGTABLE_KERNEL (GFP_KERNEL | __GFP_ZERO)
>
> == GFP_KERNEL | __GFP_ACCOUNT | __GFP_ZERO
>
> versus
>
> #define GFP_KERNEL_ACCOUNT (GFP_KERNEL | __GFP_ACCOUNT)
>
> with __GFP_ZERO explicitly OR'd in
>
> == GFP_KERNEL | __GFP_ACCOUNT | __GFP_ZERO
>
> I can put the above in the changelog, unless of course it's wrong and
> I've
> missed something.
Ah, good point. Serves me right for judging the symbol at face value!
;-)
I guess a quick mention in the changelog wouldn't hurt.
Thanks,
M.
--
Jazz is not dead. It just smells funny...