[PATCH] xen/x86: Fix memory leak in vcpu_create() error path

[PATCH] xen/x86: Fix memory leak in vcpu_create() error path

[Andrew Cooper]

Various paths in vcpu_create() end up calling paging_update_paging_modes(),
which eventually allocate a monitor pagetable if one doesn't exist.

However, an error in vcpu_create() results in the vcpu being cleaned up
locally, and not put onto the domain's vcpu list.  Therefore, the monitor
table is not freed by {hap,shadow}_teardown()'s loop.  This is caught by
assertions later that we've successfully freed the entire hap/shadow memory
pool.

The per-vcpu loops in domain teardown logic is conceptually wrong, but exist
due to insufficient existing structure in the existing logic.

Break paging_vcpu_teardown() out of paging_teardown(), with mirrored breakouts
in the hap/shadow code, and use it from arch_vcpu_create()'s error path.  This
fixes the memory leak.

The new {hap,shadow}_vcpu_teardown() must be idempotent, and are written to be
as tolerable as possible, with the minimum number of safety checks possible.
In particular, drop the mfn_valid() check - if junk is in these fields, then
Xen is going to explode anyway.

Reported-by: Michał Leszczyński <michal.leszczynski@cert.pl>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Tim Deegan <tim@xen.org>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>

This is a minimal patch which ought to be safe to backport.  The whole cleanup
infrastructure is a mess.
---
 xen/arch/x86/domain.c           |  1 +
 xen/arch/x86/mm/hap/hap.c       | 39 ++++++++++++++++++-------------
 xen/arch/x86/mm/paging.c        |  8 +++++++
 xen/arch/x86/mm/shadow/common.c | 52 ++++++++++++++++++++++++-----------------
 xen/include/asm-x86/hap.h       |  1 +
 xen/include/asm-x86/paging.h    |  3 ++-
 xen/include/asm-x86/shadow.h    |  3 ++-
 7 files changed, 67 insertions(+), 40 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index e8e91cf080..b8f5b1f5b4 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -603,6 +603,7 @@ int arch_vcpu_create(struct vcpu *v)
     return rc;
 
  fail:
+    paging_vcpu_teardown(v);
     vcpu_destroy_fpu(v);
     xfree(v->arch.msrs);
     v->arch.msrs = NULL;
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 4eedd1a995..737821a166 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -563,30 +563,37 @@ void hap_final_teardown(struct domain *d)
     paging_unlock(d);
 }
 
