Re: [PATCH v5 02/12] xen/arm: gic-v2: Implement GIC suspend/resume functions

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Volodymyr,

On Sat, Aug 23, 2025 at 3:01 AM Volodymyr Babchuk
<Volodymyr_Babchuk@xxxxxxxx> wrote:
>
> Hi Mykola,
>
> Mykola Kvach <xakep.amatop@xxxxxxxxx> writes:
>
> > 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.
> >
> > 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@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 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