Re: [PATCH] xen: domain_update_node_affinity: Correct the ASSERT

[Julien Grall <julien.grall@linaro.org>]

On 07/25/2014 04:44 PM, Andrew Cooper wrote:
> On 25/07/14 16:30, Julien Grall wrote:
>> The commit "move domain to cpupool0 before destroying it" make Xen crashes
>> when a domain is destroyed with d->vcpus allocated but no VCPU initialized.
>>
>> Assertion '!cpumask_empty(dom_cpumask)' failed at domain.c:452
>> Xen call trace:
>>     [<00207bd8>] domain_update_node_affinity+0x10c/0x238 (PC)
>>     [<00000004>] 00000004 (LR)
>>     [<00226870>] sched_move_domain+0x3cc/0x42c
>>     [<0020925c>] domain_kill+0xc8/0x178
>>     [<00206a0c>] do_domctl+0xaac/0x15e4
>>     [<002529c0>] do_trap_hypervisor+0xc5c/0xf94
>>     [<002559f0>] return_from_trap+0/0x4
>>
>> Fix the ASSERT to check if d->vcpu is allocated and VCPU 0 is initialized.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> Cc: George Dunlap <george.dunlap@citrix.com>
>> Cc: Dario Faggioli <dario.faggioli@citrix.com>
>> Cc: Juergen Gross <jgross@suse.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: Tim Deegan <tim@xen.org>
>>
>> ---
>>     This patch should be backported to Xen 4.4
>> ---
>>  xen/common/domain.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index d7a84cf..188b769 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -449,7 +449,7 @@ void domain_update_node_affinity(struct domain *d)
>>          }
>>          /* Filter out non-online cpus */
>>          cpumask_and(dom_cpumask, dom_cpumask, online);
>> -        ASSERT(!cpumask_empty(dom_cpumask));
>> +        ASSERT( !d->vcpu || !d->vcpu[0] || !cpumask_empty(dom_cpumask));
> 
> You don't actually care for the vcpu pointers themselves.  You only care
> whether the domain has vcpus.
> 
> d->max_vcpus == 0 is a more appropriate check.

Actually no... max_vcpus has been set before Xen is allocating
alloc_vcpu (see DOMCTL_max_vpus in common common/domctl.c).

So if Xen fails to allocate VCPU0, the hypervisor will still crash when
the domain is killed.

Regards,

-- 
Julien Grall