Re: [Xen-devel] [PATCH v2] xen/x86: clear per cpu stub page information in cpu_smpboot_free()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.01.2020 16:29, Jürgen Groß wrote:
> On 08.01.20 16:21, Jan Beulich wrote:
>> On 08.01.2020 15:34, Juergen Gross wrote:
>>> cpu_smpboot_free() removes the stubs for the cpu going offline, but it
>>> isn't clearing the related percpu variables. This will result in
>>> crashes when a stub page is released due to all related cpus gone
>>> offline and one of those cpus going online later.
>>>
>>> Fix that by clearing stubs.addr and stubs.mfn in order to allocate a
>>> new stub page when needed.
>>
>> I was really hoping for you to mention CPU parking here. How about
>>
>> "Fix that by clearing stubs.mfn (and also stubs.addr just to be on
>>   the safe side) in order to allocate a new stub page when needed,
>>   irrespective of whether the CPU gets parked or removed."
>>
>>> --- a/xen/arch/x86/smpboot.c
>>> +++ b/xen/arch/x86/smpboot.c
>>> @@ -945,6 +945,8 @@ static void cpu_smpboot_free(unsigned int cpu, bool 
>>> remove)
>>>                                (per_cpu(stubs.addr, cpu) | ~PAGE_MASK) + 1);
>>>           if ( i == STUBS_PER_PAGE )
>>>               free_domheap_page(mfn_to_page(mfn));
>>> +        per_cpu(stubs.addr, cpu) = 0;
>>> +        per_cpu(stubs.mfn, cpu) = 0;
>>
>> Looking more closely, I think I'd prefer these two lines (of which
>> the addr one isn't strictly needed anyway) to move ahead of the
>> if().
>>
>> If you agree, I'll be happy to do both while committing.
> 
> I agree.
> 
> I'm not sure the addr clearing can be omitted. This might result in
> problems when during onlining an early error happens in
> cpu_smpboot_alloc() and thus skipping the call of alloc_stub_page().
> The subsequent call of cpu_smpboot_free() will then overwrite mfn 0.

Oh, good point.

Jan

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