[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 10/18] xen/arm: Implement GIC suspend/resume functions (gicv2 only)
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?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
