[PATCH 0/3] x86: plug 2 vCPU creation resource leaks + some cleanup

[PATCH 0/3] x86: plug 2 vCPU creation resource leaks + some cleanup

[Jan Beulich]

1: vLAPIC: don't leak regs page from vlapic_init() upon error
2: fix resource leaks on arch_vcpu_create() error path
3: vLAPIC: vlapic_init() runs only once for a vCPU

Jan

[PATCH 1/3] x86/vLAPIC: don't leak regs page from vlapic_init() upon error

[Jan Beulich]

Fixes: 8a981e0bf25e ("Make map_domain_page_global fail")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1625,6 +1625,7 @@ int vlapic_init(struct vcpu *v)
         vlapic->regs = __map_domain_page_global(vlapic->regs_page);
         if ( vlapic->regs == NULL )
         {
+            free_domheap_page(vlapic->regs_page);
             dprintk(XENLOG_ERR, "map vlapic regs error: %d/%d\n",
                     v->domain->domain_id, v->vcpu_id);
             return -ENOMEM;

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

[Jan Beulich]

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

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.

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

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -569,6 +569,9 @@ int arch_vcpu_create(struct vcpu *v)
         vmce_init_vcpu(v);
 
         arch_vcpu_regs_init(v);
+
+        if ( (rc = init_vcpu_msr_policy(v)) )
+            goto fail;
     }
     else if ( (rc = xstate_alloc_save_area(v)) != 0 )
         return rc;
@@ -594,9 +597,6 @@ int arch_vcpu_create(struct vcpu *v)
     {
         vpmu_initialise(v);
 
-        if ( (rc = init_vcpu_msr_policy(v)) )
-            goto fail;
-
         cpuid_policy_updated(v);
     }
 

[PATCH 3/3] x86/vLAPIC: vlapic_init() runs only once for a vCPU

[Jan Beulich]

Hence there's no need to guard allocation / mapping by checks whether
the same action has been done before. I assume this was a transient
change which should have been undone before 509529e99148 ("x86 hvm: Xen
interface and implementation for virtual S3") got committed.

While touching this code, switch dprintk()-s to use %pv.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1610,27 +1610,21 @@ int vlapic_init(struct vcpu *v)
 
     vlapic->pt.source = PTSRC_lapic;
 
-    if (vlapic->regs_page == NULL)
+    vlapic->regs_page = alloc_domheap_page(v->domain, MEMF_no_owner);
+    if ( !vlapic->regs_page )
     {
-        vlapic->regs_page = alloc_domheap_page(v->domain, MEMF_no_owner);
-        if ( vlapic->regs_page == NULL )
-        {
-            dprintk(XENLOG_ERR, "alloc vlapic regs error: %d/%d\n",
-                    v->domain->domain_id, v->vcpu_id);
-            return -ENOMEM;
-        }
+        dprintk(XENLOG_ERR, "%pv: alloc vlapic regs error\n", v);
+        return -ENOMEM;
     }
-    if (vlapic->regs == NULL) 
+
+    vlapic->regs = __map_domain_page_global(vlapic->regs_page);
+    if ( vlapic->regs == NULL )
     {
-        vlapic->regs = __map_domain_page_global(vlapic->regs_page);
-        if ( vlapic->regs == NULL )
-        {
-            free_domheap_page(vlapic->regs_page);
-            dprintk(XENLOG_ERR, "map vlapic regs error: %d/%d\n",
-                    v->domain->domain_id, v->vcpu_id);
-            return -ENOMEM;
-        }
+        free_domheap_page(vlapic->regs_page);
+        dprintk(XENLOG_ERR, "%pv: map vlapic regs error\n", v);
+        return -ENOMEM;
     }
+
     clear_page(vlapic->regs);
 
     vlapic_reset(vlapic);

Re: [PATCH 1/3] x86/vLAPIC: don't leak regs page from vlapic_init() upon error

[Andrew Cooper]

On 02/10/2020 11:30, Jan Beulich wrote:
> Fixes: 8a981e0bf25e ("Make map_domain_page_global fail")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[Andrew Cooper]

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@suse.com>

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

Re: [PATCH 3/3] x86/vLAPIC: vlapic_init() runs only once for a vCPU

[Andrew Cooper]

On 02/10/2020 11:31, Jan Beulich wrote:
> Hence there's no need to guard allocation / mapping by checks whether
> the same action has been done before. I assume this was a transient
> change which should have been undone before 509529e99148 ("x86 hvm: Xen
> interface and implementation for virtual S3") got committed.
>
> While touching this code, switch dprintk()-s to use %pv.

Logging ENOMEM, especially without actually saying ENOMEM, is quite
pointless.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, preferably with
the printk()s dropped.

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

[Jan Beulich]

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@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> 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

Re: [PATCH 3/3] x86/vLAPIC: vlapic_init() runs only once for a vCPU

[Jan Beulich]

On 02.10.2020 13:19, Andrew Cooper wrote:
> On 02/10/2020 11:31, Jan Beulich wrote:
>> Hence there's no need to guard allocation / mapping by checks whether
>> the same action has been done before. I assume this was a transient
>> change which should have been undone before 509529e99148 ("x86 hvm: Xen
>> interface and implementation for virtual S3") got committed.
>>
>> While touching this code, switch dprintk()-s to use %pv.
> 
> Logging ENOMEM, especially without actually saying ENOMEM, is quite
> pointless.
> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, preferably with
> the printk()s dropped.

Thanks, and sure - I'll be happy to drop them. Just didn't want to
make more of a change than needed, and them being dprintk()-s didn't
make them look all that awful.

Jan