Re: [Xen-devel] [PATCH v1 1/2] x86/cpu: maintain a parked CPU bitmap

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.11.2019 10:47, Julien Grall wrote:
> On 20/11/2019 23:05, Chao Gao wrote:
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -39,6 +39,7 @@
>>   cpumask_t cpu_online_map;
>>   cpumask_t cpu_present_map;
>>   cpumask_t cpu_possible_map;
>> +cpumask_var_t cpu_parked_map;
> 
> You define cpu_parked_map but AFAIK it will never get allocated. The 
> risk here is any access to that variable will result to a fault.
> 
> Looking at the changes below, it looks like access in common code will 
> be protected by park_offline_cpus. This is always false on Arm, so the 
> compiler should remove any access to cpu_parked_map.
> 
> With that in mind, I think it would be best to only provide a prototype 
> for cpu_parked_map and so the linker can warn if someone used it.

+1

In fact I wonder whether the maintenance of the map should live in
common code in the first place. While clearing the respective bit
in cpu_up() looks correct (and could be done without any if()),
I'm not convinced the setting of the bit in cpu_down() is going to
be correct in all cases. I'd rather leave this to the relevant
callers of cpu_down(). To deal with cpu_add_remove_lock not being
held perhaps it would be best to set the flag _before_ calling
cpu_down(); consumers of the map then would need to double check
that a CPU is not _also_ (still) online.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel