2019-10-22 02:05:51

by Sean Christopherson

[permalink] [raw]
Subject: [PATCH 00/45] KVM: Refactor vCPU creation

*************************** DISCLAIMER **********************************
The non-x86 arch specific patches are completely untested. Although the
changes are conceptually straightforward, I'm not remotely confident that
the patches are bug free, e.g. checkpatch caught several blatant typos
that would break compilation.
*************************************************************************

The end goal of this series is to strip down the interface between common
KVM code and arch specific code so that there is precisely one arch hook
for creating a vCPU and one hook for destroying a vCPU. In addition to
cleaning up the code base, simplifying the interface gives architectures
more freedom to organize their vCPU creation code.

KVM's vCPU creation code is comically messy. kvm_vm_ioctl_create_vcpu()
calls three separate arch hooks: init(), create() and setup(). The init()
call is especially nasty as it's hidden away in a common KVM function,
kvm_init_vcpu(), that for all intents and purposes must be immediately
invoked after the vcpu object is allocated.

Not to be outdone, vCPU destruction also has three arch hooks: uninit(),
destroy() and free(), the latter of which isn't actually invoked by common
KVM code, but the hook declaration still exists because architectures are
relying on its forward declaration.

Eliminating the extra arch hooks is relatively straightforward, just
tedious. For the most part, there is no fundamental constraint that
necessitated the proliferation of arch hooks, rather they crept in over
time, usually when x86-centric code was moved out of generic KVM and into
x86 code.

E.g. kvm_arch_vcpu_setup() was added to allow x86 to do vcpu_load(), which
can only be done after preempt_notifier initialization, but adding setup()
overlooked the fact that the preempt_notifier was only initialized after
kvm_arch_vcpu_create() because preemption support was added when x86's MMU
setup (the vcpu_load() user) was called from common KVM code.

For all intents and purposes, there is no true functional change in this
series. The order of some allocations will change, and a few memory leaks
are fixed, but the actual functionality of a guest should be unaffected.

Patches 01-03 are bug fixes in error handling paths that were found by
inspection when refactoring the associated code.

Patches 04-43 refactor each arch implementation so that the unwanted arch
hooks can be dropped without a functional change, e.g. move code out of
kvm_arch_vcpu_setup() so that all implementations are empty, then drop the
functions and caller.

Patches 44-45 are minor clean up to eliminate kvm_vcpu_uninit().


The net result is to go from this:

vcpu = kvm_arch_vcpu_create(kvm, id);
|
|-> kvm_vcpu_init()
|
|-> kvm_arch_vcpu_init()

if (IS_ERR(vcpu)) {
r = PTR_ERR(vcpu);
goto vcpu_decrement;
}

preempt_notifier_init(&vcpu->preempt_notifier, &kvm_preempt_ops);

r = kvm_arch_vcpu_setup(vcpu);
if (r)
goto vcpu_destroy;

to this:

r = kvm_arch_vcpu_precreate(kvm, id);
if (r)
goto vcpu_decrement;

vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
if (!vcpu) {
r = -ENOMEM;
goto vcpu_decrement;
}

page = alloc_page(GFP_KERNEL | __GFP_ZERO);
if (!page) {
r = -ENOMEM;
goto vcpu_free;
}
vcpu->run = page_address(page);

kvm_vcpu_init(vcpu, kvm, id);

r = kvm_arch_vcpu_create(vcpu);
if (r)
goto vcpu_free_run_page;


