From: Mirela Simonovic <mirela.simonovic@aggios.com>
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.
Tested on Xilinx Ultrascale+ MPSoC with (and without) powering down
the GIC.
Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xilinx.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
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 | 154 +++++++++++++++++++++++++++++++++
xen/arch/arm/gic.c | 29 +++++++
xen/arch/arm/include/asm/gic.h | 12 +++
3 files changed, 195 insertions(+)
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index b23e72a3d0..dce8f5e924 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1098,6 +1098,151 @@ 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;
+ 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, "%s:%d: GICv2 suspend context not allocated!\n",
+ __func__, __LINE__);
+ 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);
+
+ for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 32); i++ )
+ 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);
+
+ for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 4); i++ )
+ 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;
+
+ if ( !gicv2_context.gicd_isenabler )
+ {
+ dprintk(XENLOG_WARNING, "%s:%d: GICv2 suspend context not allocated!\n",
+ __func__, __LINE__);
+ return;
+ }
+
+ 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);
+ }
+
+ for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 32); i++ ) {
+ 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);
+
+ for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 4); i++ )
+ 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);
+
+ /* 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);
+}
+
+static void gicv2_alloc_context(struct gicv2_context *gc)
+{
+ 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;
+
+ return;
+
+ err_free:
+ dprintk(XENLOG_ERR,
+ "%s:%d: failed to allocate memory for GICv2 suspend context\n",
+ __func__, __LINE__);
+
+ xfree(gc->gicd_icfgr);
+ xfree(gc->gicd_ipriorityr);
+ xfree(gc->gicd_itargetsr);
+ xfree(gc->gicd_isactiver);
+ xfree(gc->gicd_isenabler);
+
+ 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 +1447,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 +1495,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 */
+ 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;
--
2.48.1Hi Mykola,
Mykola Kvach <xakep.amatop@gmail.com> writes:
> From: Mirela Simonovic <mirela.simonovic@aggios.com>
>
> 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.
>
> Tested on Xilinx Ultrascale+ MPSoC with (and without) powering down
> the GIC.
I think you can drop this sentence, as I am pretty sure this patch
wasn't tested on Ultrascale+ for last five(?) years..
>
> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
> Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xilinx.com>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---
> 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 | 154 +++++++++++++++++++++++++++++++++
> xen/arch/arm/gic.c | 29 +++++++
> xen/arch/arm/include/asm/gic.h | 12 +++
> 3 files changed, 195 insertions(+)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index b23e72a3d0..dce8f5e924 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -1098,6 +1098,151 @@ 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;
> + 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, "%s:%d: GICv2 suspend context not allocated!\n",
> + __func__, __LINE__);
Should this be ASSERT(gicv2_context.gicd_isenabler) ? Or do we expect
that this can happen during normal runtime?
Also, you don't need to print __func__ and __LINE__ as dprintk does this
for you already:
#define dprintk(lvl, fmt, args...) \
printk(lvl "%s:%d: " fmt, __FILE__, __LINE__, ## args)
> + 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);
> +
> + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 32); i++ )
> + gicv2_context.gicd_isactiver[i] = readl_gicd(GICD_ISACTIVER + i * 4);
> +
I think you can merge this two loops
> + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 4); i++ )
> + gicv2_context.gicd_ipriorityr[i] = readl_gicd(GICD_IPRIORITYR + i * 4);
> +
> + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 4); i++ )
> + gicv2_context.gicd_itargetsr[i] = readl_gicd(GICD_ITARGETSR + i * 4);
> +
And this two as well
> + 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;
> +
> + if ( !gicv2_context.gicd_isenabler )
> + {
> + dprintk(XENLOG_WARNING, "%s:%d: GICv2 suspend context not allocated!\n",
> + __func__, __LINE__);
the same comment as for gicv2_suspend(). Actually, we should not reach
here in any case.
> + return;
> + }
> +
> + 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);
> + }
> +
> + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 32); i++ ) {
> + 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);
> +
> + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 4); i++ )
> + 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);
> +
> + /* 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);
> +}
> +
> +static void gicv2_alloc_context(struct gicv2_context *gc)
> +{
> + 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;
> +
> + return;
> +
> + err_free:
> + dprintk(XENLOG_ERR,
> + "%s:%d: failed to allocate memory for GICv2 suspend context\n",
> + __func__, __LINE__);
Okay, this partially answers to my previous question about memory
allocation. So it is possible to have active system without space for
GIC context... But this basically renders system un-suspendable. I think
this warrants non-debug print to warn user that suspend will be not
available.
[...]
--
WBR, Volodymyr
Hi Volodymyr,
On Sat, Aug 23, 2025 at 3:01 AM Volodymyr Babchuk
<Volodymyr_Babchuk@epam.com> wrote:
>
> Hi Mykola,
>
> Mykola Kvach <xakep.amatop@gmail.com> writes:
>
> > From: Mirela Simonovic <mirela.simonovic@aggios.com>
> >
> > 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.
> >
> > Tested on Xilinx Ultrascale+ MPSoC with (and without) powering down
> > the GIC.
>
> I think you can drop this sentence, as I am pretty sure this patch
> wasn't tested on Ultrascale+ for last five(?) years..
Ok, I’ll drop that sentence.
>
> >
> > Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
> > Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xilinx.com>
> > Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> > ---
> > 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 | 154 +++++++++++++++++++++++++++++++++
> > xen/arch/arm/gic.c | 29 +++++++
> > xen/arch/arm/include/asm/gic.h | 12 +++
> > 3 files changed, 195 insertions(+)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index b23e72a3d0..dce8f5e924 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -1098,6 +1098,151 @@ 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;
> > + 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, "%s:%d: GICv2 suspend context not allocated!\n",
> > + __func__, __LINE__);
>
> Should this be ASSERT(gicv2_context.gicd_isenabler) ? Or do we expect
> that this can happen during normal runtime?
>
> Also, you don't need to print __func__ and __LINE__ as dprintk does this
> for you already:
>
> #define dprintk(lvl, fmt, args...) \
> printk(lvl "%s:%d: " fmt, __FILE__, __LINE__, ## args)
>
>
>
> > + 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);
> > +
> > + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 32); i++ )
> > + gicv2_context.gicd_isactiver[i] = readl_gicd(GICD_ISACTIVER + i * 4);
> > +
>
> I think you can merge this two loops
Ok, I'll do it
>
> > + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 4); i++ )
> > + gicv2_context.gicd_ipriorityr[i] = readl_gicd(GICD_IPRIORITYR + i * 4);
> > +
> > + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 4); i++ )
> > + gicv2_context.gicd_itargetsr[i] = readl_gicd(GICD_ITARGETSR + i * 4);
> > +
>
> And this two as well
Ok
>
> > + 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;
> > +
> > + if ( !gicv2_context.gicd_isenabler )
> > + {
> > + dprintk(XENLOG_WARNING, "%s:%d: GICv2 suspend context not allocated!\n",
> > + __func__, __LINE__);
>
> the same comment as for gicv2_suspend(). Actually, we should not reach
> here in any case.
You mean we should just drop the check, right?
>
> > + return;
> > + }
> > +
> > + 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);
> > + }
> > +
> > + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 32); i++ ) {
> > + 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);
> > +
> > + for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 4); i++ )
> > + 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);
> > +
> > + /* 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);
> > +}
> > +
> > +static void gicv2_alloc_context(struct gicv2_context *gc)
> > +{
> > + 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;
> > +
> > + return;
> > +
> > + err_free:
> > + dprintk(XENLOG_ERR,
> > + "%s:%d: failed to allocate memory for GICv2 suspend context\n",
> > + __func__, __LINE__);
>
> Okay, this partially answers to my previous question about memory
> allocation. So it is possible to have active system without space for
> GIC context... But this basically renders system un-suspendable. I think
> this warrants non-debug print to warn user that suspend will be not
> available.
Ok, I’ll add a regular print instead. So your comments about asserts are
irrelevant in this case, right?
>
> [...]
>
> --
> WBR, Volodymyr
Best regards,
Mykola
© 2016 - 2025 Red Hat, Inc.