Re: [PATCH v1 05/28] x86/hvm: cleanup svm_create_vmcb()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/11/2026 1:33 am, dmukhin@xxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> svm_create_vmcb() does not need to be idempotent because in case of
> failure caller must call svm_destroy_vmcb().
>
> Simplify code in svm_create_vmcb() to account for that.

This is more complicated than it seems.

There are two paths which need to clean up a struct vcpu:
1) The domain_kill hypercall (i.e. clean destruction)
2) A failure within vcpu_create()

Nowadays, both vcpu and domain create() end up calling back into the
relevant destroy paths.  This is some work I started a long time ago and
never managed to finish.

We had a number of memory leaks because each init/create function was
required to clean up after itself, and several did this incorrectly.  It
was also wasteful because we had duplicated cleanup code.

At the moment, both vcpu_create() and hvm_vcpu_initialise() will call
the relevant destruction path.

In the future, we want to remove all the create() error handling except
from the top-level, because that removes the duplication.  However this
is a large task and it's the common/arch boundary where it gets
particularly hard to organise safely.


Anyway.  We can take this patch with an adjusted commit message.  In
particular, saying that the "caller must call ..." is a hope, rather
than a clear statement that this is how the code behaves.  I suggest
something like:

---8<---
svm_create_vmcb() does not need to do error handling itself because
vcpu_create() handles this at the top level by calling vcpu_destroy().
---8<---

But, if someone has some copious free time, it would be nicer if we
could do this a little bit wider and make better progress towards
removing the create() error handling.

~Andrew