Sean Christopherson (45):
KVM: PPC: Book3S HV: Uninit vCPU if vcore creation fails
KVM: PPC: Book3S PR: Free shared page if mmu initialization fails
KVM: x86: Free wbinvd_dirty_mask if vCPU creation fails
KVM: VMX: Allocate VPID after initializing VCPU
KVM: VMX: Use direct vcpu pointer during vCPU create/free
KVM: SVM: Use direct vcpu pointer during vCPU create/free
KVM: x86: Allocate vcpu struct in common x86 code
KVM: x86: Move FPU allocation to common x86 code
KVM: x86: Move allocation of pio_data page down a few lines
KVM: x86: Move kvm_vcpu_init() invocation to common code
KVM: PPC: e500mc: Add build-time assert that vcpu is at offset 0
KVM: PPC: Allocate vcpu struct in common PPC code
KVM: PPC: Book3S PR: Allocate book3s and shadow vcpu after common init
KVM: PPC: e500mc: Move reset of oldpir below call to kvm_vcpu_init()
KVM: PPC: Move kvm_vcpu_init() invocation to common code
KVM: MIPS: Use kvm_vcpu_cache to allocate vCPUs
KVM: MIPS: Drop kvm_arch_vcpu_free()
KVM: PPC: Drop kvm_arch_vcpu_free()
KVM: arm: Drop kvm_arch_vcpu_free()
KVM: x86: Remove spurious kvm_mmu_unload() from vcpu destruction path
KVM: x86: Remove spurious clearing of async #PF MSR
KVM: x86: Drop kvm_arch_vcpu_free()
KVM: Remove kvm_arch_vcpu_free() declaration
KVM: Add kvm_arch_vcpu_precreate() to handle pre-allocation issues
KVM: s390: Move guts of kvm_arch_vcpu_init() into
kvm_arch_vcpu_create()
KVM: s390: Invoke kvm_vcpu_init() before allocating sie_page
KVM: MIPS: Invoke kvm_vcpu_uninit() immediately prior to freeing vcpu
KVM: x86: Invoke kvm_vcpu_uninit() immediately prior to freeing vcpu
KVM: Introduce kvm_vcpu_destroy()
KVM: Move vcpu alloc and init invocation to common code
KVM: Unexport kvm_vcpu_cache and kvm_{un}init_vcpu()
KVM: Move initialization of preempt notifier to kvm_vcpu_init()
KVM: x86: Move guts of kvm_arch_vcpu_setup() into
kvm_arch_vcpu_create()
KVM: MIPS: Move .vcpu_setup() call to kvm_arch_vcpu_create()
KVM: s390: Manually invoke vcpu setup during kvm_arch_vcpu_create()
KVM: PPC: BookE: Setup vcpu during kvmppc_core_vcpu_create()
KVM: Drop kvm_arch_vcpu_setup()
KVM: x86: Move all vcpu init code into kvm_arch_vcpu_create()
KVM: MIPS: Move all vcpu init code into kvm_arch_vcpu_create()
KVM: ARM: Move all vcpu init code into kvm_arch_vcpu_create()
KVM: PPC: Move all vcpu init code into kvm_arch_vcpu_create()
KVM: arm64: Free sve_state via arm specific hook
KVM: Drop kvm_arch_vcpu_init() and kvm_arch_vcpu_uninit()
KVM: Move putting of vcpu->pid to kvm_vcpu_destroy()
KVM: Move vcpu->run page allocation out of kvm_vcpu_init()

arch/arm/include/asm/kvm_host.h | 2 +-
arch/arm/kvm/guest.c | 5 -
arch/arm64/include/asm/kvm_host.h | 2 +-
arch/arm64/kvm/guest.c | 5 -
arch/arm64/kvm/reset.c | 2 +-
arch/mips/kvm/mips.c | 84 ++++-------
arch/powerpc/include/asm/kvm_ppc.h | 6 +-
arch/powerpc/kvm/book3s.c | 9 +-
arch/powerpc/kvm/book3s_hv.c | 27 +---
arch/powerpc/kvm/book3s_pr.c | 33 ++---
arch/powerpc/kvm/booke.c | 65 ++++----
arch/powerpc/kvm/e500.c | 34 +----
arch/powerpc/kvm/e500mc.c | 32 ++--
arch/powerpc/kvm/powerpc.c | 70 ++++-----
arch/s390/include/asm/kvm_host.h | 1 -
arch/s390/kvm/kvm-s390.c | 110 +++++++-------
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kvm/svm.c | 52 +------
arch/x86/kvm/vmx/vmx.c | 72 +++------
arch/x86/kvm/x86.c | 230 ++++++++++++++---------------
include/linux/kvm_host.h | 13 +-
virt/kvm/arm/arm.c | 76 ++++------
virt/kvm/kvm_main.c | 71 +++++----
23 files changed, 387 insertions(+), 616 deletions(-)

