[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 Mirela,

On 14/11/2018 12:52, Mirela Simonovic wrote:
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

AFAICT, none of the PSCI CPU_OFF code will migrate the interrupt. I already pointed that when you send your series to fix the CPU_OFF. But this still does not seem to be addressed. So my question is still open.



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.

Well you know the upper bound. Why can't you use the upper bound?

Alternative is to allocate on suspend, but I believe it is better to do this when the system boots.

Why is it better?


+    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.

How about renaming the other functions to save_vcpu_state/restore_vcpu_state?

Cheers,

Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.