Re: [PATCH v3] xen: move per-cpu area management into common code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.09.2024 18:54, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/percpu.h
> @@ -0,0 +1,14 @@
> +#ifndef __X86_PERCPU_H__
> +#define __X86_PERCPU_H__
> +
> +#define PARK_OFFLINE_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)
> +
> +#endif /* __X86_PERCPU_H__ */

With this file appearing, doesn't arch/x86/include/asm/Makefile want the
respective line removed?

> --- /dev/null
> +++ b/xen/common/percpu.c
> @@ -0,0 +1,113 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

GPL-2.0-only

> +#include <xen/percpu.h>
> +#include <xen/cpu.h>
> +#include <xen/init.h>
> +#include <xen/mm.h>
> +#include <xen/rcupdate.h>
> +
> +#ifndef INVALID_PERCPU_AREA
> +#define INVALID_PERCPU_AREA (-(long)__per_cpu_start)
> +#endif
> +
> +#define PERCPU_ORDER get_order_from_bytes(__per_cpu_data_end - 
> __per_cpu_start)
> +
> +extern char __per_cpu_start[];
> +extern const char __per_cpu_data_end[];
> +
> +unsigned long __per_cpu_offset[NR_CPUS];

Could this perhaps become __read_mostly while it's being moved here?

> +void __init percpu_init_areas(void)
> +{
> +    unsigned int cpu;
> +
> +    for ( cpu = 1; cpu < NR_CPUS; cpu++ )
> +        __per_cpu_offset[cpu] = INVALID_PERCPU_AREA;
> +}
> +
> +static int init_percpu_area(unsigned int cpu)
> +{
> +    char *p;
> +
> +    if ( __per_cpu_offset[cpu] != INVALID_PERCPU_AREA )
> +        return park_offline_cpus ? 0 : -EBUSY;
> +
> +    if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL )
> +        return -ENOMEM;
> +
> +    memset(p, 0, __per_cpu_data_end - __per_cpu_start);
> +    __per_cpu_offset[cpu] = p - __per_cpu_start;
> +
> +    return 0;
> +}
> +
> +struct free_info {
> +    unsigned int cpu;
> +    struct rcu_head rcu;
> +};
> +static DEFINE_PER_CPU(struct free_info, free_info);
> +
> +static void cf_check _free_percpu_area(struct rcu_head *head)
> +{
> +    struct free_info *info = container_of(head, struct free_info, rcu);
> +    unsigned int cpu = info->cpu;
> +    char *p = __per_cpu_start + __per_cpu_offset[cpu];
> +
> +    free_xenheap_pages(p, PERCPU_ORDER);

It's quite sad that just because of this __per_cpu_start[] can be const-ified.

> --- a/xen/include/xen/percpu.h
> +++ b/xen/include/xen/percpu.h
> @@ -29,6 +29,36 @@
>  
>  #include <asm/percpu.h>
>  
> +#ifndef __ASSEMBLY__
> +
> +#include <xen/types.h>
> +#include <asm/current.h>
> +
> +#ifndef PARK_OFFLINE_CPUS
> +/*
> + * Do we, for platform reasons, need to actually keep CPUs online when we
> + * would otherwise prefer them to be off?
> + */
> +#define park_offline_cpus false
> +#endif

In the (implicit) #else case the identifier is a variable, which may end up
being set to true or false. Therefore I consider PARK_OFFLINE_CPUS somewhat
misleading: x86, #define-ing the variable, doesn't always mean to park CPUs.
Perhaps MAYBE_PARK_OFFLINE_CPUS or PARK_OFFLINE_CPUS_VAR?

Jan