--
2.22.0


2019-10-22 02:06:42

by Sean Christopherson

[permalink] [raw]
Subject: [PATCH 07/45] KVM: x86: Allocate vcpu struct in common x86 code

Move allocation of VMX and SVM vcpus to common x86. Although the struct
being allocated is technically a VMX/SVM struct, it can be interpreted
directly as a 'struct kvm_vcpu' because of the pre-existing requirement
that 'struct kvm_vcpu' be located at offset zero of the arch/vendor vcpu
struct.

Remove the message from the build-time assertions regarding placement of
the struct, as compatibility with the arch usercopy region is no longer
the sole dependent on 'struct kvm_vcpu' being at offset zero.

Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kvm/svm.c | 28 +++++++++-------------------
arch/x86/kvm/vmx/vmx.c | 24 ++++++++----------------
arch/x86/kvm/x86.c | 16 ++++++++++++----
4 files changed, 30 insertions(+), 40 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 5d8056ff7390..77581eeb02e3 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1014,7 +1014,7 @@ struct kvm_x86_ops {
void (*vm_destroy)(struct kvm *kvm);

/* Create, but do not attach this VCPU */
- struct kvm_vcpu *(*vcpu_create)(struct kvm *kvm, unsigned id);
+ int (*vcpu_create)(struct kvm *kvm, struct kvm_vcpu *vcpu, unsigned id);
void (*vcpu_free)(struct kvm_vcpu *vcpu);
void (*vcpu_reset)(struct kvm_vcpu *vcpu, bool init_event);

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 2f66c52e1b5d..bbc5dac9d400 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2138,9 +2138,9 @@ static int avic_init_vcpu(struct vcpu_svm *svm)
return ret;
}

-static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
+static int svm_create_vcpu(struct kvm *kvm, struct kvm_vcpu *vcpu,
+ unsigned int id)
{
- struct kvm_vcpu *vcpu;
struct vcpu_svm *svm;
struct page *page;
struct page *msrpm_pages;
@@ -2148,22 +2148,15 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
struct page *nested_msrpm_pages;
int err;

- BUILD_BUG_ON_MSG(offsetof(struct vcpu_svm, vcpu) != 0,
- "struct kvm_vcpu must be at offset 0 for arch usercopy region");
-
- svm = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL_ACCOUNT);
- if (!svm) {
- err = -ENOMEM;
- goto out;
- }
- vcpu = &svm->vcpu;
+ BUILD_BUG_ON(offsetof(struct vcpu_svm, vcpu) != 0);
+ svm = to_svm(vcpu);

vcpu->arch.user_fpu = kmem_cache_zalloc(x86_fpu_cache,
GFP_KERNEL_ACCOUNT);
if (!vcpu->arch.user_fpu) {
printk(KERN_ERR "kvm: failed to allocate kvm userspace's fpu\n");
err = -ENOMEM;
- goto free_partial_svm;
+ goto out;
}

vcpu->arch.guest_fpu = kmem_cache_zalloc(x86_fpu_cache,
@@ -2176,7 +2169,7 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)

err = kvm_vcpu_init(vcpu, kvm, id);
if (err)
- goto free_svm;
+ goto free_guest_fpu;

err = -ENOMEM;
page = alloc_page(GFP_KERNEL_ACCOUNT);
@@ -2220,7 +2213,7 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)

svm_init_osvw(vcpu);

- return vcpu;
+ return 0;

free_page4:
__free_page(hsave_page);
@@ -2232,14 +2225,12 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
__free_page(page);
uninit:
kvm_vcpu_uninit(vcpu);
-free_svm:
+free_guest_fpu:
kmem_cache_free(x86_fpu_cache, vcpu->arch.guest_fpu);
free_user_fpu:
kmem_cache_free(x86_fpu_cache, vcpu->arch.user_fpu);
-free_partial_svm:
- kmem_cache_free(kvm_vcpu_cache, svm);
out:
- return ERR_PTR(err);
+ return err;
}

