Re: [PATCH 2/3] x86: fix resource leaks on arch_vcpu_create() error path

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/10/2020 11:30, Jan Beulich wrote:
> {hvm,pv}_vcpu_initialise() have always been meant to be the final
> possible source of errors in arch_vcpu_create(), hence not requiring
> any unrolling of what they've done on the error path. (Of course this
> may change once the various involved paths all have become idempotent.)

I'd agree that the way the code was previously laid out expected
{hvm,pv}_vcpu_initialise() to be the final failing option.

I don't think "has always meant to be" is reasonable, because where is
the code comment explaining this design choice?

The idempotent plans will definitely be removing this misbehaviour, and
the memory leaks it causes.

> But even beyond this aspect I think it is more logical to do policy
> initialization ahead of the calling of these two functions, as they may
> in principle want to access it.

Not these MSRs.  They're currently a block of zeroes, and while that
will eventually change, it will still be a bunch of MSRs in their RESET
state.

The interesting MSRs are the domain ones, not the vCPU ones.

>
> Fixes: 4187f79dc718 ("x86/msr: introduce struct msr_vcpu_policy")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> although I'd
prefer some adjustment to the commit message along the indicated lines.