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

[oleksii.kurochko@xxxxxxxxx]

On Mon, 2024-09-30 at 15:25 +0200, Jan Beulich wrote:
> 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?
For sure, it should be removed ( as it was done in previous patch
series:
https://lore.kernel.org/xen-devel/e573f9d46e7af0806381f6a41af00dc415bf87bb.1727185495.git.oleksii.kurochko@xxxxxxxxx/#Z31xen:arch:x86:include:asm:Makefile
).

> 
> > --- /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?
Sure, it makes sense to me. I'll add __read_mostly.

> 
> > +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?
IMO PARK_OFFLINE_CPUS_VAR describes better the occurrence of "#define
park_offlince_cpus false". I will stick to it in the next patch
version.

Thanks.

~ Oleksii