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

["Halder, Ayan Kumar" <ayankuma@xxxxxxx>]

On 04/09/2026 20:21, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
> It is usually preferred to send a new version in its own thread rather 
> than in-reply-to an existing version.
Apologies, this was my mistake (bad git send command).
> 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?
fixed
> > +
> > +#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
ACCESS_ONCE(this_cpu(sgi_test_count))++;

>  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).
fixed.

- Ayan