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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

>> diff --git a/xen/common/percpu.c b/xen/common/percpu.c
>> new file mode 100644
>> index 0000000000..3837ef5714
>> --- /dev/null
>> +++ b/xen/common/percpu.c
>> @@ -0,0 +1,127 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#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];

unsigned long __per_cpu_offset[NR_CPUS] = {
    [0 ... NR_CPUS - 1] = INVALID_PERCPU_AREA,
};

should work, removing the need for percpu_init_areas() and avoids a
window during boot where all CPUs "share" a percpu area.

~Andrew