+void hap_vcpu_teardown(struct vcpu *v)
+{
+    struct domain *d = v->domain;
+    mfn_t mfn;
+
+    paging_lock(d);
+
+    if ( !paging_mode_hap(d) || !v->arch.paging.mode )
+        goto out;
+
+    mfn = pagetable_get_mfn(v->arch.hvm.monitor_table);
+    if ( mfn_x(mfn) )
+        hap_destroy_monitor_table(v, mfn);
+    v->arch.hvm.monitor_table = pagetable_null();
+
+ out:
+    paging_unlock(d);
+}
+
 void hap_teardown(struct domain *d, bool *preempted)
 {
     struct vcpu *v;
-    mfn_t mfn;
 
     ASSERT(d->is_dying);
     ASSERT(d != current->domain);
 
-    paging_lock(d); /* Keep various asserts happy */
+    /* TODO - Remove when the teardown path is better structured. */
+    for_each_vcpu ( d, v )
+        hap_vcpu_teardown(v);
 
-    if ( paging_mode_enabled(d) )
-    {
-        /* release the monitor table held by each vcpu */
-        for_each_vcpu ( d, v )
-        {
-            if ( paging_get_hostmode(v) && paging_mode_external(d) )
-            {
-                mfn = pagetable_get_mfn(v->arch.hvm.monitor_table);
-                if ( mfn_valid(mfn) && (mfn_x(mfn) != 0) )
-                    hap_destroy_monitor_table(v, mfn);
-                v->arch.hvm.monitor_table = pagetable_null();
-            }
-        }
-    }
+    paging_lock(d); /* Keep various asserts happy */
 
     if ( d->arch.paging.hap.total_pages != 0 )
     {
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 695372783d..d5e967fcd5 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -794,6 +794,14 @@ long paging_domctl_continuation(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 }
 #endif /* CONFIG_PV_SHIM_EXCLUSIVE */
 
+void paging_vcpu_teardown(struct vcpu *v)
+{
+    if ( hap_enabled(v->domain) )
+        hap_vcpu_teardown(v);
+    else
+        shadow_vcpu_teardown(v);
+}
+
 /* Call when destroying a domain */
 int paging_teardown(struct domain *d)
 {
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 7c7204fd34..ea51068530 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -2775,6 +2775,32 @@ int shadow_enable(struct domain *d, u32 mode)
     return rv;
 }
 
+void shadow_vcpu_teardown(struct vcpu *v)
+{
+    struct domain *d = v->domain;
+
+    paging_lock(d);
+
+    if ( !paging_mode_shadow(d) || !v->arch.paging.mode )
+        goto out;
+
+    v->arch.paging.mode->shadow.detach_old_tables(v);
+#ifdef CONFIG_HVM
+    if ( shadow_mode_external(d) )
+    {
+        mfn_t mfn = pagetable_get_mfn(v->arch.hvm.monitor_table);
+
+        if ( mfn_x(mfn) )
+            v->arch.paging.mode->shadow.destroy_monitor_table(v, mfn);
+
+        v->arch.hvm.monitor_table = pagetable_null();
+    }
+#endif
+
+ out:
+    paging_unlock(d);
+}
+
 void shadow_teardown(struct domain *d, bool *preempted)
 /* Destroy the shadow pagetables of this domain and free its shadow memory.
  * Should only be called for dying domains. */
@@ -2785,29 +2811,11 @@ void shadow_teardown(struct domain *d, bool *preempted)
     ASSERT(d->is_dying);
     ASSERT(d != current->domain);
 
-    paging_lock(d);
-
-    if ( shadow_mode_enabled(d) )
-    {
-        /* Release the shadow and monitor tables held by each vcpu */
-        for_each_vcpu(d, v)
-        {
-            if ( v->arch.paging.mode )
-            {
-                v->arch.paging.mode->shadow.detach_old_tables(v);
-#ifdef CONFIG_HVM
-                if ( shadow_mode_external(d) )
-                {
-                    mfn_t mfn = pagetable_get_mfn(v->arch.hvm.monitor_table);
+    /* TODO - Remove when the teardown path is better structured. */
+    for_each_vcpu ( d, v )
+        shadow_vcpu_teardown(v);
 
-                    if ( mfn_valid(mfn) && (mfn_x(mfn) != 0) )
-                        v->arch.paging.mode->shadow.destroy_monitor_table(v, mfn);
-                    v->arch.hvm.monitor_table = pagetable_null();
-                }
-#endif /* CONFIG_HVM */
-            }
-        }
-    }
+    paging_lock(d);
 
 #if (SHADOW_OPTIMIZATIONS & (SHOPT_VIRTUAL_TLB|SHOPT_OUT_OF_SYNC))
     /* Free the virtual-TLB array attached to each vcpu */
diff --git a/xen/include/asm-x86/hap.h b/xen/include/asm-x86/hap.h
index d489df3812..90dece29de 100644
--- a/xen/include/asm-x86/hap.h
+++ b/xen/include/asm-x86/hap.h
@@ -36,6 +36,7 @@ int   hap_domctl(struct domain *d, struct xen_domctl_shadow_op *sc,
                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
 int   hap_enable(struct domain *d, u32 mode);
 void  hap_final_teardown(struct domain *d);
+void  hap_vcpu_teardown(struct vcpu *v);
 void  hap_teardown(struct domain *d, bool *preempted);
 void  hap_vcpu_init(struct vcpu *v);
 int   hap_track_dirty_vram(struct domain *d,
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index b803efa7b5..6b8ed80c64 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -234,7 +234,8 @@ int paging_domctl(struct domain *d, struct xen_domctl_shadow_op *sc,
 /* Helper hypercall for dealing with continuations. */
 long paging_domctl_continuation(XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
 
-/* Call when destroying a domain */
+/* Call when destroying a vcpu/domain */
+void paging_vcpu_teardown(struct vcpu *v);
 int paging_teardown(struct domain *d);
 
 /* Call once all of the references to the domain have gone away */
diff --git a/xen/include/asm-x86/shadow.h b/xen/include/asm-x86/shadow.h
index 76e47f257f..29a86ed78e 100644
--- a/xen/include/asm-x86/shadow.h
+++ b/xen/include/asm-x86/shadow.h
@@ -74,7 +74,8 @@ int shadow_domctl(struct domain *d,
                   struct xen_domctl_shadow_op *sc,
                   XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
 
-/* Call when destroying a domain */
+/* Call when destroying a vcpu/domain */
+void shadow_vcpu_teardown(struct vcpu *v);
 void shadow_teardown(struct domain *d, bool *preempted);
 
 /* Call once all of the references to the domain have gone away */
-- 
2.11.0

Re: [PATCH] xen/x86: Fix memory leak in vcpu_create() error path

[Jan Beulich]

On 28.09.2020 17:47, Andrew Cooper wrote:
> Various paths in vcpu_create() end up calling paging_update_paging_modes(),
> which eventually allocate a monitor pagetable if one doesn't exist.
> 
> However, an error in vcpu_create() results in the vcpu being cleaned up
> locally, and not put onto the domain's vcpu list.  Therefore, the monitor
> table is not freed by {hap,shadow}_teardown()'s loop.  This is caught by
> assertions later that we've successfully freed the entire hap/shadow memory
> pool.
> 
> The per-vcpu loops in domain teardown logic is conceptually wrong, but exist
> due to insufficient existing structure in the existing logic.
> 
> Break paging_vcpu_teardown() out of paging_teardown(), with mirrored breakouts
> in the hap/shadow code, and use it from arch_vcpu_create()'s error path.  This
> fixes the memory leak.
> 
> The new {hap,shadow}_vcpu_teardown() must be idempotent, and are written to be
> as tolerable as possible, with the minimum number of safety checks possible.
> In particular, drop the mfn_valid() check - if junk is in these fields, then
> Xen is going to explode anyway.
> 
> Reported-by: Michał Leszczyński <michal.leszczynski@cert.pl>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
yet I've got a couple of simple questions:

> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -563,30 +563,37 @@ void hap_final_teardown(struct domain *d)
>      paging_unlock(d);
>  }
>  
> +void hap_vcpu_teardown(struct vcpu *v)
> +{
> +    struct domain *d = v->domain;
> +    mfn_t mfn;
> +
> +    paging_lock(d);
> +
> +    if ( !paging_mode_hap(d) || !v->arch.paging.mode )
> +        goto out;

Any particular reason you don't use paging_get_hostmode() (as the
original code did) here? Any particular reason for the seemingly
redundant (and hence somewhat in conflict with the description's
"with the minimum number of safety checks possible")
paging_mode_hap()?

> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -2775,6 +2775,32 @@ int shadow_enable(struct domain *d, u32 mode)
>      return rv;
>  }
>  
> +void shadow_vcpu_teardown(struct vcpu *v)
> +{
> +    struct domain *d = v->domain;
> +
> +    paging_lock(d);
> +
> +    if ( !paging_mode_shadow(d) || !v->arch.paging.mode )

Same question regarding paging_get_hostmode() here, albeit I see
the original code open-coded it in this case.

Jan

Re: [PATCH] xen/x86: Fix memory leak in vcpu_create() error path

[Andrew Cooper]

On 29/09/2020 07:18, Jan Beulich wrote:
> On 28.09.2020 17:47, Andrew Cooper wrote:
>> Various paths in vcpu_create() end up calling paging_update_paging_modes(),
>> which eventually allocate a monitor pagetable if one doesn't exist.
>>
>> However, an error in vcpu_create() results in the vcpu being cleaned up
>> locally, and not put onto the domain's vcpu list.  Therefore, the monitor
>> table is not freed by {hap,shadow}_teardown()'s loop.  This is caught by
>> assertions later that we've successfully freed the entire hap/shadow memory
>> pool.
>>
>> The per-vcpu loops in domain teardown logic is conceptually wrong, but exist
>> due to insufficient existing structure in the existing logic.
>>
>> Break paging_vcpu_teardown() out of paging_teardown(), with mirrored breakouts
>> in the hap/shadow code, and use it from arch_vcpu_create()'s error path.  This
>> fixes the memory leak.
>>
>> The new {hap,shadow}_vcpu_teardown() must be idempotent, and are written to be
>> as tolerable as possible, with the minimum number of safety checks possible.
>> In particular, drop the mfn_valid() check - if junk is in these fields, then
>> Xen is going to explode anyway.
>>
>> Reported-by: Michał Leszczyński <michal.leszczynski@cert.pl>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.  (Wow it really is a long time since needing to drop everything
for security work...)

>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -563,30 +563,37 @@ void hap_final_teardown(struct domain *d)
>>      paging_unlock(d);
>>  }
>>  
>> +void hap_vcpu_teardown(struct vcpu *v)
>> +{
>> +    struct domain *d = v->domain;
>> +    mfn_t mfn;
>> +
>> +    paging_lock(d);
>> +
>> +    if ( !paging_mode_hap(d) || !v->arch.paging.mode )
>> +        goto out;
> Any particular reason you don't use paging_get_hostmode() (as the
> original code did) here? Any particular reason for the seemingly
> redundant (and hence somewhat in conflict with the description's
> "with the minimum number of safety checks possible")
> paging_mode_hap()?

Yes to both.  As you spotted, I converted the shadow side first, and
made the two consistent.

The paging_mode_{shadow,hap})() is necessary for idempotency.  These
functions really might get called before paging is set up, for an early
failure in domain_create().

The paging mode has nothing really to do with hostmode/guestmode/etc. 
It is the only way of expressing the logic where it is clear that the
lower pointer dereferences are trivially safe.  (Also, the guestmode
predicate isn't going to survive the nested virt work.  It's
conceptually broken.)

~Andrew

Re: [PATCH] xen/x86: Fix memory leak in vcpu_create() error path

[Jan Beulich]

On 17.12.2020 22:46, Andrew Cooper wrote:
> On 29/09/2020 07:18, Jan Beulich wrote:
>> On 28.09.2020 17:47, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/mm/hap/hap.c
>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>> @@ -563,30 +563,37 @@ void hap_final_teardown(struct domain *d)
>>>      paging_unlock(d);
>>>  }
>>>  
>>> +void hap_vcpu_teardown(struct vcpu *v)
>>> +{
>>> +    struct domain *d = v->domain;
>>> +    mfn_t mfn;
>>> +
>>> +    paging_lock(d);
>>> +
>>> +    if ( !paging_mode_hap(d) || !v->arch.paging.mode )
>>> +        goto out;
>> Any particular reason you don't use paging_get_hostmode() (as the
>> original code did) here? Any particular reason for the seemingly
>> redundant (and hence somewhat in conflict with the description's
>> "with the minimum number of safety checks possible")
>> paging_mode_hap()?
> 
> Yes to both.  As you spotted, I converted the shadow side first, and
> made the two consistent.
> 
> The paging_mode_{shadow,hap})() is necessary for idempotency.  These
> functions really might get called before paging is set up, for an early
> failure in domain_create().

In which case how would v->arch.paging.mode be non-NULL already?
They get set in {hap,shadow}_vcpu_init() only.

> The paging mode has nothing really to do with hostmode/guestmode/etc. 
> It is the only way of expressing the logic where it is clear that the
> lower pointer dereferences are trivially safe.

Well, yes and no - the other uses of course should then also use
paging_get_hostmode(), like various of the wrappers in paging.h
do. Or else I question why we have paging_get_hostmode() in the
first place. There are more examples in shadow code where this
gets open-coded when it probably shouldn't be. There haven't been
any such cases in HAP code so far ...

Additionally (noticing only now) in the shadow case you may now
loop over all vCPU-s in shadow_teardown() just for
shadow_vcpu_teardown() to bail right away. Wouldn't it make sense
to retain the "if ( shadow_mode_enabled(d) )" there around the
loop?

Jan

Re: [PATCH] xen/x86: Fix memory leak in vcpu_create() error path

[Andrew Cooper]

On 18/12/2020 08:27, Jan Beulich wrote:
> On 17.12.2020 22:46, Andrew Cooper wrote:
>> On 29/09/2020 07:18, Jan Beulich wrote:
>>> On 28.09.2020 17:47, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/mm/hap/hap.c
>>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>>> @@ -563,30 +563,37 @@ void hap_final_teardown(struct domain *d)
>>>>      paging_unlock(d);
>>>>  }
>>>>  
>>>> +void hap_vcpu_teardown(struct vcpu *v)
>>>> +{
>>>> +    struct domain *d = v->domain;
>>>> +    mfn_t mfn;
>>>> +
>>>> +    paging_lock(d);
>>>> +
>>>> +    if ( !paging_mode_hap(d) || !v->arch.paging.mode )
>>>> +        goto out;
>>> Any particular reason you don't use paging_get_hostmode() (as the
>>> original code did) here? Any particular reason for the seemingly
>>> redundant (and hence somewhat in conflict with the description's
>>> "with the minimum number of safety checks possible")
>>> paging_mode_hap()?
>> Yes to both.  As you spotted, I converted the shadow side first, and
>> made the two consistent.
>>
>> The paging_mode_{shadow,hap})() is necessary for idempotency.  These
>> functions really might get called before paging is set up, for an early
>> failure in domain_create().
> In which case how would v->arch.paging.mode be non-NULL already?
> They get set in {hap,shadow}_vcpu_init() only.

