On Tue, May 11, 2021 at 10:56 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
On Tue, May 11, 2021, Ben Gardon wrote:
Small refactor to facilitate allocating rmaps for all memslots at once.
No functional change expected.
Signed-off-by: Ben Gardon <bgardon@xxxxxxxxxx>
---
arch/x86/kvm/x86.c | 39 ++++++++++++++++++++++++++++++---------
1 file changed, 30 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1e1f4f31e586..cc0440b5b35d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10911,10 +10911,35 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot)
kvm_page_track_free_memslot(slot);
}
+static int memslot_rmap_alloc(struct kvm_memory_slot *slot,
+ unsigned long npages)
+{
+ int i;
+
+ for (i = 0; i < KVM_NR_PAGE_SIZES; ++i) {
+ int lpages;
+ int level = i + 1;
+
+ lpages = gfn_to_index(slot->base_gfn + npages - 1,
+ slot->base_gfn, level) + 1;
Might as well assign lpages at its declaration, i.e.
int lpages = gfn_to_index(slot->base_gfn + npages - 1,
slot->base_gfn, level) + 1;
I'll do this if I end up sending out a v5.
+
+ slot->arch.rmap[i] =
+ kvcalloc(lpages, sizeof(*slot->arch.rmap[i]),
+ GFP_KERNEL_ACCOUNT);
Eh, I don't think avoiding a 3 char overrun is worth splitting across three lines.
E.g. this is perfectly readable
slot->arch.rmap[i] = kvcalloc(lpages, sizeof(*slot->arch.rmap[i]),
GFP_KERNEL_ACCOUNT);
Alternatively, the rmap size could be captured in a local var, e.g.
const int sz = sizeof(*slot->arch.rmap[0]);
...
slot->arch.rmap[i] = kvcalloc(lpages, sz, GFP_KERNEL_ACCOUNT);
I like this suggestion. Much nicer. Will incorporate if I send a v5.
if (!slot->arch.rmap[i]) {
memslot_rmap_free(slot);
return -ENOMEM;
}
+ if (!slot->arch.rmap[i]) {
+ memslot_rmap_free(slot);
+ return -ENOMEM;
Reaaaally getting into nitpicks, what do you think about changing this to a goto
with the error handling at the bottom? Obviously not necessary by any means,
but for me it makes it easier to see that all rmaps are freed on failure. My
eyes skipped over that on the first read through. E.g.
if (!slot_arch.rmap[i])
goto err;
}
return 0;
err:
memslot_rmap_free(slot);
return -ENOMEM;
Lol, I had a goto in v3, but David Hildenbrand suggested removing it
and putting the free in the loop. I think I like it more this way too.