|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 02/13] xen/arm: gic-v2: Implement GIC suspend/resume functions
Hi Julien,
Thank you for the review.
On Sat, Sep 13, 2025 at 2:30 AM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi Mykola,
>
> On 01/09/2025 23:10, Mykola Kvach wrote:
> > From: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> >
> > System suspend may lead to a state where GIC would be powered down.
> > Therefore, Xen should save/restore the context of GIC on suspend/resume.
> >
> > Note that the context consists of states of registers which are
> > controlled by the hypervisor. Other GIC registers which are accessible
> > by guests are saved/restored on context switch.
> >
> > Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> > Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xxxxxxxxxx>
> > Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> > Changes in v6:
> > - drop extra func/line printing from dprintk
> > - drop checking context allocation from resume handler
> > - merge some loops where it is possible
> >
> > Changes in v4:
> > - Add error logging for allocation failures
> >
> > Changes in v3:
> > - Drop asserts and return error codes instead.
> > - Wrap code with CONFIG_SYSTEM_SUSPEND.
> >
> > Changes in v2:
> > - Minor fixes after review.
> > ---
> > xen/arch/arm/gic-v2.c | 143 +++++++++++++++++++++++++++++++++
> > xen/arch/arm/gic.c | 29 +++++++
> > xen/arch/arm/include/asm/gic.h | 12 +++
> > 3 files changed, 184 insertions(+)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index b23e72a3d0..6373599e69 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -1098,6 +1098,140 @@ static int gicv2_iomem_deny_access(struct domain *d)
> > return iomem_deny_access(d, mfn, mfn + nr);
> > }
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +
> > +/* GICv2 registers to be saved/restored on system suspend/resume */
> > +struct gicv2_context {
> > + /* GICC context */
> > + uint32_t gicc_ctlr;
> > + uint32_t gicc_pmr;
> > + uint32_t gicc_bpr;
> > + /* GICD context */
> > + uint32_t gicd_ctlr;
>
> I don't quite follow why all the registers above needs to be
> saved/restored. Is it just convenience because it is too complicated to
> recreate the value?
Do you mean reinitializing them with the same values as in the init path?
My reasoning for saving/restoring is to avoid duplicating assumptions from
initialization in the resume code. If the init sequence changes in the
future, or if some registers are modified outside of init, the resume path
would also need to be updated. Saving/restoring directly feels like a more
universal and robust approach.
>
> > + uint32_t *gicd_isenabler;
> > + uint32_t *gicd_isactiver;
> > + uint32_t *gicd_ipriorityr;
> > + uint32_t *gicd_itargetsr;
> > + uint32_t *gicd_icfgr;
> > +};> +
> > +static struct gicv2_context gicv2_context;
> > +
> > +static int gicv2_suspend(void)
> > +{
> > + unsigned int i;
> > +
> > + if ( !gicv2_context.gicd_isenabler )
> > + {
> > + dprintk(XENLOG_WARNING, "GICv2 suspend context not allocated!\n");
> > + return -ENOMEM;
> > + }
> > +
> > + /* Save GICC configuration */
> > + gicv2_context.gicc_ctlr = readl_gicc(GICC_CTLR);
> > + gicv2_context.gicc_pmr = readl_gicc(GICC_PMR);
> > + gicv2_context.gicc_bpr = readl_gicc(GICC_BPR);
> > +
> > + /* Save GICD configuration */
> > + gicv2_context.gicd_ctlr = readl_gicd(GICD_CTLR);
> > +
> > + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 32); i++ )
> > + {
> > + gicv2_context.gicd_isenabler[i] = readl_gicd(GICD_ISENABLER + i *
> > 4);
> > + gicv2_context.gicd_isactiver[i] = readl_gicd(GICD_ISACTIVER + i *
> > 4);
> > + }
> > +
> > + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 4); i++ )
> > + {
> > + gicv2_context.gicd_ipriorityr[i] = readl_gicd(GICD_IPRIORITYR + i
> > * 4);
> > + gicv2_context.gicd_itargetsr[i] = readl_gicd(GICD_ITARGETSR + i *
> > 4);
> > + }
> > +
> > + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 16); i++ )
> > + gicv2_context.gicd_icfgr[i] = readl_gicd(GICD_ICFGR + i * 4);
> > +
> > + return 0;
> > +}
> > +
> > +static void gicv2_resume(void)
> > +{
> > + unsigned int i;
> > +
> > + gicv2_cpu_disable();> + /* Disable distributor */
> > + writel_gicd(0, GICD_CTLR);
> > +
> > + /* Restore GICD configuration */
> > + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 32); i++ )
> > + {
> > + writel_gicd(0xffffffff, GICD_ICENABLER + i * 4);
> > + writel_gicd(gicv2_context.gicd_isenabler[i], GICD_ISENABLER + i *
> > 4);
> > +
> > + writel_gicd(0xffffffff, GICD_ICACTIVER + i * 4);
> > + writel_gicd(gicv2_context.gicd_isactiver[i], GICD_ISACTIVER + i *
> > 4);
> > + }
> > +
> > + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 4); i++ )
> > + {
> > + writel_gicd(gicv2_context.gicd_ipriorityr[i], GICD_IPRIORITYR + i
> > * 4);
> > + writel_gicd(gicv2_context.gicd_itargetsr[i], GICD_ITARGETSR + i *
> > 4);
> > + }
> > +
> > + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 16); i++ )
> > + writel_gicd(gicv2_context.gicd_icfgr[i], GICD_ICFGR + i * 4);
> > +
> > + /* Make sure all registers are restored and enable distributor */
> > + writel_gicd(gicv2_context.gicd_ctlr | GICD_CTL_ENABLE, GICD_CTLR);
>
> Why are we forcing CTL_ENABLE? Surely it should have been set and if
> not, then why is it fine to override it?
You are right — forcing GICD_CTL_ENABLE is unnecessary here.
The value of GICD_CTLR was already saved before suspend, so restoring
it as-is should be sufficient.
I will drop the | GICD_CTL_ENABLE and just restore the saved value.
>
> > +
> > + /* Restore GIC CPU interface configuration */
> > + writel_gicc(gicv2_context.gicc_pmr, GICC_PMR);
> > + writel_gicc(gicv2_context.gicc_bpr, GICC_BPR);
> > +
> > + /* Enable GIC CPU interface */
> > + writel_gicc(gicv2_context.gicc_ctlr | GICC_CTL_ENABLE | GICC_CTL_EOI,
> > + GICC_CTLR);
>
> Same question here for both ENABLE and EOI.
You are right here as well — we don’t need to force GICC_CTL_ENABLE
or GICC_CTL_EOI. The saved GICC_CTLR value should already reflect
the correct state at the time of suspend.
So it would be cleaner to just restore the saved register value
directly, without OR’ing additional bits.
>
> > +}
> > +
> > +static void gicv2_alloc_context(struct gicv2_context *gc)
>
> I am a bit surprised this is not returning an error? Why is it ok to
> ignore the error and continue? At least for now, if someone enable
> CONFIG_SYSTEM_SUSPEND, they would likely want the feature. So it would
> be better to crash early.
This behavior was introduced based on feedback on one of the earlier
versions of the patch series.
I agree with your point — if CONFIG_SYSTEM_SUSPEND is enabled, then
failing to allocate the context should be treated as fatal. I will
update the code to crash early in this case.
>
> > +{
> > + uint32_t n = gicv2_info.nr_lines;
> > +
> > + gc->gicd_isenabler = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 32));
> > + if ( !gc->gicd_isenabler )
> > + goto err_free;
> > +
> > + gc->gicd_isactiver = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 32));
> > + if ( !gc->gicd_isactiver )
> > + goto err_free;
> > +
> > + gc->gicd_itargetsr = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 4));
> > + if ( !gc->gicd_itargetsr )
> > + goto err_free;
> > +
> > + gc->gicd_ipriorityr = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 4));
> > + if ( !gc->gicd_ipriorityr )
> > + goto err_free;
> > +
> > + gc->gicd_icfgr = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 16));
> > + if ( !gc->gicd_icfgr )
> > + goto err_free;
>
> I am wondering if we are really saving that much by allocating each
> array separately? It would simply the code if we fix the array to
> support up to 1024 interrupts so we allocate a single structure.
I suppose some systems may have only local interrupts, or a very small
number of SPIs, which is allowed by the spec.
We could rewrite the code to use a single allocation for all arrays, or
possibly avoid dynamic allocation entirely and declare the arrays of
structs in global scope. The latter approach would simplify the code
and reduce the number of allocations, but it would use memory less
efficiently.
>
> > +> + return;
> > +
> > + err_free:
> > + printk(XENLOG_ERR "Failed to allocate memory for GICv2 suspend
> > context\n");
> > +> + xfree(gc->gicd_icfgr);
> > + xfree(gc->gicd_ipriorityr);
> > + xfree(gc->gicd_itargetsr);
> > + xfree(gc->gicd_isactiver);
> > + xfree(gc->gicd_isenabler);
>
> NIT: If you use XFREE(), then you don't need the memset below.
Ack.
>
> > +
> > + memset(gc, 0, sizeof(*gc));
> > +}
> > +
> > +#endif /* CONFIG_SYSTEM_SUSPEND */
> > +
> > #ifdef CONFIG_ACPI
> > static unsigned long gicv2_get_hwdom_extra_madt_size(const struct domain
> > *d)
> > {
> > @@ -1302,6 +1436,11 @@ static int __init gicv2_init(void)
> >
> > spin_unlock(&gicv2.lock);
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > + /* Allocate memory to be used for saving GIC context during the
> > suspend */
> > + gicv2_alloc_context(&gicv2_context);
> > +#endif /* CONFIG_SYSTEM_SUSPEND */
> > +
> > return 0;
> > }
> >
> > @@ -1345,6 +1484,10 @@ static const struct gic_hw_operations gicv2_ops = {
> > .map_hwdom_extra_mappings = gicv2_map_hwdom_extra_mappings,
> > .iomem_deny_access = gicv2_iomem_deny_access,
> > .do_LPI = gicv2_do_LPI,
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > + .suspend = gicv2_suspend,
> > + .resume = gicv2_resume,
> > +#endif /* CONFIG_SYSTEM_SUSPEND */
> > };
> >
> > /* Set up the GIC */
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index e80fe0ca24..a018bd7715 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -425,6 +425,35 @@ int gic_iomem_deny_access(struct domain *d)
> > return gic_hw_ops->iomem_deny_access(d);
> > }
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +
> > +int gic_suspend(void)
> > +{
> > + /* Must be called by boot CPU#0 with interrupts disabled */
>
> What would prevent us to suspend from another CPU?
Nothing prevents suspend from being called on another CPU.
According to the PSCI specification, it just needs to be the last
running CPU in the system.
>
> > + ASSERT(!local_irq_is_enabled());
> > + ASSERT(!smp_processor_id());
> > +
> > + if ( !gic_hw_ops->suspend || !gic_hw_ops->resume )
> > + return -ENOSYS;
> > +
> > + return gic_hw_ops->suspend();
> > +}
> > +
> > +void gic_resume(void)
> > +{
> > + /*
> > + * Must be called by boot CPU#0 with interrupts disabled after
> > gic_suspend
> > + * has returned successfully.
> > + */
> > + ASSERT(!local_irq_is_enabled());
> > + ASSERT(!smp_processor_id());
> > + ASSERT(gic_hw_ops->resume);
> > +
> > + gic_hw_ops->resume();
> > +}
> > +
> > +#endif /* CONFIG_SYSTEM_SUSPEND */
> > +
> > static int cpu_gic_callback(struct notifier_block *nfb,
> > unsigned long action,
> > void *hcpu)
> > diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
> > index 541f0eeb80..a706303008 100644
> > --- a/xen/arch/arm/include/asm/gic.h
> > +++ b/xen/arch/arm/include/asm/gic.h
> > @@ -280,6 +280,12 @@ extern int gicv_setup(struct domain *d);
> > extern void gic_save_state(struct vcpu *v);
> > extern void gic_restore_state(struct vcpu *v);
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +/* Suspend/resume */
> > +extern int gic_suspend(void);
> > +extern void gic_resume(void);
> > +#endif /* CONFIG_SYSTEM_SUSPEND */
> > +
> > /* SGI (AKA IPIs) */
> > enum gic_sgi {
> > GIC_SGI_EVENT_CHECK,
> > @@ -395,6 +401,12 @@ struct gic_hw_operations {
> > int (*iomem_deny_access)(struct domain *d);
> > /* Handle LPIs, which require special handling */
> > void (*do_LPI)(unsigned int lpi);
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > + /* Save GIC configuration due to the system suspend */
> > + int (*suspend)(void);
> > + /* Restore GIC configuration due to the system resume */
> > + void (*resume)(void);
> > +#endif /* CONFIG_SYSTEM_SUSPEND */
> > };
> >
> > extern const struct gic_hw_operations *gic_hw_ops;
>
> Cheers,
>
> --
> Julien Grall
>
Best regards,
Mykola
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |