Re: [Xen-devel] [PATCH 10/18] xen/arm: Implement GIC suspend/resume functions (gicv2 only)

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

On Wed, Nov 14, 2018 at 10:13 AM Julien Grall <julien.grall@xxxxxxx> wrote:
>
> Hi Stefano,
>
> On 11/13/18 11:41 PM, Stefano Stabellini wrote:
> > On Mon, 12 Nov 2018, Mirela Simonovic wrote:
> >> 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@xxxxxxxxxx>
> >> Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xxxxxxxxxx>
> >> ---
> >>   xen/arch/arm/gic-v2.c     | 147 
> >> ++++++++++++++++++++++++++++++++++++++++++++++
> >>   xen/arch/arm/gic.c        |  27 +++++++++
> >>   xen/include/asm-arm/gic.h |   8 +++
> >>   3 files changed, 182 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> >> index e7eb01f30a..bb52b64ecb 100644
> >> --- a/xen/arch/arm/gic-v2.c
> >> +++ b/xen/arch/arm/gic-v2.c
> >> @@ -123,6 +123,25 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
> >>   /* Maximum cpu interface per GIC */
> >>   #define NR_GIC_CPU_IF 8
> >>
> >> +/* 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 void gicv2_alloc_context(struct gicv2_context *gc);
> >> +
> >>   static inline void writeb_gicd(uint8_t val, unsigned int offset)
> >>   {
> >>       writeb_relaxed(val, gicv2.map_dbase + offset);
> >> @@ -1310,6 +1329,9 @@ static int __init gicv2_init(void)
> >>
> >>       spin_unlock(&gicv2.lock);
> >>
> >> +    /* Allocate memory to be used for saving GIC context during the 
> >> suspend */
> >> +    gicv2_alloc_context(&gicv2_context);
> >
> > Please check for the return of gicv2_alloc_context and return error
> > accordingly.
>
> Suspend/resume is not a critical feature in common case. So I would
> prefer if we disable it when we can't alloc memory.
>
> I would also be tempt to #ifdef all related suspend/resume code to allow
> the integrator disabling the feature when they don't want it.
>
> >
> >
> >>       return 0;
> >>   }
> >>
> >> @@ -1319,6 +1341,129 @@ static void gicv2_do_LPI(unsigned int lpi)
> >>       BUG();
> >>   }
> >>
> >> +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 )
> >> +        return;
> >
> > I would return error and return error also below for all the other
> > similar cases.
> >
> >
> >> +    gc->gicd_isactiver = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 32));
> >> +    if ( !gc->gicd_isactiver )
> >> +        goto free_gicd_isenabler;
> >> +
> >> +    gc->gicd_itargetsr = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 4));
> >> +    if ( !gc->gicd_itargetsr )
> >> +        goto free_gicd_isactiver;
> >> +
> >> +    gc->gicd_ipriorityr = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 4));
> >> +    if ( !gc->gicd_ipriorityr )
> >> +        goto free_gicd_itargetsr;
> >> +
> >> +    gc->gicd_icfgr = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 16));
> >> +    if ( gc->gicd_icfgr )
> >> +        return;
> >> +
> >> +    xfree(gc->gicd_ipriorityr);
> >> +
> >> +free_gicd_itargetsr:
> >
> > You can have just one label that frees everything, as you can rely on
> > xfree working fine (doing nothing) for NULL pointers.
> >
> >
> >> +    xfree(gc->gicd_itargetsr);
> >> +
> >> +free_gicd_isactiver:
> >> +    xfree(gc->gicd_isactiver);
> >> +
> >> +free_gicd_isenabler:
> >> +    xfree(gc->gicd_isenabler);
> >> +    gc->gicd_isenabler = NULL;
> >> +}
> >> +
> >> +static int gicv2_suspend(void)
> >> +{
> >> +    int i;
> >> +
> >> +    /* 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);
> >> +
> >> +    /* If gicv2_alloc_context() hasn't allocated memory, return */
> >> +    if ( !gicv2_context.gicd_isenabler )
> >> +        return -ENOMEM;
> >
> > If you are going to check for this, then please check for all the others
> > as well (gicd_isactiver, gicd_ipriorityr, etc.) But if you follow my
> > other suggestion to return error if we fail the memory allocation at
> > init, then this can become an ASSERT. Also, ASSERTS or checks should be
> > at the very beginning of this function.
> >
> >
> >> +    /* 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);
> >
> > Technically, GICD_ICFGR doesn't need to be saved because it could be
> > entirely reconstructed from the informations we have, but I imagine it
> > could be difficult to call the right set of route_irq_to_guest/xen calls
> > at resume time, so I think it is OK.
> >
> >
> >> +    return 0;
> >> +}
> >> +
> >> +static void gicv2_resume(void)
> >> +{
> >> +    int i;
> >> +
> >> +    ASSERT(gicv2_context.gicd_isenabler);
> >> +    ASSERT(gicv2_context.gicd_isactiver);
> >> +    ASSERT(gicv2_context.gicd_ipriorityr);
> >> +    ASSERT(gicv2_context.gicd_itargetsr);
> >> +    ASSERT(gicv2_context.gicd_icfgr);
> >> +
> >> +    /* Disable CPU interface and distributor */
> >> +    writel_gicc(0, GICC_CTLR);
> >> +    writel_gicd(0, GICD_CTLR);
> >> +    isb();
> >> +
> >> +    /* Restore GICD configuration */
> >> +    for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 32); i++ )
> >> +        writel_gicd(0xffffffff, GICD_ICENABLER + i * 4);
> >> +
> >> +    for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 32); i++ )
> >> +        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);
> >> +
> >> +    for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 32); i++ )
> >> +        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 */
> >> +    isb();
> >> +    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);
> >> +    isb();
> >
> > I don't think we need all these isb()'s in this function. Maybe only one
> > at the end, but probably not even that. Julien, what do you think?
>
> I don't think any of the isb() in the code are necessary. What are we
> trying to prevent with them?
>

I also think it's not needed - anywhere

> Cheers,
>
> --
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel