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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.10.2020 13:13, Andrew Cooper wrote:
> 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?

It's probably more a "happened to be that way and then it was easiest
to keep it like this", but I recall the behavior having been the subject
of discussions, with the outcome that it's at least "kind of" intended.

Would adding "kind of" make things look better to you?

>> 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.

If you had said "The more interesting ...", I'd have agreed. What I
was thinking of as possible uses (be it reading or writing) is
e.g. reset state that may depend on certain further properties.

Furthermore I was thinking of code paths that vCPU initialization
simply may re-use, and which expect the policy to at least be
available, irrespective of the individual MSRs' values still
being their reset 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.

Thanks. As far as adjustments go, I don't really see how to better
reflect what you want, considering my replies above. If you have
any hints ... (I'll hold off committing this for a little while,
but I think I'd like to put it in before I leave for weekend and
vacation.)

Jan