static void svm_clear_current_vmcb(struct vmcb *vmcb)
@@ -2268,7 +2259,6 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
kvm_vcpu_uninit(vcpu);
kmem_cache_free(x86_fpu_cache, vcpu->arch.user_fpu);
kmem_cache_free(x86_fpu_cache, vcpu->arch.guest_fpu);
- kmem_cache_free(kvm_vcpu_cache, svm);
}

static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 70b8d15eb2c5..832d8e38acd4 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6693,31 +6693,24 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu)
kvm_vcpu_uninit(vcpu);
kmem_cache_free(x86_fpu_cache, vcpu->arch.user_fpu);
kmem_cache_free(x86_fpu_cache, vcpu->arch.guest_fpu);
- kmem_cache_free(kvm_vcpu_cache, vmx);
}

-static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
+static int vmx_create_vcpu(struct kvm *kvm, struct kvm_vcpu *vcpu,
+ unsigned int id)
{
- struct kvm_vcpu *vcpu;
struct vcpu_vmx *vmx;
unsigned long *msr_bitmap;
int cpu, err;

- BUILD_BUG_ON_MSG(offsetof(struct vcpu_vmx, vcpu) != 0,
- "struct kvm_vcpu must be at offset 0 for arch usercopy region");
-
- vmx = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL_ACCOUNT);
- if (!vmx)
- return ERR_PTR(-ENOMEM);
-
- vcpu = &vmx->vcpu;
+ BUILD_BUG_ON(offsetof(struct vcpu_vmx, vcpu) != 0);
+ vmx = to_vmx(vcpu);

vcpu->arch.user_fpu = kmem_cache_zalloc(x86_fpu_cache,
GFP_KERNEL_ACCOUNT);
if (!vcpu->arch.user_fpu) {
printk(KERN_ERR "kvm: failed to allocate kvm userspace's fpu\n");
err = -ENOMEM;
- goto free_partial_vcpu;
+ goto out;
}

vcpu->arch.guest_fpu = kmem_cache_zalloc(x86_fpu_cache,
@@ -6815,7 +6808,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)

vmx->ept_pointer = INVALID_PAGE;

- return vcpu;
+ return 0;

free_vmcs:
free_loaded_vmcs(vmx->loaded_vmcs);
@@ -6830,9 +6823,8 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
kmem_cache_free(x86_fpu_cache, vcpu->arch.guest_fpu);
free_user_fpu:
kmem_cache_free(x86_fpu_cache, vcpu->arch.user_fpu);
-free_partial_vcpu:
- kmem_cache_free(kvm_vcpu_cache, vmx);
- return ERR_PTR(err);
+out:
+ return err;
}

#define L1TF_MSG_SMT "L1TF CPU bug present and SMT on, data leak possible. See CVE-2018-3646 and https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/l1tf.html for details.\n"
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c9a291693279..45b296a9fdbb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9012,26 +9012,34 @@ static void fx_init(struct kvm_vcpu *vcpu)

void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)
{
- void *wbinvd_dirty_mask = vcpu->arch.wbinvd_dirty_mask;
-
kvmclock_reset(vcpu);

kvm_x86_ops->vcpu_free(vcpu);
- free_cpumask_var(wbinvd_dirty_mask);
+
+ free_cpumask_var(vcpu->arch.wbinvd_dirty_mask);
+ kmem_cache_free(kvm_vcpu_cache, vcpu);
}

struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
unsigned int id)
{
struct kvm_vcpu *vcpu;
+ int r;

if (kvm_check_tsc_unstable() && atomic_read(&kvm->online_vcpus) != 0)
printk_once(KERN_WARNING
"kvm: SMP vm created on host with unstable TSC; "
"guest TSC will not be reliable\n");

- vcpu = kvm_x86_ops->vcpu_create(kvm, id);
+ vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL_ACCOUNT);
+ if (!vcpu)
+ return ERR_PTR(-ENOMEM);

+ r = kvm_x86_ops->vcpu_create(kvm, vcpu, id);
+ if (r) {
+ kmem_cache_free(kvm_vcpu_cache, vcpu);
+ return ERR_PTR(r);
+ }
return vcpu;
}

--
2.22.0

