[PATCH for-4.22] xen/arm: propagate secondary GIC initialization failures

[PATCH for-4.22] xen/arm: propagate secondary GIC initialization failures

[Mykola Kvach]

The GICv3 secondary_init() callback can fail while discovering or
waking a Redistributor, enabling LPIs, or setting up an ITS collection.
gic_init_secondary_cpu() currently discards that status. start_secondary()
then marks the CPU online even though its per-CPU GIC interface may be
unusable.

Return the callback status through the common GIC layer. Have
start_secondary() report the failure and stop the affected CPU before it
is added to cpu_online_map.

Fixes: bc183a0235e0 ("xen/arm: Add support for GIC v3")
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
 xen/arch/arm/gic.c             | 10 ++++++++--
 xen/arch/arm/include/asm/gic.h |  2 +-
 xen/arch/arm/smpboot.c         |  9 ++++++++-
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index ee75258fc3..078049e741 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -282,11 +282,17 @@ void smp_send_state_dump(unsigned int cpu)
 }
 
 /* Set up the per-CPU parts of the GIC for a secondary CPU */
-void gic_init_secondary_cpu(void)
+int gic_init_secondary_cpu(void)
 {
-    gic_hw_ops->secondary_init();
+    int rc = gic_hw_ops->secondary_init();
+
+    if ( rc )
+        return rc;
+
     /* Clear LR mask for secondary cpus */
     clear_cpu_lr_mask();
+
+    return 0;
 }
 
 /* Shut down the per-CPU GIC interface */
diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
index ff22dea40d..ee2c26adb4 100644
--- a/xen/arch/arm/include/asm/gic.h
+++ b/xen/arch/arm/include/asm/gic.h
@@ -291,7 +291,7 @@ extern void gic_preinit(void);
 /* Bring up the interrupt controller, and report # cpus attached */
 extern void gic_init(void);
 /* Bring up a secondary CPU's per-CPU GIC interface */
-extern void gic_init_secondary_cpu(void);
+extern int gic_init_secondary_cpu(void);
 /* Take down a CPU's per-CPU GIC interface */
 extern void gic_disable_cpu(void);
 /* setup the gic virtual interface for a guest */
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index ba5fd2dd52..5e23b0b6a9 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -319,6 +319,7 @@ smp_prepare_cpus(void)
 void asmlinkage noreturn start_secondary(void)
 {
     unsigned int cpuid = init_data.cpuid;
+    int rc;
 
     memset(get_cpu_info(), 0, sizeof (struct cpu_info));
 
@@ -373,7 +374,13 @@ void asmlinkage noreturn start_secondary(void)
      */
     update_system_features(&current_cpu_data);
 
-    gic_init_secondary_cpu();
+    rc = gic_init_secondary_cpu();
+    if ( rc )
+    {
+        printk(XENLOG_ERR "CPU%u: Failed to initialize the GIC: %d\n",
+               cpuid, rc);
+        stop_cpu();
+    }
 
     set_current(idle_vcpu[cpuid]);
 
-- 
2.43.0

Re: [PATCH for-4.22] xen/arm: propagate secondary GIC initialization failures

[Oleksii Kurochko]

On 7/10/26 12:20 PM, Mykola Kvach wrote:
> The GICv3 secondary_init() callback can fail while discovering or
> waking a Redistributor, enabling LPIs, or setting up an ITS collection.
> gic_init_secondary_cpu() currently discards that status. start_secondary()
> then marks the CPU online even though its per-CPU GIC interface may be
> unusable.
> 
> Return the callback status through the common GIC layer. Have
> start_secondary() report the failure and stop the affected CPU before it
> is added to cpu_online_map.
> 
> Fixes: bc183a0235e0 ("xen/arm: Add support for GIC v3")
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---

This doesn't look release-critical. IIUC, the issue only occurs if 
`gic_hw_ops->secondary_init()` fails, which itself appears to require an 
abnormal condition (e.g. invalid DT data or a failure while initializing 
the GIC Redistributor/LPI/ITS state). On correctly configured systems, 
this path should never be taken. We've also been living with this 
behavior for a long time, so unless the maintainers feel strongly that 
it should go in now, I think we could postpone this patch until the 4.22 
release.

Thanks.

~ Oleksii

Re: [PATCH for-4.22] xen/arm: propagate secondary GIC initialization failures

[Orzel, Michal]

On 10-Jul-26 12:20, Mykola Kvach wrote:
> The GICv3 secondary_init() callback can fail while discovering or
> waking a Redistributor, enabling LPIs, or setting up an ITS collection.
> gic_init_secondary_cpu() currently discards that status. start_secondary()
> then marks the CPU online even though its per-CPU GIC interface may be
> unusable.
> 
> Return the callback status through the common GIC layer. Have
> start_secondary() report the failure and stop the affected CPU before it
> is added to cpu_online_map.
> 
> Fixes: bc183a0235e0 ("xen/arm: Add support for GIC v3")
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---
>  xen/arch/arm/gic.c             | 10 ++++++++--
>  xen/arch/arm/include/asm/gic.h |  2 +-
>  xen/arch/arm/smpboot.c         |  9 ++++++++-
>  3 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index ee75258fc3..078049e741 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -282,11 +282,17 @@ void smp_send_state_dump(unsigned int cpu)
>  }
>  
>  /* Set up the per-CPU parts of the GIC for a secondary CPU */
> -void gic_init_secondary_cpu(void)
> +int gic_init_secondary_cpu(void)
>  {
> -    gic_hw_ops->secondary_init();
> +    int rc = gic_hw_ops->secondary_init();
> +
> +    if ( rc )
> +        return rc;
> +
>      /* Clear LR mask for secondary cpus */
>      clear_cpu_lr_mask();
> +
> +    return 0;
>  }
>  
>  /* Shut down the per-CPU GIC interface */
> diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
> index ff22dea40d..ee2c26adb4 100644
> --- a/xen/arch/arm/include/asm/gic.h
> +++ b/xen/arch/arm/include/asm/gic.h
> @@ -291,7 +291,7 @@ extern void gic_preinit(void);
>  /* Bring up the interrupt controller, and report # cpus attached */
>  extern void gic_init(void);
>  /* Bring up a secondary CPU's per-CPU GIC interface */
> -extern void gic_init_secondary_cpu(void);
> +extern int gic_init_secondary_cpu(void);
>  /* Take down a CPU's per-CPU GIC interface */
>  extern void gic_disable_cpu(void);
>  /* setup the gic virtual interface for a guest */
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index ba5fd2dd52..5e23b0b6a9 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -319,6 +319,7 @@ smp_prepare_cpus(void)
>  void asmlinkage noreturn start_secondary(void)
>  {
>      unsigned int cpuid = init_data.cpuid;
> +    int rc;
>  
>      memset(get_cpu_info(), 0, sizeof (struct cpu_info));
>  
> @@ -373,7 +374,13 @@ void asmlinkage noreturn start_secondary(void)
>       */
>      update_system_features(&current_cpu_data);
>  
> -    gic_init_secondary_cpu();
> +    rc = gic_init_secondary_cpu();
> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR "CPU%u: Failed to initialize the GIC: %d\n",
> +               cpuid, rc);
NIT: While functionally identical, for consistency with the neighbouring
messages you may want to use smp_processor_id() here instead of cpuid. I can
change on commit.

Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH for-4.22] xen/arm: propagate secondary GIC initialization failures

