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

2022-12-06 Thread Julien Grall

Hi,

On 07/10/2022 11:32, Mykyta Poturai wrote:

From: Mirela Simonovic 

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 
Signed-off-by: Saeed Nowshadi 


Your signed-off-by is missing.


---
  xen/arch/arm/gic-v2.c | 138 +-
  xen/arch/arm/gic.c|  27 
  xen/include/asm-arm/gic.h |   8 +++
  3 files changed, 172 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index b2adc8ec9a..b077b66d92 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -123,6 +123,23 @@ 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 inline void writeb_gicd(uint8_t val, unsigned int offset)
  {
  writeb_relaxed(val, gicv2.map_dbase + offset);
@@ -1251,6 +1268,40 @@ static void __init gicv2_acpi_init(void)
  static void __init gicv2_acpi_init(void) { }
  #endif
  
+static int 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 )
+goto err_free;
+
+gc->gicd_isactiver = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 32));
+if ( !gc->gicd_isactiver )
+goto err_free;
+
+gc->gicd_itargetsr = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 4));
+if ( !gc->gicd_itargetsr )
+goto err_free;
+
+gc->gicd_ipriorityr = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 4));
+if ( !gc->gicd_ipriorityr )
+goto err_free;
+
+gc->gicd_icfgr = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 16));
+if ( !gc->gicd_icfgr )
+goto err_free;
+
+return 0;
+err_free:
+xfree(gc->gicd_icfgr);
+xfree(gc->gicd_ipriorityr);
+xfree(gc->gicd_itargetsr);
+xfree(gc->gicd_isactiver);
+xfree(gc->gicd_isenabler);


Please add a newline.


+return -ENOMEM;
+}
+
  static int __init gicv2_init(void)
  {
  uint32_t aliased_offset = 0;
@@ -1318,7 +1369,8 @@ static int __init gicv2_init(void)
  
  spin_unlock();
  
-return 0;

+/* Allocate memory to be used for saving GIC context during the suspend */
+return gicv2_alloc_context(_context);
As I pointed out in the previous version, suspend/resume is not going to 
be widely used. So I don't think we should prevent booting if we can't 
allocate the memory.


In addition to that, I think we should consider to have:
  * a command line option to avoid wasting memory if the feature is not 
going to be used
  * ifdef the suspend/resume code to allow an integrator to compile out 
any related code.



  }
  
  static void gicv2_do_LPI(unsigned int lpi)

@@ -1327,6 +1379,88 @@ static void gicv2_do_LPI(unsigned int lpi)
  BUG();
  }
  
+static int gicv2_suspend(void)

+{
+int i;


This should be unsigned int.


+
+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);
+
+/* 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);
+
+/* 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);
+
+return 0;
+}
+
+static void gicv2_resume(void)
+{
+int i;


Ditto.


+
+

[PATCH 09/19] xen/arm: Implement GIC suspend/resume functions (gicv2 only)

2022-10-07 Thread Mykyta Poturai
From: Mirela Simonovic 

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 
Signed-off-by: Saeed Nowshadi 
---
 xen/arch/arm/gic-v2.c | 138 +-
 xen/arch/arm/gic.c|  27 
 xen/include/asm-arm/gic.h |   8 +++
 3 files changed, 172 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index b2adc8ec9a..b077b66d92 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -123,6 +123,23 @@ 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 inline void writeb_gicd(uint8_t val, unsigned int offset)
 {
 writeb_relaxed(val, gicv2.map_dbase + offset);
@@ -1251,6 +1268,40 @@ static void __init gicv2_acpi_init(void)
 static void __init gicv2_acpi_init(void) { }
 #endif
 
+static int 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 )
+goto err_free;
+
+gc->gicd_isactiver = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 32));
+if ( !gc->gicd_isactiver )
+goto err_free;
+
+gc->gicd_itargetsr = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 4));
+if ( !gc->gicd_itargetsr )
+goto err_free;
+
+gc->gicd_ipriorityr = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 4));
+if ( !gc->gicd_ipriorityr )
+goto err_free;
+
+gc->gicd_icfgr = xzalloc_array(uint32_t, DIV_ROUND_UP(n, 16));
+if ( !gc->gicd_icfgr )
+goto err_free;
+
+return 0;
+err_free:
+xfree(gc->gicd_icfgr);
+xfree(gc->gicd_ipriorityr);
+xfree(gc->gicd_itargetsr);
+xfree(gc->gicd_isactiver);
+xfree(gc->gicd_isenabler);
+return -ENOMEM;
+}
+
 static int __init gicv2_init(void)
 {
 uint32_t aliased_offset = 0;
@@ -1318,7 +1369,8 @@ static int __init gicv2_init(void)
 
 spin_unlock();
 
-return 0;
+/* Allocate memory to be used for saving GIC context during the suspend */
+return gicv2_alloc_context(_context);
 }
 
 static void gicv2_do_LPI(unsigned int lpi)
@@ -1327,6 +1379,88 @@ static void gicv2_do_LPI(unsigned int lpi)
 BUG();
 }
 
+static int gicv2_suspend(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);
+
+/* 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);
+
+/* 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);
+
+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);
+
+/* Restore GICD configuration */
+for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 32); i++ ) {
+writel_gicd(0x, GICD_ICENABLER + i * 4);
+writel_gicd(gicv2_context.gicd_isenabler[i], GICD_ISENABLER + i * 4);
+}
+
+for ( i = 0; i <