2019-10-22 02:07:15

by Sean Christopherson

[permalink] [raw]
Subject: [PATCH 06/45] KVM: SVM: Use direct vcpu pointer during vCPU create/free

Capture the vcpu pointer in a local varaible and replace '&svm->vcpu'
references with a direct reference to the pointer in anticipation of
moving bits of the code to common x86 and passing the vcpu pointer into
svm_create_vcpu(), i.e. eliminate unnecessary noise from future patches.

Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/svm.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index e479ea9bc9da..2f66c52e1b5d 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2140,6 +2140,7 @@ static int avic_init_vcpu(struct vcpu_svm *svm)

static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
{
+ struct kvm_vcpu *vcpu;
struct vcpu_svm *svm;
struct page *page;
struct page *msrpm_pages;
@@ -2155,24 +2156,25 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
err = -ENOMEM;
goto out;
}
+ vcpu = &svm->vcpu;

- svm->vcpu.arch.user_fpu = kmem_cache_zalloc(x86_fpu_cache,
- GFP_KERNEL_ACCOUNT);
- if (!svm->vcpu.arch.user_fpu) {
+ vcpu->arch.user_fpu = kmem_cache_zalloc(x86_fpu_cache,
+ GFP_KERNEL_ACCOUNT);
+ if (!vcpu->arch.user_fpu) {
printk(KERN_ERR "kvm: failed to allocate kvm userspace's fpu\n");
err = -ENOMEM;
goto free_partial_svm;
}

- svm->vcpu.arch.guest_fpu = kmem_cache_zalloc(x86_fpu_cache,
- GFP_KERNEL_ACCOUNT);
- if (!svm->vcpu.arch.guest_fpu) {
+ vcpu->arch.guest_fpu = kmem_cache_zalloc(x86_fpu_cache,
+ GFP_KERNEL_ACCOUNT);
+ if (!vcpu->arch.guest_fpu) {
printk(KERN_ERR "kvm: failed to allocate vcpu's fpu\n");
err = -ENOMEM;
goto free_user_fpu;
}

- err = kvm_vcpu_init(&svm->vcpu, kvm, id);
+ err = kvm_vcpu_init(vcpu, kvm, id);
if (err)
goto free_svm;

@@ -2216,9 +2218,9 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
svm->asid_generation = 0;
init_vmcb(svm);

- svm_init_osvw(&svm->vcpu);
+ svm_init_osvw(vcpu);

- return &svm->vcpu;
+ return vcpu;

free_page4:
__free_page(hsave_page);
@@ -2229,11 +2231,11 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
free_page1:
__free_page(page);
uninit:
- kvm_vcpu_uninit(&svm->vcpu);
+ kvm_vcpu_uninit(vcpu);
free_svm:
- kmem_cache_free(x86_fpu_cache, svm->vcpu.arch.guest_fpu);
+ kmem_cache_free(x86_fpu_cache, vcpu->arch.guest_fpu);
free_user_fpu:
- kmem_cache_free(x86_fpu_cache, svm->vcpu.arch.user_fpu);
+ kmem_cache_free(x86_fpu_cache, vcpu->arch.user_fpu);
free_partial_svm:
kmem_cache_free(kvm_vcpu_cache, svm);
out:
@@ -2264,8 +2266,8 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
__free_page(virt_to_page(svm->nested.hsave));
__free_pages(virt_to_page(svm->nested.msrpm), MSRPM_ALLOC_ORDER);
kvm_vcpu_uninit(vcpu);
- kmem_cache_free(x86_fpu_cache, svm->vcpu.arch.user_fpu);
- kmem_cache_free(x86_fpu_cache, svm->vcpu.arch.guest_fpu);
+ kmem_cache_free(x86_fpu_cache, vcpu->arch.user_fpu);
+ kmem_cache_free(x86_fpu_cache, vcpu->arch.guest_fpu);
kmem_cache_free(kvm_vcpu_cache, svm);
}

--
2.22.0

2019-10-22 15:33:33

by Christoffer Dall

[permalink] [raw]
Subject: Re: [PATCH 00/45] KVM: Refactor vCPU creation

Hi Sean,

On Mon, Oct 21, 2019 at 06:58:40PM -0700, Sean Christopherson wrote:
> *************************** DISCLAIMER **********************************
> The non-x86 arch specific patches are completely untested. Although the
> changes are conceptually straightforward, I'm not remotely confident that
> the patches are bug free, e.g. checkpatch caught several blatant typos
> that would break compilation.
> *************************************************************************
>
> The end goal of this series is to strip down the interface between common
> KVM code and arch specific code so that there is precisely one arch hook
> for creating a vCPU and one hook for destroying a vCPU. In addition to
> cleaning up the code base, simplifying the interface gives architectures
> more freedom to organize their vCPU creation code.
>
> KVM's vCPU creation code is comically messy. kvm_vm_ioctl_create_vcpu()
> calls three separate arch hooks: init(), create() and setup(). The init()
> call is especially nasty as it's hidden away in a common KVM function,
> kvm_init_vcpu(), that for all intents and purposes must be immediately
> invoked after the vcpu object is allocated.
>
> Not to be outdone, vCPU destruction also has three arch hooks: uninit(),
> destroy() and free(), the latter of which isn't actually invoked by common
> KVM code, but the hook declaration still exists because architectures are
> relying on its forward declaration.
>
> Eliminating the extra arch hooks is relatively straightforward, just
> tedious. For the most part, there is no fundamental constraint that
> necessitated the proliferation of arch hooks, rather they crept in over
> time, usually when x86-centric code was moved out of generic KVM and into
> x86 code.
>
> E.g. kvm_arch_vcpu_setup() was added to allow x86 to do vcpu_load(), which
> can only be done after preempt_notifier initialization, but adding setup()
> overlooked the fact that the preempt_notifier was only initialized after
> kvm_arch_vcpu_create() because preemption support was added when x86's MMU
> setup (the vcpu_load() user) was called from common KVM code.
>
> For all intents and purposes, there is no true functional change in this
> series. The order of some allocations will change, and a few memory leaks
> are fixed, but the actual functionality of a guest should be unaffected.
>
> Patches 01-03 are bug fixes in error handling paths that were found by
> inspection when refactoring the associated code.
>
> Patches 04-43 refactor each arch implementation so that the unwanted arch
> hooks can be dropped without a functional change, e.g. move code out of
> kvm_arch_vcpu_setup() so that all implementations are empty, then drop the
> functions and caller.
>
> Patches 44-45 are minor clean up to eliminate kvm_vcpu_uninit().
>
>
> The net result is to go from this:
>
> vcpu = kvm_arch_vcpu_create(kvm, id);
> |
> |-> kvm_vcpu_init()
> |
> |-> kvm_arch_vcpu_init()
>
> if (IS_ERR(vcpu)) {
> r = PTR_ERR(vcpu);
> goto vcpu_decrement;
> }
>
> preempt_notifier_init(&vcpu->preempt_notifier, &kvm_preempt_ops);
>
> r = kvm_arch_vcpu_setup(vcpu);
> if (r)
> goto vcpu_destroy;
>
> to this:
>
> r = kvm_arch_vcpu_precreate(kvm, id);
> if (r)
> goto vcpu_decrement;
>
> vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
> if (!vcpu) {
> r = -ENOMEM;
> goto vcpu_decrement;
> }
>
> page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> if (!page) {
> r = -ENOMEM;
> goto vcpu_free;
> }
> vcpu->run = page_address(page);
>
> kvm_vcpu_init(vcpu, kvm, id);
>
> r = kvm_arch_vcpu_create(vcpu);
> if (r)
> goto vcpu_free_run_page;
>

What a fantastically welcome piece of work! Thanks for doing this,
many's the time I waded through all those calls to ensure a patch was
doing the right thing.

Modulo the nit in patch 42, the arm64 changes survive a guest boot +
hackbench and build fine. The lack of changing the arm-specific destroy
function to a void also causes a series of warnings for a 32-bit arm
build, but otherwise builds fine.

You can add my:

Acked-by: Christoffer Dall <[email protected]>

To the arm/arm64 and generic parts.


Thanks,

Christoffer