Right, but we also might end up here with an error early in
vcpu_create(), where d->arch.paging is set up, but v->arch.paging isn't.

This logic needs to be safe to use at any point of partial initialisation.

(And to be clear, I found I needed both of these based on some
artificial error injection testing.)

>> The paging mode has nothing really to do with hostmode/guestmode/etc. 
>> It is the only way of expressing the logic where it is clear that the
>> lower pointer dereferences are trivially safe.
> Well, yes and no - the other uses of course should then also use
> paging_get_hostmode(), like various of the wrappers in paging.h
> do. Or else I question why we have paging_get_hostmode() in the
> first place.

I'm not convinced it is an appropriate abstraction to have, and I don't
expect it to survive the nested virt work.

> There are more examples in shadow code where this
> gets open-coded when it probably shouldn't be. There haven't been
> any such cases in HAP code so far ...

Doesn't matter.  Its use here would obfuscate the code (this is one part
of why I think it is a bad abstraction to begin with), and if the
implementation ever changed, the function would lose its safety.

> Additionally (noticing only now) in the shadow case you may now
> loop over all vCPU-s in shadow_teardown() just for
> shadow_vcpu_teardown() to bail right away. Wouldn't it make sense
> to retain the "if ( shadow_mode_enabled(d) )" there around the
> loop?

