[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)
Thanks for the review
On 11/14/2018 01:41 PM, Julien Grall wrote:
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?
disable_nonboot_cpus will lead to CPU_OFF PSCI to be called. That is
orthogonal to suspend support in this series
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@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;
+};
It took me a long time to understand that you will only save the
context for the CPU0 and Distributor.
Yes, other physical CPUs are hot-unplugged at this point.
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.
Agreed
+
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?
We need to allocate dynamically because the size of allocated data
depends on the number of irq lines, which is not known at the compile time.
Alternative is to allocate on suspend, but I believe it is better to do
this when the system boots.
+ 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.
I thought that too, but save/restore already exist and are used for
other purposes.
Cheers,
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
