Re: [PATCH v3 for 4.23] Add GIC SGI boot/self tests in Xen

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

It is usually preferred to send a new version in its own thread rather 
than in-reply-to an existing version.
On 28/08/2026 12:29, Ayan Kumar Halder wrote:
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index b7afd3e58c..71f177824b 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -24,6 +24,7 @@ obj-y += domctl.o
>   obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
>   obj-y += efi/
>   obj-y += gic.o
> +obj-$(CONFIG_BOOT_SELFTEST) += gic-test.o
>   obj-$(CONFIG_GICV2) += gic-v2.o
>   obj-$(CONFIG_GICV3) += gic-v3.o
>   obj-$(CONFIG_HAS_ITS) += gic-v3-its.o
> diff --git a/xen/arch/arm/gic-test.c b/xen/arch/arm/gic-test.c
> new file mode 100644
> index 0000000000..9ddd47cad2
> --- /dev/null
> +++ b/xen/arch/arm/gic-test.c
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
The preferred license for Xen is GPLv2-only (see COPYING). Can you 
confirm the use of GPL2+ is intended?
> +
> +#include <xen/atomic.h>
> +#include <xen/cpumask.h>
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/param.h>
> +#include <xen/percpu.h>
> +#include <xen/smp.h>
> +#include <xen/time.h>
> +#include <asm/gic.h>
> +#include <asm/processor.h>
> +#include <asm/setup.h>
> +
> +static bool __initdata opt_gic_test;
> +boolean_param("gic-test", opt_gic_test);
> +
> +static DEFINE_PER_CPU(unsigned int, sgi_test_count);
> +
> +void gic_sgi_test_interrupt(void)
> +{
> +    this_cpu(sgi_test_count)++;
In sgi_count(), you are using ACCESS_ONCE() to read the content of the 
variable, but I am not entirely sure this_cpu(...)++ is guarantee to be 
a single write.
As this happen on different CPU, don't we also need to use ACCESS_ONCE() 
here too or atomically increment?
[...]

> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 078049e741..6a132c64e1 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -330,6 +330,11 @@ static void do_static_sgi(struct cpu_user_regs *regs, enum 
> gic_sgi sgi)
>       case GIC_SGI_CALL_FUNCTION:
>           smp_call_function_interrupt();
>           break;
> +#ifdef CONFIG_BOOT_SELFTEST
> +    case GIC_SGI_TEST:
> +        gic_sgi_test_interrupt();
> +        break;
> +#endif
>       default:
>           panic("Unhandled SGI %d on CPU%d\n", sgi, smp_processor_id());
>           break;
> diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
> index ee2c26adb4..40635a9d32 100644
> --- a/xen/arch/arm/include/asm/gic.h
> +++ b/xen/arch/arm/include/asm/gic.h
> @@ -306,6 +306,9 @@ enum gic_sgi {
>       GIC_SGI_EVENT_CHECK,
>       GIC_SGI_DUMP_STATE,
>       GIC_SGI_CALL_FUNCTION,
> +#ifdef CONFIG_BOOT_SELFTEST
> +    GIC_SGI_TEST,
> +#endif
>       GIC_SGI_STATIC_MAX,
>   };
>   
> @@ -321,6 +324,11 @@ extern void send_SGI_one(unsigned int cpu, enum gic_sgi sgi);
>   extern void send_SGI_self(enum gic_sgi sgi);
>   extern void send_SGI_allbutself(enum gic_sgi sgi);
>   
> +#ifdef CONFIG_BOOT_SELFTEST
> +/* Record a GIC_SGI_TEST delivered to this CPU (see arch/arm/gic-test.c). */
I would suggest to remove (see ...). One can easily find 
gic_sgi_test_interrupt() and this reduces the risk of stale file name.
> +void gic_sgi_test_interrupt(void);
> +#endif
> +
>   /* print useful debug info */
>   extern void gic_dump_info(struct vcpu *v);
>   extern void gic_dump_vgic_info(struct vcpu *v);
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 0adfa4993a..2fdf5da526 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -50,6 +50,15 @@ void setup_mm(void);
>   extern uint32_t hyp_traps_vector[];
>   void init_traps(void);
>   
> +#ifdef CONFIG_BOOT_SELFTEST
> +#define __initcallboottest(fn) \
> +    static const initcall_t __initcall_##fn __init_call("boottest") = (fn)
> +
> +void do_init_boottests(void);
> +#else
> +static inline void do_init_boottests(void) {}
> +#endif
> +
>   int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t 
> p2mt,
>                     struct rangeset *iomem_ranges, struct rangeset *irq_ranges);
>   
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 6310a47d68..c7abbdb04e 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -83,6 +83,24 @@ static void __init init_idle_domain(void)
>       /* TODO: setup_idle_pagetable(); */
>   }
>   
> +#ifdef CONFIG_BOOT_SELFTEST
> +extern const initcall_t __initcall_boot_test_start[],
> +    __initcall_boot_test_end[];
> +
> +void do_init_boottests(void)
> +{
> +    const initcall_t *call;
> +
> +    printk("CPU%u: boot self-tests start\n", smp_processor_id());
> +
> +    for ( call = __initcall_boot_test_start; call < __initcall_boot_test_end;
> +          call++ )
> +        (*call)();
> +
> +    printk("CPU%u: boot self-tests done\n", smp_processor_id());
> +}
> +#endif /* CONFIG_BOOT_SELFTEST */
> +
>   static const char * __initdata processor_implementers[] = {
>       ['A'] = "ARM Limited",
>       ['B'] = "Broadcom Corporation",
> @@ -470,6 +488,8 @@ void asmlinkage __init noreturn start_xen(unsigned long 
> fdt_paddr)
>       enable_errata_workarounds();
>       enable_cpu_features();
>   
> +    do_init_boottests();
> +
>       /* Create initial domain 0. */
>       if ( !is_dom0less_mode() )
>           create_dom0();
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 1806c47a08..97d8b19cf4 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -28,6 +28,7 @@
>   #include <asm/gic.h>
>   #include <asm/procinfo.h>
>   #include <asm/psci.h>
> +#include <asm/setup.h>
Style: I think this wants to go after asm/tee/tee.h (acpi.h seems to be 
misplaced).
>   #include <asm/acpi.h>
>   #include <asm/tee/tee.h>
>   
> @@ -413,6 +414,8 @@ void asmlinkage noreturn start_secondary(void)
>   
>       printk(XENLOG_DEBUG "CPU %u booted.\n", smp_processor_id());
>   
> +    do_init_boottests();
> +
>       startup_cpu_idle_loop();
>   }
>   
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 2d5f1c516d..14f64a856c 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -146,6 +146,10 @@ SECTIONS
>          *(.initcall1.init)
>          __initcall_end = .;
>   
> +       __initcall_boot_test_start = .;
> +       *(.initcallboottest.init)
> +       __initcall_boot_test_end = .;
> +
>          . = ALIGN(4);
>          __alt_instructions = .;
>          *(.altinstructions)
Cheers,

--
Julien Grall