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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.


>      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?


> +    /* Enable GIC CPU interface */
> +    writel_gicc(gicv2_context.gicc_ctlr | GICC_CTL_ENABLE | GICC_CTL_EOI,
> +                GICC_CTLR);
> +    isb();
> +}
> +
>  const static struct gic_hw_operations gicv2_ops = {
>      .info                = &gicv2_info,
>      .init                = gicv2_init,
> @@ -1351,6 +1496,8 @@ const static struct gic_hw_operations gicv2_ops = {
>      .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings,
>      .iomem_deny_access   = gicv2_iomem_deny_access,
>      .do_LPI              = gicv2_do_LPI,
> +    .suspend             = gicv2_suspend,
> +    .resume              = gicv2_resume,
>  };
>  
>  /* Set up the GIC */
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index e524ad583d..6e98f43691 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -464,6 +464,33 @@ int gic_iomem_deny_access(const struct domain *d)
>      return gic_hw_ops->iomem_deny_access(d);
>  }
>  
> +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;
> +
> +    gic_hw_ops->suspend();

Should return the return value of this suspend() call.


> +    return 0;
> +}
> +
> +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();
> +}
> +
>  static int cpu_gic_callback(struct notifier_block *nfb,
>                              unsigned long action,
>                              void *hcpu)
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 22fa122e52..46066caac8 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -277,6 +277,10 @@ extern int gicv_setup(struct domain *d);
>  extern void gic_save_state(struct vcpu *v);
>  extern void gic_restore_state(struct vcpu *v);
>  
> +/* Suspend/resume */
> +extern int gic_suspend(void);
> +extern void gic_resume(void);
> +
>  /* SGI (AKA IPIs) */
>  enum gic_sgi {
>      GIC_SGI_EVENT_CHECK = 0,
> @@ -390,6 +394,10 @@ struct gic_hw_operations {
>      int (*iomem_deny_access)(const struct domain *d);
>      /* Handle LPIs, which require special handling */
>      void (*do_LPI)(unsigned int lpi);
> +    /* Save GIC configuration due to the system suspend */
> +    int (*suspend)(void);
> +    /* Restore GIC configuration due to the system resume */
> +    void (*resume)(void);
>  };
>  
>  extern const struct gic_hw_operations *gic_hw_ops;
> -- 
> 2.13.0
> 

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