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

[oleksii.kurochko@xxxxxxxxx]

On Wed, 2024-10-02 at 15:41 +0200, Michal Orzel wrote:
> 
> 
> On 30/09/2024 18:39, Oleksii Kurochko wrote:
> > 
> > 
> > Centralize per-cpu area management to reduce code duplication and
> > enhance maintainability across architectures.
> > 
> > The per-cpu area management code, which is largely common among
> > architectures, is moved to a shared implementation in
> > xen/common/percpu.c. This change includes:
> >  * Remove percpu.c from the X86 and Arm architectures.
> >  * For x86, define INVALID_PERCPU_AREAS and PARK_OFFLINE_CPUS_VAR.
> >  * Drop the declaration of __per_cpu_offset[] from stubs.c in
> >    PPC and RISC-V to facilitate the build of the common per-cpu
> > code.
> > 
> > No functional changes for x86.
> > 
> > For Arm add support of CPU_RESUME_FAILED, CPU_REMOVE and freeing of
> > percpu in the case when system_state != SYS_STATE_suspend.
> Behaviorwise there is no change for Arm given that none of these
> actions can be executed.
> That said, by looking at the code I realized that we never call
> CPU_REMOVE so it is effectively
> a dead code.
There is no change for now but what I mean by this message if one day,
for example, enable_nonboot_cpus() will be called and park_offline_cpus
will be enabled/implemented for Arm then CPU_RESUME_FAILED will be
handled differently in comparison with original Arm code. In original
Arm code it will do just break but for common code it will do
free_percpu_area().
And the similar for CPU_REMOVE if park_offline_cpus is enabled and
cpu_notifier_call_chain(..., CPU_REMOVE,...) will be called then the
behaviour of common code will be different from Arm code.

Do you think it would be better just drop this part of the commit
message?
Or would it be better to add:
  ...(what I wrote before), however, there is no change in behavior for
Arm at this time.

> 
> As for the change itself:
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> 
> with one question below ...
> 
> [...]
> 
> > +static int cf_check cpu_percpu_callback(
> > +    struct notifier_block *nfb, unsigned long action, void *hcpu)
> > +{
> > +    unsigned int cpu = (unsigned long)hcpu;
> > +    int rc = 0;
> > +
> > +    switch ( action )
> > +    {
> > +    case CPU_UP_PREPARE:
> > +        rc = init_percpu_area(cpu);
> > +        break;
> > +    case CPU_UP_CANCELED:
> > +    case CPU_DEAD:
> > +    case CPU_RESUME_FAILED:
> > +        if ( !park_offline_cpus && system_state !=
> > SYS_STATE_suspend )
> > +            free_percpu_area(cpu);
> > +        break;
> > +    case CPU_REMOVE:
> > +        if ( park_offline_cpus )
> > +            free_percpu_area(cpu);
> > +        break;
> Aren't we missing default statement here as per MISRA C 16.4?
> I think we only allow to drop it for enums.
Yes, you are right:
https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/people/olkur/xen/ECLAIR_normal/percpu-common-v4/X86_64/7975314011/PROJECT.ecd;/sources/xen/common/percpu.c.html#R5092_1

I will add then:
        default:
           break;

Thanks.

Best regards,
 Oleksii