Re: [PATCH v1 2/2] xen: move per-cpu area management into common code

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/09/2024 10:26 am, oleksii.kurochko@xxxxxxxxx wrote:
> On Sun, 2024-09-22 at 10:43 +0200, Andrew Cooper wrote:
>> On 22/09/2024 10:23 am, Julien Grall wrote:
>>> On 19/09/2024 17:59, Oleksii Kurochko wrote:
>>>> diff --git a/xen/arch/x86/percpu.c b/xen/arch/x86/percpu.c
>>>> index 3205eacea6..33bded8cac 100644
>>>> --- a/xen/arch/x86/percpu.c
>>>> +++ b/xen/arch/x86/percpu.c
>>>> @@ -1,79 +1,19 @@
>>>> -#include <xen/percpu.h>
>>>>   #include <xen/cpu.h>
>>>> -#include <xen/init.h>
>>>> -#include <xen/mm.h>
>>>> -#include <xen/rcupdate.h>
>>>> -
>>>> -unsigned long __per_cpu_offset[NR_CPUS];
>>>> -
>>>> -/*
>>>> - * Force uses of per_cpu() with an invalid area to attempt to
>>>> access
>>>> the
>>>> - * middle of the non-canonical address space resulting in a #GP,
>>>> rather than a
>>>> - * possible #PF at (NULL + a little) which has security
>>>> implications
>>>> in the
>>>> - * context of PV guests.
>>>> - */
>>>> -#define INVALID_PERCPU_AREA (0x8000000000000000UL - (unsigned
>>>> long)__per_cpu_start)
>>>> -#define PERCPU_ORDER get_order_from_bytes(__per_cpu_data_end -
>>>> __per_cpu_start)
>>>> -
>>>> -void __init percpu_init_areas(void)
>>>> -{
>>>> -    unsigned int cpu;
>>>> -
>>>> -    for ( cpu = 1; cpu < NR_CPUS; cpu++ )
>>>> -        __per_cpu_offset[cpu] = INVALID_PERCPU_AREA;
>>>> -}
>>>> +#include <xen/percpu.h>
>>>> +#include <xen/smp.h>
>>>>   -static int init_percpu_area(unsigned int cpu)
>>>> +int arch_percpu_area_init_status(void)
>>> I understand that Arm and x86 are returning a different value
>>> today.
>>> But now that we are provising a common implementation, I think we
>>> need
>>> to explain the difference. This is probably a question for the x86
>>> folks.
>> The declarations for CPU Parking (variable, or compile time false)
>> wants
>> to be in the new common header, at which point
>> arch_percpu_area_init_status() doesn't need to exist.
>>
>> That also makes it very clear that there's a difference in return
>> value
>> based on CPU Parking (and the comment beside the variable explains
>> this
>> is about not quite offlining CPUs), which is far clearer than some
>> arch
>> function.
> Thanks for suggestion. It would be better, I had also concerns about
> arch_percpu_area_init_status but couldn't come up with better thing.
>
> Just to make sure I understand correctly—are you saying that I can use
> park_offline_cpus instead of arch_percpu_area_init_status()?
>    diff --git a/xen/common/percpu.c b/xen/common/percpu.c
>    index 3837ef5714..f997418586 100644
>    --- a/xen/common/percpu.c
>    +++ b/xen/common/percpu.c
>    @@ -51,7 +51,7 @@ static int init_percpu_area(unsigned int cpu)
>         char *p;
>     
>         if ( __per_cpu_offset[cpu] != INVALID_PERCPU_AREA )
>    -        return arch_percpu_area_init_status();
>    +        return park_offline_cpus ? 0 : -EBUSY;
>     
>         if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL )
>             return -ENOMEM;

Yes, that's exactly what I had in mind.

~Andrew