[Orzel, Michal]

On 22-Jul-26 13:04, Orzel, Michal wrote:
> 
> 
> On 10-Jul-26 12:20, Mykola Kvach wrote:
>> The GICv3 secondary_init() callback can fail while discovering or
>> waking a Redistributor, enabling LPIs, or setting up an ITS collection.
>> gic_init_secondary_cpu() currently discards that status. start_secondary()
>> then marks the CPU online even though its per-CPU GIC interface may be
>> unusable.
>>
>> Return the callback status through the common GIC layer. Have
>> start_secondary() report the failure and stop the affected CPU before it
>> is added to cpu_online_map.
>>
>> Fixes: bc183a0235e0 ("xen/arm: Add support for GIC v3")
>> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
>> ---
>>  xen/arch/arm/gic.c             | 10 ++++++++--
>>  xen/arch/arm/include/asm/gic.h |  2 +-
>>  xen/arch/arm/smpboot.c         |  9 ++++++++-
>>  3 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index ee75258fc3..078049e741 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -282,11 +282,17 @@ void smp_send_state_dump(unsigned int cpu)
>>  }
>>  
>>  /* Set up the per-CPU parts of the GIC for a secondary CPU */
>> -void gic_init_secondary_cpu(void)
>> +int gic_init_secondary_cpu(void)
>>  {
>> -    gic_hw_ops->secondary_init();
>> +    int rc = gic_hw_ops->secondary_init();
>> +
>> +    if ( rc )
>> +        return rc;
>> +
>>      /* Clear LR mask for secondary cpus */
>>      clear_cpu_lr_mask();
>> +
>> +    return 0;
>>  }
>>  
>>  /* Shut down the per-CPU GIC interface */
>> diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
>> index ff22dea40d..ee2c26adb4 100644
>> --- a/xen/arch/arm/include/asm/gic.h
>> +++ b/xen/arch/arm/include/asm/gic.h
>> @@ -291,7 +291,7 @@ extern void gic_preinit(void);
>>  /* Bring up the interrupt controller, and report # cpus attached */
>>  extern void gic_init(void);
>>  /* Bring up a secondary CPU's per-CPU GIC interface */
>> -extern void gic_init_secondary_cpu(void);
>> +extern int gic_init_secondary_cpu(void);
>>  /* Take down a CPU's per-CPU GIC interface */
>>  extern void gic_disable_cpu(void);
>>  /* setup the gic virtual interface for a guest */
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index ba5fd2dd52..5e23b0b6a9 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -319,6 +319,7 @@ smp_prepare_cpus(void)
>>  void asmlinkage noreturn start_secondary(void)
>>  {
>>      unsigned int cpuid = init_data.cpuid;
>> +    int rc;
>>  
>>      memset(get_cpu_info(), 0, sizeof (struct cpu_info));
>>  
>> @@ -373,7 +374,13 @@ void asmlinkage noreturn start_secondary(void)
>>       */
>>      update_system_features(&current_cpu_data);
>>  
>> -    gic_init_secondary_cpu();
>> +    rc = gic_init_secondary_cpu();
>> +    if ( rc )
>> +    {
>> +        printk(XENLOG_ERR "CPU%u: Failed to initialize the GIC: %d\n",
>> +               cpuid, rc);
> NIT: While functionally identical, for consistency with the neighbouring
> messages you may want to use smp_processor_id() here instead of cpuid. I can
> change on commit.
> 
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
I was about to commit this patch when I realized that it should be moved above
update_system_features() as the comment above it suggests i.e. we should update
system features only if we do not stop the core. Please send a v2 with remarks
addressed.

~Michal