I'm not entirely convinced that was necessarily safe.  Irrespective, see
the TODO.  The foreach_vcpu() is only a stopgap until some cleanup
structure changes come along (which I had queued behind this patch anyway).

~Andrew

Re: [PATCH] xen/x86: Fix memory leak in vcpu_create() error path

[Jan Beulich]

On 18.12.2020 14:58, Andrew Cooper wrote:
> On 18/12/2020 08:27, Jan Beulich wrote:
>> On 17.12.2020 22:46, Andrew Cooper wrote:
>>> On 29/09/2020 07:18, Jan Beulich wrote:
>>>> On 28.09.2020 17:47, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/mm/hap/hap.c
>>>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>>>> @@ -563,30 +563,37 @@ void hap_final_teardown(struct domain *d)
>>>>>      paging_unlock(d);
>>>>>  }
>>>>>  
>>>>> +void hap_vcpu_teardown(struct vcpu *v)
>>>>> +{
>>>>> +    struct domain *d = v->domain;
>>>>> +    mfn_t mfn;
>>>>> +
>>>>> +    paging_lock(d);
>>>>> +
>>>>> +    if ( !paging_mode_hap(d) || !v->arch.paging.mode )
>>>>> +        goto out;
>>>> Any particular reason you don't use paging_get_hostmode() (as the
>>>> original code did) here? Any particular reason for the seemingly
>>>> redundant (and hence somewhat in conflict with the description's
>>>> "with the minimum number of safety checks possible")
>>>> paging_mode_hap()?
>>> Yes to both.  As you spotted, I converted the shadow side first, and
>>> made the two consistent.
>>>
>>> The paging_mode_{shadow,hap})() is necessary for idempotency.  These
>>> functions really might get called before paging is set up, for an early
>>> failure in domain_create().
>> In which case how would v->arch.paging.mode be non-NULL already?
>> They get set in {hap,shadow}_vcpu_init() only.
> 
> Right, but we also might end up here with an error early in
> vcpu_create(), where d->arch.paging is set up, but v->arch.paging isn't.
> 
> This logic needs to be safe to use at any point of partial initialisation.
> 
> (And to be clear, I found I needed both of these based on some
> artificial error injection testing.)
> 
>>> The paging mode has nothing really to do with hostmode/guestmode/etc. 
>>> It is the only way of expressing the logic where it is clear that the
>>> lower pointer dereferences are trivially safe.
>> Well, yes and no - the other uses of course should then also use
>> paging_get_hostmode(), like various of the wrappers in paging.h
>> do. Or else I question why we have paging_get_hostmode() in the
>> first place.
> 
> I'm not convinced it is an appropriate abstraction to have, and I don't
> expect it to survive the nested virt work.
> 
>> There are more examples in shadow code where this
>> gets open-coded when it probably shouldn't be. There haven't been
>> any such cases in HAP code so far ...
> 
> Doesn't matter.  Its use here would obfuscate the code (this is one part
> of why I think it is a bad abstraction to begin with), and if the
> implementation ever changed, the function would lose its safety.
> 
>> Additionally (noticing only now) in the shadow case you may now
>> loop over all vCPU-s in shadow_teardown() just for
>> shadow_vcpu_teardown() to bail right away. Wouldn't it make sense
>> to retain the "if ( shadow_mode_enabled(d) )" there around the
>> loop?
> 
> I'm not entirely convinced that was necessarily safe.  Irrespective, see
> the TODO.  The foreach_vcpu() is only a stopgap until some cleanup
> structure changes come along (which I had queued behind this patch anyway).

Well, fair enough (for all of the points). You have my R-b already,
and all you need to do (if you haven't already) is re-base the
change, as the conflicting one of mine (which was triggered by
reviewing yours) has gone in already.

Jan