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

[oleksii.kurochko@xxxxxxxxx]

On Mon, 2024-09-30 at 17:50 +0200, oleksii.kurochko@xxxxxxxxx wrote:
> 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
> ).
Actually there is the same removing in this version:
https://lore.kernel.org/xen-devel/183f0be3788bd281067d32d35d7aedfe07bf84ab.camel@xxxxxxxxx/T/#Z2e.:..:d52cd7cddb09c3b87bc4c66458f619dbf7ac214f.1727365499.git.oleksii.kurochko::40gmail.com:1xen:arch:x86:include:asm:Makefile

~ Oleksii
> 
> > 
> > > --- /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
>