Re: [Xen-devel] [PATCH 10/18] xen/arm: Implement GIC suspend/resume functions (gicv2 only)
On Wed, 14 Nov 2018, Julien Grall wrote:
> Hi,
>
> On 14/11/2018 22:18, Stefano Stabellini wrote:
> > On Wed, 14 Nov 2018, Julien Grall wrote:
> > @@ -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?
> >
> > I'll reply here also to your other point because they are related:
> >
> >> Suspend/resume is not a critical feature in common case. So I would
> >> prefer if we disable it when we can't alloc memory.
> >
> >
> > It is true that suspend/resume is not a critical feature for the common
> > case, but proceeding as "normal" when a memory allocation fails is not a
> > good idea: if the hypervisor is so low in memory as to fail in an
> > allocation like this one, it is not going to be able to work right. In
> > no other cases in Xen we continue on memory allocation failures, even
> > for less-critical features.
> >
> > I suggest that we either allocate statically using the upper bound as
> > you suggested, although it leads to some memory being wasted.
>
> We are speaking of at most 2KB of memory. I don't think it is going to
> be waste given of the number of interrupts GIC usually supports.
>
> The more that we already statically allocate irq_desc.
OK___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 10/18] xen/arm: Implement GIC suspend/resume functions (gicv2 only)
Hi,
On 14/11/2018 22:18, Stefano Stabellini wrote:
> On Wed, 14 Nov 2018, Julien Grall wrote:
> @@ -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?
>
> I'll reply here also to your other point because they are related:
>
>> Suspend/resume is not a critical feature in common case. So I would
>> prefer if we disable it when we can't alloc memory.
>
>
> It is true that suspend/resume is not a critical feature for the common
> case, but proceeding as "normal" when a memory allocation fails is not a
> good idea: if the hypervisor is so low in memory as to fail in an
> allocation like this one, it is not going to be able to work right. In
> no other cases in Xen we continue on memory allocation failures, even
> for less-critical features.
>
> I suggest that we either allocate statically using the upper bound as
> you suggested, although it leads to some memory being wasted.
We are speaking of at most 2KB of memory. I don't think it is going to
be waste given of the number of interrupts GIC usually supports.
The more that we already statically allocate irq_desc.
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 10/18] xen/arm: Implement GIC suspend/resume functions (gicv2 only)
On Wed, 14 Nov 2018, Julien Grall wrote:
> > > > @@ -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?
I'll reply here also to your other point because they are related:
> Suspend/resume is not a critical feature in common case. So I would
> prefer if we disable it when we can't alloc memory.
It is true that suspend/resume is not a critical feature for the common
case, but proceeding as "normal" when a memory allocation fails is not a
good idea: if the hypervisor is so low in memory as to fail in an
allocation like this one, it is not going to be able to work right. In
no other cases in Xen we continue on memory allocation failures, even
for less-critical features.
I suggest that we either allocate statically using the upper bound as
you suggested, although it leads to some memory being wasted. Or, and
this is my favorite option, we allocate it dynamically but we return a
proper error on memory allocation failures. We should at the very
least print an error.
I would prefer if it is done at boot time so that the user can figure
out that their configuration is wrong and fix it straight away, but it
could also be done at suspend time. Better to fail reliably early,
rather than failing unpredictably later.___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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
Signed-off-by: Saeed Nowshadi
---
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();
+ /* Allocate memory to be used for saving GIC context during the
suspend */
+ gicv2_alloc_context(_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
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
Signed-off-by: Saeed Nowshadi
---
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();
+/* Allocate memory to be used for saving GIC context during
the suspend */
+gicv2_alloc_context(_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 <
Re: [Xen-devel] [PATCH 10/18] xen/arm: Implement GIC suspend/resume functions (gicv2 only)
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
Signed-off-by: Saeed Nowshadi
---
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();
+/* Allocate memory to be used for saving GIC context during the suspend */
+gicv2_alloc_context(_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++ )
+
Re: [Xen-devel] [PATCH 10/18] xen/arm: Implement GIC suspend/resume functions (gicv2 only)
On Wed, Nov 14, 2018 at 10:13 AM Julien Grall wrote:
>
> 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
> >> Signed-off-by: Saeed Nowshadi
> >> ---
> >> 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();
> >>
> >> +/* Allocate memory to be used for saving GIC context during the
> >> suspend */
> >> +gicv2_alloc_context(_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 */
> >> +
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
Signed-off-by: Saeed Nowshadi
---
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();
+/* Allocate memory to be used for saving GIC context during the suspend */
+gicv2_alloc_context(_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 <
Re: [Xen-devel] [PATCH 10/18] xen/arm: Implement GIC suspend/resume functions (gicv2 only)
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
> Signed-off-by: Saeed Nowshadi
> ---
> 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();
>
> +/* Allocate memory to be used for saving GIC context during the suspend
> */
> +gicv2_alloc_context(_context);
Please check for the return of gicv2_alloc_context and return error
accordingly.
> 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);
[Xen-devel] [PATCH 10/18] xen/arm: Implement GIC suspend/resume functions (gicv2 only)
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 | 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();
+/* Allocate memory to be used for saving GIC context during the suspend */
+gicv2_alloc_context(_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)
+{
+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);
+
+/* 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);
+
+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(0x, 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(0x, GICD_ICACTIVER + i * 4);
+
+for ( i = 0; i < DIV_ROUND_UP(gicv2_info.nr_lines, 32); i++ )
+
