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

[oleksii.kurochko@xxxxxxxxx]

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;

~ Oleksii