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

[Julien Grall <julien.grall@arm.com>]

Hi,

I am not entirely sure where to ask it, so I will do it here. From my 
understanding of the PSCI spec, all the interrupts should have been migrated 
away from turned off CPU. Where do you ensure that in the suspend path?


On 12/11/2018 11:30, 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@aggios.com>
> Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xilinx.com>
> ---
>   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;
> +};

It took me a long time to understand that you will only save the context for the 
CPU0 and Distributor.

I would prefer if we keep separate per-CPU context and common context. This 
would keep the logic very similar to the rest of the GIC drivers.

> +
> +static struct gicv2_context gicv2_context;
> +
> +static void gicv2_alloc_context(struct gicv2_context *gc);

Please don't do forward declaration. The code should be added in the correct place.

> +
>   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);
> +
>       return 0;
>   }
>   
> @@ -1319,6 +1341,129 @@ static void gicv2_do_LPI(unsigned int lpi)
>       BUG();
>   }
>   
> +static void gicv2_alloc_context(struct gicv2_context *gc)
> +{

Is it necessary to allocate them at boot? Can we make them static or allocate 
them when we suspend?

> +    uint32_t n = gicv2_info.nr_lines;
> +
> +    gc->gicd_isenabler = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 32));
> +    if ( !gc->gicd_isenabler )
> +        return;
> +
> +    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:
> +    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);

AFAICT, those values should always be the same. So I would just restore them 
with the hardcoded value.

This could likely be factored in a separate function that could be used in 
gicv2_cpu_init as well.

> +
> +    /* If gicv2_alloc_context() hasn't allocated memory, return */
> +    if ( !gicv2_context.gicd_isenabler )
> +        return -ENOMEM;
> +
> +    /* Save GICD configuration */
> +    gicv2_context.gicd_ctlr = readl_gicd(GICD_CTLR);

Same here.

> +
> +    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)
> +{
> +    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);

I would prefer if there is only one for with 2 write. This would make more 
obvious why you need to clear all enabled bit first.

> +
> +    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);

Same here.

> +
> +    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();
> +
> +    /* 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();
> +
> +    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);

The comments on tops of suspend/resume suggest that this should be called 
save/restore.

Cheers,

-- 
Julien Grall

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