[PATCH RFC v1 00/14] xen: arm: support for save restore and dead migration

[PATCH RFC v1 00/14] xen: arm: support for save restore and dead migration

[Ian Campbell]

For arm32 + gicv2 systems the following supports apparently successful
save/restore as well dead migrate a domain.

There are several caveats/blockers, hence RFC.

 * GIC v2 support only, no GIC v3 at all
 * ARM32 only. Doesn't even build for ARM64 (vfp state handling needs
   adjustment)
 * No live migration, only "dead" (no logdirty support yet)
 * Have not fully audited the set of state which needs to be saved (i.e. I
   might have missed some registers or something)

On the flip side, thanks to migration v2 the toolstack side of this
practically trivial.

This is (of course) using the current Xen hvm blob handling strata, which I
believe there are plans afoot to rework along the lines of the libxc/libxl
migration v2 protocol.

For now I have no problem waiting and reworking in that vein as things
progress, clearly the blockers/caveats (logdirty support in particular) are
going to take some time to sort out.
This is all based (very distantly) on some original patches from Evgeny
Fedotov and Junghyun Yoo (both of Samsung) however very little of that code
remains here.

As well as the Xen series there will be a single Linux patch too, I'll only
CC ARM folks on that one.

Ian.

[PATCH RFC XEN v1 01/14] xen: arm: Add gic_hw_desc

[Ian Campbell]

Because the enum gic_version values do not correspond to the gic
version (in order to allow space for variants such as GICv2m, although
that is currently not present) logging the raw value is not terribly
useful. Provide gic_hw_desc which provides a string describing each
GIC version.

Will be used in a later patch.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/gic.c        | 14 ++++++++++++++
 xen/include/asm-arm/gic.h |  1 +
 2 files changed, 15 insertions(+)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 1e1e5ba..bea39d6 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -62,6 +62,20 @@ enum gic_version gic_hw_version(void)
    return gic_hw_ops->info->hw_version;
 }
 
+const char *gic_hw_desc(enum gic_version v)
+{
+    switch (v) {
+    case GIC_V2: return "GIC v2";
+    case GIC_V3: return "GIC v3";
+    }
+
+    /*
+     * The compiler ought to gripe if the above doesn't cover all enum
+     * gic_version, in case some version doesn't.
+     */
+    return "Unknown";
+}
+
 unsigned int gic_number_lines(void)
 {
     return gic_hw_ops->info->nr_lines;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 42a2eec..818384b 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -221,6 +221,7 @@ enum gic_version {
 };
 
 extern enum gic_version gic_hw_version(void);
+extern const char *gic_hw_desc(enum gic_version v);
 
 /* Program the GIC to route an interrupt */
 extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
-- 
2.6.1

Re: [PATCH RFC XEN v1 01/14] xen: arm: Add gic_hw_desc

[Stefano Stabellini]

On Wed, 9 Dec 2015, Ian Campbell wrote:
> Because the enum gic_version values do not correspond to the gic
> version (in order to allow space for variants such as GICv2m, although
> that is currently not present) logging the raw value is not terribly
> useful. Provide gic_hw_desc which provides a string describing each
> GIC version.
> 
> Will be used in a later patch.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/gic.c        | 14 ++++++++++++++
>  xen/include/asm-arm/gic.h |  1 +
>  2 files changed, 15 insertions(+)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 1e1e5ba..bea39d6 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -62,6 +62,20 @@ enum gic_version gic_hw_version(void)
>     return gic_hw_ops->info->hw_version;
>  }
>  
> +const char *gic_hw_desc(enum gic_version v)
> +{
> +    switch (v) {
> +    case GIC_V2: return "GIC v2";
> +    case GIC_V3: return "GIC v3";
> +    }
> +
> +    /*
> +     * The compiler ought to gripe if the above doesn't cover all enum
> +     * gic_version, in case some version doesn't.
> +     */
> +    return "Unknown";

default: ?

In any case

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


> +}
> +
>  unsigned int gic_number_lines(void)
>  {
>      return gic_hw_ops->info->nr_lines;
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 42a2eec..818384b 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -221,6 +221,7 @@ enum gic_version {
>  };
>  
>  extern enum gic_version gic_hw_version(void);
> +extern const char *gic_hw_desc(enum gic_version v);
>  
>  /* Program the GIC to route an interrupt */
>  extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
> -- 
> 2.6.1
> 

Re: [PATCH RFC XEN v1 01/14] xen: arm: Add gic_hw_desc

[Ian Campbell]

On Tue, 2015-12-15 at 16:15 +0000, Stefano Stabellini wrote:
> On Wed, 9 Dec 2015, Ian Campbell wrote:
> > Because the enum gic_version values do not correspond to the gic
> > version (in order to allow space for variants such as GICv2m, although
> > that is currently not present) logging the raw value is not terribly
> > useful. Provide gic_hw_desc which provides a string describing each
> > GIC version.
> > 
> > Will be used in a later patch.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/gic.c        | 14 ++++++++++++++
> >  xen/include/asm-arm/gic.h |  1 +
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 1e1e5ba..bea39d6 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -62,6 +62,20 @@ enum gic_version gic_hw_version(void)
> >     return gic_hw_ops->info->hw_version;
> >  }
> >  
> > +const char *gic_hw_desc(enum gic_version v)
> > +{
> > +    switch (v) {
> > +    case GIC_V2: return "GIC v2";
> > +    case GIC_V3: return "GIC v3";
> > +    }
> > +
> > +    /*
> > +     * The compiler ought to gripe if the above doesn't cover all enum
> > +     * gic_version, in case some version doesn't.
> > +     */
> > +    return "Unknown";
> 
> default: ?

Deliberately not, since that would quash the expected compiler warning.

> 
> In any case
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Thanks.

> 
> 
> > +}
> > +
> >  unsigned int gic_number_lines(void)
> >  {
> >      return gic_hw_ops->info->nr_lines;
> > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> > index 42a2eec..818384b 100644
> > --- a/xen/include/asm-arm/gic.h
> > +++ b/xen/include/asm-arm/gic.h
> > @@ -221,6 +221,7 @@ enum gic_version {
> >  };
> >  
> >  extern enum gic_version gic_hw_version(void);
> > +extern const char *gic_hw_desc(enum gic_version v);
> >  
> >  /* Program the GIC to route an interrupt */
> >  extern void gic_route_irq_to_xen(struct irq_desc *desc, const
> > cpumask_t *cpu_mask,

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH RFC XEN v1 01/14] xen: arm: Add gic_hw_desc

[Andrew Cooper]

On 15/12/15 16:21, Ian Campbell wrote:
> On Tue, 2015-12-15 at 16:15 +0000, Stefano Stabellini wrote:
>> On Wed, 9 Dec 2015, Ian Campbell wrote:
>>> Because the enum gic_version values do not correspond to the gic
>>> version (in order to allow space for variants such as GICv2m, although
>>> that is currently not present) logging the raw value is not terribly
>>> useful. Provide gic_hw_desc which provides a string describing each
>>> GIC version.
>>>
>>> Will be used in a later patch.
>>>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>>  xen/arch/arm/gic.c        | 14 ++++++++++++++
>>>  xen/include/asm-arm/gic.h |  1 +
>>>  2 files changed, 15 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index 1e1e5ba..bea39d6 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -62,6 +62,20 @@ enum gic_version gic_hw_version(void)
>>>     return gic_hw_ops->info->hw_version;
>>>  }
>>>  
>>> +const char *gic_hw_desc(enum gic_version v)
>>> +{
>>> +    switch (v) {
>>> +    case GIC_V2: return "GIC v2";
>>> +    case GIC_V3: return "GIC v3";
>>> +    }
>>> +
>>> +    /*
>>> +     * The compiler ought to gripe if the above doesn't cover all enum
>>> +     * gic_version, in case some version doesn't.
>>> +     */
>>> +    return "Unknown";
>> default: ?
> Deliberately not, since that would quash the expected compiler warning.

What expected compiler warning?

That switch statement, complete or not, is always valid.

The only way you can plausibly get a warning is to drop the return
"Unknown";, and have the compiler warning about a missing return
statement.  This, however, will backfire on older compilers which
can't/don't check whether the switch statement is complete.

~Andrew

Re: [PATCH RFC XEN v1 01/14] xen: arm: Add gic_hw_desc

[Ian Campbell]

On Tue, 2015-12-15 at 16:35 +0000, Andrew Cooper wrote:
> On 15/12/15 16:21, Ian Campbell wrote:
> > On Tue, 2015-12-15 at 16:15 +0000, Stefano Stabellini wrote:
> > > On Wed, 9 Dec 2015, Ian Campbell wrote:
> > > > Because the enum gic_version values do not correspond to the gic
> > > > version (in order to allow space for variants such as GICv2m,
> > > > although
> > > > that is currently not present) logging the raw value is not
> > > > terribly
> > > > useful. Provide gic_hw_desc which provides a string describing each
> > > > GIC version.
> > > > 
> > > > Will be used in a later patch.
> > > > 
> > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > > ---
> > > >  xen/arch/arm/gic.c        | 14 ++++++++++++++
> > > >  xen/include/asm-arm/gic.h |  1 +
> > > >  2 files changed, 15 insertions(+)
> > > > 
> > > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > > > index 1e1e5ba..bea39d6 100644
> > > > --- a/xen/arch/arm/gic.c
> > > > +++ b/xen/arch/arm/gic.c
> > > > @@ -62,6 +62,20 @@ enum gic_version gic_hw_version(void)
> > > >     return gic_hw_ops->info->hw_version;
> > > >  }
> > > >  
> > > > +const char *gic_hw_desc(enum gic_version v)
> > > > +{
> > > > +    switch (v) {
> > > > +    case GIC_V2: return "GIC v2";
> > > > +    case GIC_V3: return "GIC v3";
> > > > +    }
> > > > +
> > > > +    /*
> > > > +     * The compiler ought to gripe if the above doesn't cover all
> > > > enum
> > > > +     * gic_version, in case some version doesn't.
> > > > +     */
> > > > +    return "Unknown";
> > > default: ?
> > Deliberately not, since that would quash the expected compiler warning.
> 
> What expected compiler warning?
> 
> That switch statement, complete or not, is always valid.

I meant the sort of thing which -Wswitch-enum warns about, i.e. the switch
not covering the whole range of enum values.

The return statement is there for the benefit of other compilers which
cannot do that analysis for some reason.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH RFC XEN v1 02/14] xen: arm: Provide a mechanism to read (and decode) an LR from a saved VCPU

[Ian Campbell]

This will allow generic vgic save code to get at this state without needing
to know about gic v2 vs v3 etc.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/gic-v2.c     | 20 ++++++++++++++++----
 xen/arch/arm/gic-v3.c     | 21 ++++++++++++++++-----
 xen/arch/arm/gic.c        |  5 +++++
 xen/include/asm-arm/gic.h |  7 +++++++
 4 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 01e36b5..5cc4cc3 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -417,11 +417,8 @@ static void gicv2_clear_lr(int lr)
     writel_gich(0, GICH_LR + lr * 4);
 }
 
-static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
+static void gicv2_decode_lr_value(uint32_t lrv, struct gic_lr *lr_reg)
 {
-    uint32_t lrv;
-
-    lrv          = readl_gich(GICH_LR + lr * 4);
     lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) & GICH_V2_LR_PHYSICAL_MASK;
     lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
     lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
@@ -430,6 +427,14 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
     lr_reg->grp       = (lrv >> GICH_V2_LR_GRP_SHIFT) & GICH_V2_LR_GRP_MASK;
 }
 
+static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
+{
+    uint32_t lrv;
+
+    lrv = readl_gich(GICH_LR + lr * 4);
+    gicv2_decode_lr_value(lrv, lr_reg);
+}
+
 static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
 {
     uint32_t lrv = 0;
@@ -447,6 +452,12 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
     writel_gich(lrv, GICH_LR + lr * 4);
 }
 
+static void gicv2_vcpu_saved_lr(const struct vcpu *v, int lr,
+                                struct gic_lr *lr_val)
+{
+    gicv2_decode_lr_value(v->arch.gic.v2.lr[lr], lr_val);
+}
+
 static void gicv2_hcr_status(uint32_t flag, bool_t status)
 {
     uint32_t hcr = readl_gich(GICH_HCR);
@@ -746,6 +757,7 @@ const static struct gic_hw_operations gicv2_ops = {
     .save_state          = gicv2_save_state,
     .restore_state       = gicv2_restore_state,
     .dump_state          = gicv2_dump_state,
+    .vcpu_saved_lr       = gicv2_vcpu_saved_lr,
     .gic_host_irq_type   = &gicv2_host_irq_type,
     .gic_guest_irq_type  = &gicv2_guest_irq_type,
     .eoi_irq             = gicv2_eoi_irq,
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 4fe0c37..342aa4a 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -908,12 +908,8 @@ static void gicv3_clear_lr(int lr)
     gicv3_ich_write_lr(lr, 0);
 }
 
-static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
+static void gicv3_decode_lr_value(uint64_t lrv, struct gic_lr *lr_reg)
 {
-    uint64_t lrv;
-
-    lrv = gicv3_ich_read_lr(lr);
-
     lr_reg->pirq = (lrv >> GICH_LR_PHYSICAL_SHIFT) & GICH_LR_PHYSICAL_MASK;
     lr_reg->virq = (lrv >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
 
@@ -923,6 +919,14 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
     lr_reg->grp       = (lrv >> GICH_LR_GRP_SHIFT) & GICH_LR_GRP_MASK;
 }
 
+static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
+{
+    uint64_t lrv;
+
+    lrv = gicv3_ich_read_lr(lr);
+    gicv3_decode_lr_value(lrv, lr_reg);
+}
+
 static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
 {
     uint64_t lrv = 0;
@@ -937,6 +941,12 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
     gicv3_ich_write_lr(lr_reg, lrv);
 }
 
+static void gicv3_vcpu_saved_lr(const struct vcpu *v, int lr,
+                                struct gic_lr *lr_val)
+{
+    gicv3_decode_lr_value(v->arch.gic.v3.lr[lr], lr_val);
+}
+
 static void gicv3_hcr_status(uint32_t flag, bool_t status)
 {
     uint32_t hcr;
@@ -1298,6 +1308,7 @@ static const struct gic_hw_operations gicv3_ops = {
     .save_state          = gicv3_save_state,
     .restore_state       = gicv3_restore_state,
     .dump_state          = gicv3_dump_state,
+    .vcpu_saved_lr       = gicv3_vcpu_saved_lr,
     .gic_host_irq_type   = &gicv3_host_irq_type,
     .gic_guest_irq_type  = &gicv3_guest_irq_type,
     .eoi_irq             = gicv3_eoi_irq,
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index bea39d6..8d219e3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -108,6 +108,11 @@ void gic_restore_state(struct vcpu *v)
     gic_restore_pending_irqs(v);
 }
 
+void gic_vcpu_saved_lr(const struct vcpu *v, int lr, struct gic_lr *lr_val)
+{
+    gic_hw_ops->vcpu_saved_lr(v, lr, lr_val);
+}
+
 /*
  * needs to be called with a valid cpu_mask, ie each cpu in the mask has
  * already called gic_cpu_init
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 818384b..b6516f4 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -263,6 +263,10 @@ extern int gicv_setup(struct domain *d);
 extern void gic_save_state(struct vcpu *v);
 extern void gic_restore_state(struct vcpu *v);
 
+/* Save/restore */
+extern void gic_vcpu_saved_lr(const struct vcpu *v, int lr,
+                              struct gic_lr *lr_val);
+
 /* SGI (AKA IPIs) */
 enum gic_sgi {
     GIC_SGI_EVENT_CHECK = 0,
@@ -318,6 +322,9 @@ struct gic_hw_operations {
     /* Dump GIC LR register information */
     void (*dump_state)(const struct vcpu *);
 
+    /* Get saved LR state */
+    void (*vcpu_saved_lr)(const struct vcpu *v, int lr, struct gic_lr *lr_val);
+
     /* hw_irq_controller to enable/disable/eoi host irq */
     hw_irq_controller *gic_host_irq_type;
 
-- 
2.6.1

Re: [PATCH RFC XEN v1 02/14] xen: arm: Provide a mechanism to read (and decode) an LR from a saved VCPU

[Stefano Stabellini]

On Wed, 9 Dec 2015, Ian Campbell wrote:
> This will allow generic vgic save code to get at this state without needing
> to know about gic v2 vs v3 etc.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/arch/arm/gic-v2.c     | 20 ++++++++++++++++----
>  xen/arch/arm/gic-v3.c     | 21 ++++++++++++++++-----
>  xen/arch/arm/gic.c        |  5 +++++
>  xen/include/asm-arm/gic.h |  7 +++++++
>  4 files changed, 44 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 01e36b5..5cc4cc3 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -417,11 +417,8 @@ static void gicv2_clear_lr(int lr)
>      writel_gich(0, GICH_LR + lr * 4);
>  }
>  
> -static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
> +static void gicv2_decode_lr_value(uint32_t lrv, struct gic_lr *lr_reg)
>  {
> -    uint32_t lrv;
> -
> -    lrv          = readl_gich(GICH_LR + lr * 4);
>      lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) & GICH_V2_LR_PHYSICAL_MASK;
>      lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
>      lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
> @@ -430,6 +427,14 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>      lr_reg->grp       = (lrv >> GICH_V2_LR_GRP_SHIFT) & GICH_V2_LR_GRP_MASK;
>  }
>  
> +static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
> +{
> +    uint32_t lrv;
> +
> +    lrv = readl_gich(GICH_LR + lr * 4);
> +    gicv2_decode_lr_value(lrv, lr_reg);
> +}
> +
>  static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
>  {
>      uint32_t lrv = 0;
> @@ -447,6 +452,12 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
>      writel_gich(lrv, GICH_LR + lr * 4);
>  }
>  
> +static void gicv2_vcpu_saved_lr(const struct vcpu *v, int lr,
> +                                struct gic_lr *lr_val)
> +{
> +    gicv2_decode_lr_value(v->arch.gic.v2.lr[lr], lr_val);
> +}
> +
>  static void gicv2_hcr_status(uint32_t flag, bool_t status)
>  {
>      uint32_t hcr = readl_gich(GICH_HCR);
> @@ -746,6 +757,7 @@ const static struct gic_hw_operations gicv2_ops = {
>      .save_state          = gicv2_save_state,
>      .restore_state       = gicv2_restore_state,
>      .dump_state          = gicv2_dump_state,
> +    .vcpu_saved_lr       = gicv2_vcpu_saved_lr,
>      .gic_host_irq_type   = &gicv2_host_irq_type,
>      .gic_guest_irq_type  = &gicv2_guest_irq_type,
>      .eoi_irq             = gicv2_eoi_irq,
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 4fe0c37..342aa4a 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -908,12 +908,8 @@ static void gicv3_clear_lr(int lr)
>      gicv3_ich_write_lr(lr, 0);
>  }
>  
> -static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
> +static void gicv3_decode_lr_value(uint64_t lrv, struct gic_lr *lr_reg)
>  {
> -    uint64_t lrv;
> -
> -    lrv = gicv3_ich_read_lr(lr);
> -
>      lr_reg->pirq = (lrv >> GICH_LR_PHYSICAL_SHIFT) & GICH_LR_PHYSICAL_MASK;
>      lr_reg->virq = (lrv >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
>  
> @@ -923,6 +919,14 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
>      lr_reg->grp       = (lrv >> GICH_LR_GRP_SHIFT) & GICH_LR_GRP_MASK;
>  }
>  
> +static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
> +{
> +    uint64_t lrv;
> +
> +    lrv = gicv3_ich_read_lr(lr);
> +    gicv3_decode_lr_value(lrv, lr_reg);
> +}
> +
>  static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>  {
>      uint64_t lrv = 0;
> @@ -937,6 +941,12 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>      gicv3_ich_write_lr(lr_reg, lrv);
>  }
>  
> +static void gicv3_vcpu_saved_lr(const struct vcpu *v, int lr,
> +                                struct gic_lr *lr_val)
> +{
> +    gicv3_decode_lr_value(v->arch.gic.v3.lr[lr], lr_val);
> +}
> +
>  static void gicv3_hcr_status(uint32_t flag, bool_t status)
>  {
>      uint32_t hcr;
> @@ -1298,6 +1308,7 @@ static const struct gic_hw_operations gicv3_ops = {
>      .save_state          = gicv3_save_state,
>      .restore_state       = gicv3_restore_state,
>      .dump_state          = gicv3_dump_state,
> +    .vcpu_saved_lr       = gicv3_vcpu_saved_lr,
>      .gic_host_irq_type   = &gicv3_host_irq_type,
>      .gic_guest_irq_type  = &gicv3_guest_irq_type,
>      .eoi_irq             = gicv3_eoi_irq,
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index bea39d6..8d219e3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -108,6 +108,11 @@ void gic_restore_state(struct vcpu *v)
>      gic_restore_pending_irqs(v);
>  }
>  
> +void gic_vcpu_saved_lr(const struct vcpu *v, int lr, struct gic_lr *lr_val)
> +{
> +    gic_hw_ops->vcpu_saved_lr(v, lr, lr_val);
> +}
> +
>  /*
>   * needs to be called with a valid cpu_mask, ie each cpu in the mask has
>   * already called gic_cpu_init
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 818384b..b6516f4 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -263,6 +263,10 @@ extern int gicv_setup(struct domain *d);
>  extern void gic_save_state(struct vcpu *v);
>  extern void gic_restore_state(struct vcpu *v);
>  
> +/* Save/restore */
> +extern void gic_vcpu_saved_lr(const struct vcpu *v, int lr,
> +                              struct gic_lr *lr_val);
> +
>  /* SGI (AKA IPIs) */
>  enum gic_sgi {
>      GIC_SGI_EVENT_CHECK = 0,
> @@ -318,6 +322,9 @@ struct gic_hw_operations {
>      /* Dump GIC LR register information */
>      void (*dump_state)(const struct vcpu *);
>  
> +    /* Get saved LR state */
> +    void (*vcpu_saved_lr)(const struct vcpu *v, int lr, struct gic_lr *lr_val);
> +
>      /* hw_irq_controller to enable/disable/eoi host irq */
>      hw_irq_controller *gic_host_irq_type;
>  
> -- 
> 2.6.1
> 

[PATCH RFC XEN v1 03/14] xen: arm: switch arch_do_domctl to a common exit path

[Ian Campbell]

... with copyback functionality. A future domctl is going to want
this, rather than end up with different ops having different return
behaviour, simply switch everything over to a single exit path scheme.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domctl.c | 76 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 51 insertions(+), 25 deletions(-)

diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index 30453d8..d42b2bf 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -12,11 +12,15 @@
 #include <xen/hypercall.h>
 #include <xen/iocap.h>
 #include <xsm/xsm.h>
+#include <xen/guest_access.h>
 #include <public/domctl.h>
 
 long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
                     XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 {
+    long rc;
+    bool_t copyback = 0;
+
     switch ( domctl->cmd )
     {
     case XEN_DOMCTL_cacheflush:
@@ -25,30 +29,36 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
         unsigned long e = s + domctl->u.cacheflush.nr_pfns;
 
         if ( domctl->u.cacheflush.nr_pfns > (1U<<MAX_ORDER) )
-            return -EINVAL;
-
-        if ( e < s )
-            return -EINVAL;
+            rc = -EINVAL;
+        else if ( e < s )
+            rc = -EINVAL;
+        else
+            rc = p2m_cache_flush(d, s, e);
 
-        return p2m_cache_flush(d, s, e);
+        break;
     }
     case XEN_DOMCTL_bind_pt_irq:
     {
-        int rc;
         xen_domctl_bind_pt_irq_t *bind = &domctl->u.bind_pt_irq;
         uint32_t irq = bind->u.spi.spi;
         uint32_t virq = bind->machine_irq;
 
         /* We only support PT_IRQ_TYPE_SPI */
         if ( bind->irq_type != PT_IRQ_TYPE_SPI )
-            return -EOPNOTSUPP;
+        {
+            rc = -EOPNOTSUPP;
+            break;
+        }
 
         /*
          * XXX: For now map the interrupt 1:1. Other support will require to
          * modify domain_pirq_to_irq macro.
          */
         if ( irq != virq )
-            return -EINVAL;
+        {
+            rc = -EINVAL;
+            break;
+        }
 
         /*
          * ARM doesn't require separating IRQ assignation into 2
@@ -60,66 +70,82 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
          */
         rc = xsm_map_domain_irq(XSM_HOOK, d, irq, NULL);
         if ( rc )
-            return rc;
+            break;
 
         rc = xsm_bind_pt_irq(XSM_HOOK, d, bind);
         if ( rc )
-            return rc;
+            break;
 
         if ( !irq_access_permitted(current->domain, irq) )
-            return -EPERM;
+        {
+            rc = -EPERM;
+            break;
+        }
 
         if ( !vgic_reserve_virq(d, virq) )
-            return -EBUSY;
+        {
+            rc = -EBUSY;
+            break;
+        }
 
         rc = route_irq_to_guest(d, virq, irq, "routed IRQ");
         if ( rc )
             vgic_free_virq(d, virq);
 
-        return rc;
+        break;
     }
     case XEN_DOMCTL_unbind_pt_irq:
     {
-        int rc;
         xen_domctl_bind_pt_irq_t *bind = &domctl->u.bind_pt_irq;
         uint32_t irq = bind->u.spi.spi;
         uint32_t virq = bind->machine_irq;
 
         /* We only support PT_IRQ_TYPE_SPI */
         if ( bind->irq_type != PT_IRQ_TYPE_SPI )
-            return -EOPNOTSUPP;
+        {
+            rc = -EOPNOTSUPP;
+            break;
+        }
 
         /* For now map the interrupt 1:1 */
         if ( irq != virq )
-            return -EINVAL;
+        {
+            rc = -EINVAL;
+            break;
+        }
 
         rc = xsm_unbind_pt_irq(XSM_HOOK, d, bind);
         if ( rc )
-            return rc;
+            break;
 
         if ( !irq_access_permitted(current->domain, irq) )
-            return -EPERM;
+        {
+            rc = -EPERM;
+            break;
+        }
 
         rc = release_guest_irq(d, virq);
         if ( rc )
-            return rc;
+            break;
 
         vgic_free_virq(d, virq);
 
-        return 0;
+        rc = 0;
+        break;
     }
     default:
-    {
-        int rc;
-
         rc = subarch_do_domctl(domctl, d, u_domctl);
 
         if ( rc == -ENOSYS )
             rc = iommu_do_domctl(domctl, d, u_domctl);
 
-        return rc;
-    }
+        break;
     }
+
+    if ( copyback && __copy_to_guest(u_domctl, domctl, 1) )
+        rc = -EFAULT;
+
+    return rc;
 }
 
 void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
-- 
2.6.1

Re: [PATCH RFC XEN v1 03/14] xen: arm: switch arch_do_domctl to a common exit path

[Stefano Stabellini]

On Wed, 9 Dec 2015, Ian Campbell wrote:
> ... with copyback functionality. A future domctl is going to want
> this, rather than end up with different ops having different return
> behaviour, simply switch everything over to a single exit path scheme.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/domctl.c | 76 ++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 51 insertions(+), 25 deletions(-)
> 
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index 30453d8..d42b2bf 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -12,11 +12,15 @@
>  #include <xen/hypercall.h>
>  #include <xen/iocap.h>
>  #include <xsm/xsm.h>
> +#include <xen/guest_access.h>
>  #include <public/domctl.h>
>  
>  long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>                      XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>  {
> +    long rc;
> +    bool_t copyback = 0;
> +
>      switch ( domctl->cmd )
>      {
>      case XEN_DOMCTL_cacheflush:
> @@ -25,30 +29,36 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>          unsigned long e = s + domctl->u.cacheflush.nr_pfns;
>  
>          if ( domctl->u.cacheflush.nr_pfns > (1U<<MAX_ORDER) )
> -            return -EINVAL;
> -
> -        if ( e < s )
> -            return -EINVAL;
> +            rc = -EINVAL;
> +        else if ( e < s )
> +            rc = -EINVAL;
> +        else
> +            rc = p2m_cache_flush(d, s, e);
>  
> -        return p2m_cache_flush(d, s, e);
> +        break;
>      }
>      case XEN_DOMCTL_bind_pt_irq:
>      {
> -        int rc;
>          xen_domctl_bind_pt_irq_t *bind = &domctl->u.bind_pt_irq;
>          uint32_t irq = bind->u.spi.spi;
>          uint32_t virq = bind->machine_irq;
>  
>          /* We only support PT_IRQ_TYPE_SPI */
>          if ( bind->irq_type != PT_IRQ_TYPE_SPI )
> -            return -EOPNOTSUPP;
> +        {
> +            rc = -EOPNOTSUPP;
> +            break;
> +        }
>  
>          /*
>           * XXX: For now map the interrupt 1:1. Other support will require to
>           * modify domain_pirq_to_irq macro.
>           */
>          if ( irq != virq )
> -            return -EINVAL;
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
>  
>          /*
>           * ARM doesn't require separating IRQ assignation into 2
> @@ -60,66 +70,82 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>           */
>          rc = xsm_map_domain_irq(XSM_HOOK, d, irq, NULL);
>          if ( rc )
> -            return rc;
> +            break;
>  
>          rc = xsm_bind_pt_irq(XSM_HOOK, d, bind);
>          if ( rc )
> -            return rc;
> +            break;
>  
>          if ( !irq_access_permitted(current->domain, irq) )
> -            return -EPERM;
> +        {
> +            rc = -EPERM;
> +            break;
> +        }
>  
>          if ( !vgic_reserve_virq(d, virq) )
> -            return -EBUSY;
> +        {
> +            rc = -EBUSY;
> +            break;
> +        }
>  
>          rc = route_irq_to_guest(d, virq, irq, "routed IRQ");
>          if ( rc )
>              vgic_free_virq(d, virq);
>  
> -        return rc;
> +        break;
>      }
>      case XEN_DOMCTL_unbind_pt_irq:
>      {
> -        int rc;
>          xen_domctl_bind_pt_irq_t *bind = &domctl->u.bind_pt_irq;
>          uint32_t irq = bind->u.spi.spi;
>          uint32_t virq = bind->machine_irq;
>  
>          /* We only support PT_IRQ_TYPE_SPI */
>          if ( bind->irq_type != PT_IRQ_TYPE_SPI )
> -            return -EOPNOTSUPP;
> +        {
> +            rc = -EOPNOTSUPP;
> +            break;
> +        }
>  
>          /* For now map the interrupt 1:1 */
>          if ( irq != virq )
> -            return -EINVAL;
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
>  
>          rc = xsm_unbind_pt_irq(XSM_HOOK, d, bind);
>          if ( rc )
> -            return rc;
> +            break;
>  
>          if ( !irq_access_permitted(current->domain, irq) )
> -            return -EPERM;
> +        {
> +            rc = -EPERM;
> +            break;
> +        }
>  
>          rc = release_guest_irq(d, virq);
>          if ( rc )
> -            return rc;
> +            break;
>  
>          vgic_free_virq(d, virq);
>  
> -        return 0;
> +        rc = 0;
> +        break;
>      }
>      default:
> -    {
> -        int rc;
> -
>          rc = subarch_do_domctl(domctl, d, u_domctl);
>  
>          if ( rc == -ENOSYS )
>              rc = iommu_do_domctl(domctl, d, u_domctl);
>  
> -        return rc;
> -    }
> +        break;
>      }
> +
> +    if ( copyback && __copy_to_guest(u_domctl, domctl, 1) )
> +        rc = -EFAULT;

This code is already present in do_domctl (the caller of this function).
I wonder if it makes sense to pass a copyback* parameter to
arch_do_domctl and just rely on the caller's exit path.

All the other substitutions in this patch are fine.


> +    return rc;
>  }
>  
>  void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
> -- 
> 2.6.1
> 

Re: [PATCH RFC XEN v1 03/14] xen: arm: switch arch_do_domctl to a common exit path

[Ian Campbell]

On Tue, 2015-12-15 at 16:34 +0000, Stefano Stabellini wrote:
> .. .arch_do_domclt...
> > +
> > +    if ( copyback && __copy_to_guest(u_domctl, domctl, 1) )
> > +        rc = -EFAULT;
> 
> This code is already present in do_domctl (the caller of this function).
> I wonder if it makes sense to pass a copyback* parameter to
> arch_do_domctl and just rely on the caller's exit path.

Could do, cc-ing Jan in addition to Andy for x86 perspective (since this
would be a generic change.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH RFC XEN v1 03/14] xen: arm: switch arch_do_domctl to a common exit path

[Andrew Cooper]

On 15/12/15 16:57, Ian Campbell wrote:
> On Tue, 2015-12-15 at 16:34 +0000, Stefano Stabellini wrote:
>>  .. .arch_do_domclt...
>>> +
>>> +    if ( copyback && __copy_to_guest(u_domctl, domctl, 1) )
>>> +        rc = -EFAULT;
>> This code is already present in do_domctl (the caller of this function).
>> I wonder if it makes sense to pass a copyback* parameter to
>> arch_do_domctl and just rely on the caller's exit path.
> Could do, cc-ing Jan in addition to Andy for x86 perspective (since this
> would be a generic change.

+1

~Andrew

Re: [PATCH RFC XEN v1 03/14] xen: arm: switch arch_do_domctl to a common exit path

[Jan Beulich]

>>> On 15.12.15 at 17:57, <ian.campbell@citrix.com> wrote:
> On Tue, 2015-12-15 at 16:34 +0000, Stefano Stabellini wrote:
>> .. .arch_do_domclt...
>> > +
>> > +    if ( copyback && __copy_to_guest(u_domctl, domctl, 1) )
>> > +        rc = -EFAULT;
>> 
>> This code is already present in do_domctl (the caller of this function).
>> I wonder if it makes sense to pass a copyback* parameter to
>> arch_do_domctl and just rely on the caller's exit path.
> 
> Could do, cc-ing Jan in addition to Andy for x86 perspective (since this
> would be a generic change.

I see no reason not to, the more that domctl-s shouldn't be
performance critical, and hence less code duplication can be
considered the main goal here.

Jan

[PATCH RFC XEN v1 04/14] xen: arm: Implement XEN_DOMCTL_getpageframeinfo3.

[Ian Campbell]

This is used by the save/restore code.

On ARM we only have RAM (0) or not-RAM (XTAB) types.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domctl.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index d42b2bf..d3459ee 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -133,6 +133,55 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
         rc = 0;
         break;
     }
+
+   case XEN_DOMCTL_getpageframeinfo3:
+    {
+        unsigned int i;
+        unsigned int num = domctl->u.getpageframeinfo3.num;
+        XEN_GUEST_HANDLE_64(xen_pfn_t) array = domctl->u.getpageframeinfo3.array;
+
+        if ( unlikely(num > 1024) ||
+             unlikely(num != domctl->u.getpageframeinfo3.num) )
+        {
+            rc = -E2BIG;
+            break;
+        }
+
+        for ( i = 0; i < num; ++i )
+        {
+            xen_pfn_t gfn = 0, type = 0;
+            struct page_info *page;
+            p2m_type_t t;
+
+            if ( copy_from_guest_offset(&gfn, array, i, 1) )
+            {
+                rc = -EFAULT;
+                break;
+            }
+
+            page = get_page_from_gfn(d, gfn, &t, P2M_ALLOC);
+
+            if ( unlikely(!page) ||
+                 unlikely(is_xen_heap_page(page)) )
+                type = XEN_DOMCTL_PFINFO_XTAB;
+            else
+                type = 0; /* Just regular RAM */
+
+            if ( page )
+                put_page(page);
+
+            if ( copy_to_guest_offset(array, i, &type, 1) )
+            {
+                rc = -EFAULT;
+                break;
+            }
+        }
+
+        rc = 0;
+
+        break;
+    }
+
     default:
         rc = subarch_do_domctl(domctl, d, u_domctl);
 
-- 
2.6.1

Re: [PATCH RFC XEN v1 04/14] xen: arm: Implement XEN_DOMCTL_getpageframeinfo3.

[Stefano Stabellini]

On Wed, 9 Dec 2015, Ian Campbell wrote:
> This is used by the save/restore code.
> 
> On ARM we only have RAM (0) or not-RAM (XTAB) types.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/arch/arm/domctl.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index d42b2bf..d3459ee 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -133,6 +133,55 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>          rc = 0;
>          break;
>      }
> +
> +   case XEN_DOMCTL_getpageframeinfo3:
> +    {
> +        unsigned int i;
> +        unsigned int num = domctl->u.getpageframeinfo3.num;
> +        XEN_GUEST_HANDLE_64(xen_pfn_t) array = domctl->u.getpageframeinfo3.array;
> +
> +        if ( unlikely(num > 1024) ||
> +             unlikely(num != domctl->u.getpageframeinfo3.num) )

I realize that this check is the same as x86, but where is the 1024
limitation coming from? If it is part of the interface it would be nice
to add it to the comment in xen/include/public/domctl.h.

The implementation below looks correct.

> +        {
> +            rc = -E2BIG;
> +            break;
> +        }
> +
> +        for ( i = 0; i < num; ++i )
> +        {
> +            xen_pfn_t gfn = 0, type = 0;
> +            struct page_info *page;
> +            p2m_type_t t;
> +
> +            if ( copy_from_guest_offset(&gfn, array, i, 1) )
> +            {
> +                rc = -EFAULT;
> +                break;
> +            }
> +
> +            page = get_page_from_gfn(d, gfn, &t, P2M_ALLOC);
> +
> +            if ( unlikely(!page) ||
> +                 unlikely(is_xen_heap_page(page)) )
> +                type = XEN_DOMCTL_PFINFO_XTAB;
> +            else
> +                type = 0; /* Just regular RAM */
> +
> +            if ( page )
> +                put_page(page);
> +
> +            if ( copy_to_guest_offset(array, i, &type, 1) )
> +            {
> +                rc = -EFAULT;
> +                break;
> +            }
> +        }
> +
> +        rc = 0;
> +
> +        break;
> +    }
> +
>      default:
>          rc = subarch_do_domctl(domctl, d, u_domctl);
>  
> -- 
> 2.6.1
> 

[PATCH RFC XEN v1 05/14] xen: arm: Implement basic XEN_DOMCTL_{set, get}hvmcontext support

[Ian Campbell]

This is just the minimally required basic infra and header, no actual
useful data is saved yet.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/Makefile                  |  1 +
 xen/arch/arm/domctl.c                  | 74 ++++++++++++++++++++++++++++++++++
 xen/arch/arm/save.c                    | 53 ++++++++++++++++++++++++
 xen/common/Makefile                    |  2 +
 xen/include/asm-arm/hvm/support.h      | 25 ++++++++++++
 xen/include/public/arch-arm/hvm/save.h | 17 ++++++++
 6 files changed, 172 insertions(+)
 create mode 100644 xen/arch/arm/save.c
 create mode 100644 xen/include/asm-arm/hvm/support.h

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 4ac5edd..5a96486 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -40,6 +40,7 @@ obj-y += device.o
 obj-y += decode.o
 obj-y += processor.o
 obj-y += smc.o
+obj-y += save.o
 
 #obj-bin-y += ....o
 
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index d3459ee..3403e23 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -12,6 +12,7 @@
 #include <xen/hypercall.h>
 #include <xen/iocap.h>
 #include <xsm/xsm.h>
+#include <xen/hvm/save.h>
 #include <xen/guest_access.h>
 #include <public/domctl.h>
 
@@ -134,6 +135,79 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
         break;
     }
 
+    case XEN_DOMCTL_sethvmcontext:
+    {
+        struct hvm_domain_context c = { .size = domctl->u.hvmcontext.size };
+
+        if ( (c.data = xmalloc_bytes(c.size)) == NULL )
+        {
+            rc = -ENOMEM;
+            goto sethvmcontext_out;
+        }
+
+        if ( copy_from_guest(c.data, domctl->u.hvmcontext.buffer, c.size) != 0 )
+        {
+            rc = -EFAULT;
+            goto sethvmcontext_out;
+        }
+
+        domain_pause(d);
+        rc = hvm_load(d, &c);
+        domain_unpause(d);
+
+    sethvmcontext_out:
+        if ( c.data != NULL )
+            xfree(c.data);
+
+        break;
+
+    }
+    case XEN_DOMCTL_gethvmcontext:
+    {
+        struct hvm_domain_context c = { 0 };
+
+        c.size = hvm_save_size(d);
+
+        if ( guest_handle_is_null(domctl->u.hvmcontext.buffer) )
+        {
+            /* Client is querying for the correct buffer size */
+            domctl->u.hvmcontext.size = c.size;
+            rc = 0;
+            goto gethvmcontext_out;
+        }
+
+        /* Check that the client has a big enough buffer */
+        if ( domctl->u.hvmcontext.size < c.size )
+        {
+            rc = -ENOSPC;
+            goto gethvmcontext_out;
+        }
+
+        /* Allocate our own marshalling buffer */
+        if ( (c.data = xmalloc_bytes(c.size)) == NULL )
+        {
+            rc = -ENOMEM;
+            goto gethvmcontext_out;
+        }
+
+        domain_pause(d);
+        rc = hvm_save(d, &c);
+        domain_unpause(d);
+
+        domctl->u.hvmcontext.size = c.cur;
+        if ( copy_to_guest(domctl->u.hvmcontext.buffer, c.data, c.size) != 0 )
+            rc = -EFAULT;
+        else
+            rc = 0;
+
+    gethvmcontext_out:
+        copyback = 1;
+
+        if ( c.data != NULL )
+            xfree(c.data);
+    }
+    break;
+
    case XEN_DOMCTL_getpageframeinfo3:
     {
         unsigned int i;
diff --git a/xen/arch/arm/save.c b/xen/arch/arm/save.c
new file mode 100644
index 0000000..6a1934b
--- /dev/null
+++ b/xen/arch/arm/save.c
@@ -0,0 +1,53 @@
+/*
+ * hvm/save.c: Save and restore HVM guest's emulated hardware state for ARM.
+ *
+ * Copyright (c) 2013, Samsung Electronics.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <asm/hvm/support.h>
+#include <public/hvm/save.h>
+
+void arch_hvm_save(struct domain *d, struct hvm_save_header *hdr)
+{
+}
+
+int arch_hvm_load(struct domain *d, struct hvm_save_header *hdr)
+{
+    if ( hdr->magic != HVM_FILE_MAGIC )
+    {
+        printk(XENLOG_G_ERR "HVM%d restore: bad magic number %#"PRIx32"\n",
+               d->domain_id, hdr->magic);
+        return -1;
+    }
+
+    if ( hdr->version != HVM_FILE_VERSION )
+    {
+        printk(XENLOG_G_ERR "HVM%d restore: unsupported version %u\n",
+               d->domain_id, hdr->version);
+        return -1;
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 3547c8e..ce8078f 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -67,6 +67,8 @@ obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o kernel.o memory.o multicall
 
 subdir-$(x86_64) += hvm
 
+subdir-$(CONFIG_ARM) += hvm
+
 subdir-$(coverage) += gcov
 
 subdir-y += libelf
diff --git a/xen/include/asm-arm/hvm/support.h b/xen/include/asm-arm/hvm/support.h
new file mode 100644
index 0000000..842d8a3
--- /dev/null
+++ b/xen/include/asm-arm/hvm/support.h
@@ -0,0 +1,25 @@
+/*
+ * support.h: HVM support routines used by ARM.
+ *
+ * Copyright (c) 2013, Samsung Electronics
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __ASM_ARM_HVM_SUPPORT_H__
+#define __ASM_ARM_HVM_SUPPORT_H__
+
+#include <xen/sched.h>
+#include <xen/hvm/save.h>
+
+#endif /* __ASM_ARM_HVM_SUPPORT_H__ */
diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h
index 75b8e65..5f4de94 100644
--- a/xen/include/public/arch-arm/hvm/save.h
+++ b/xen/include/public/arch-arm/hvm/save.h
@@ -26,6 +26,23 @@
 #ifndef __XEN_PUBLIC_HVM_SAVE_ARM_H__
 #define __XEN_PUBLIC_HVM_SAVE_ARM_H__
 
+#define HVM_FILE_MAGIC   0x92385520
+#define HVM_FILE_VERSION 0x00000001
+
+struct hvm_save_header
+{
+    uint32_t magic;             /* Must be HVM_FILE_MAGIC */
+    uint32_t version;           /* File format version */
+    uint64_t changeset;         /* Version of Xen that saved this file */
+};
+
+DECLARE_HVM_SAVE_TYPE(HEADER, 1, struct hvm_save_header);
+
+/*
+ * Largest type-code in use
+ */
+#define HVM_SAVE_CODE_MAX 1
+
 #endif
 
 /*
-- 
2.6.1

Re: [PATCH RFC XEN v1 05/14] xen: arm: Implement basic XEN_DOMCTL_{set, get}hvmcontext support

[Stefano Stabellini]

On Wed, 9 Dec 2015, Ian Campbell wrote:
> This is just the minimally required basic infra and header, no actual
> useful data is saved yet.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/Makefile                  |  1 +
>  xen/arch/arm/domctl.c                  | 74 ++++++++++++++++++++++++++++++++++
>  xen/arch/arm/save.c                    | 53 ++++++++++++++++++++++++
>  xen/common/Makefile                    |  2 +
>  xen/include/asm-arm/hvm/support.h      | 25 ++++++++++++
>  xen/include/public/arch-arm/hvm/save.h | 17 ++++++++
>  6 files changed, 172 insertions(+)
>  create mode 100644 xen/arch/arm/save.c
>  create mode 100644 xen/include/asm-arm/hvm/support.h
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 4ac5edd..5a96486 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -40,6 +40,7 @@ obj-y += device.o
>  obj-y += decode.o
>  obj-y += processor.o
>  obj-y += smc.o
> +obj-y += save.o
>  
>  #obj-bin-y += ....o
>  
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index d3459ee..3403e23 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -12,6 +12,7 @@
>  #include <xen/hypercall.h>
>  #include <xen/iocap.h>
>  #include <xsm/xsm.h>
> +#include <xen/hvm/save.h>
>  #include <xen/guest_access.h>
>  #include <public/domctl.h>
>  
> @@ -134,6 +135,79 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>          break;
>      }
>  
> +    case XEN_DOMCTL_sethvmcontext:
> +    {
> +        struct hvm_domain_context c = { .size = domctl->u.hvmcontext.size };

if ( d == currend->domain )
    return some_kind_of_error;
?


> +        if ( (c.data = xmalloc_bytes(c.size)) == NULL )
> +        {
> +            rc = -ENOMEM;
> +            goto sethvmcontext_out;
> +        }
> +
> +        if ( copy_from_guest(c.data, domctl->u.hvmcontext.buffer, c.size) != 0 )
> +        {
> +            rc = -EFAULT;
> +            goto sethvmcontext_out;
> +        }
> +
> +        domain_pause(d);
> +        rc = hvm_load(d, &c);
> +        domain_unpause(d);
> +
> +    sethvmcontext_out:
> +        if ( c.data != NULL )
> +            xfree(c.data);
> +
> +        break;
> +
> +    }
> +    case XEN_DOMCTL_gethvmcontext:
> +    {
> +        struct hvm_domain_context c = { 0 };

again:
if ( d == currend->domain )
    return some_kind_of_error;
?


> +        c.size = hvm_save_size(d);
> +
> +        if ( guest_handle_is_null(domctl->u.hvmcontext.buffer) )
> +        {
> +            /* Client is querying for the correct buffer size */
> +            domctl->u.hvmcontext.size = c.size;
> +            rc = 0;
> +            goto gethvmcontext_out;
> +        }
> +
> +        /* Check that the client has a big enough buffer */
> +        if ( domctl->u.hvmcontext.size < c.size )
> +        {
> +            rc = -ENOSPC;
> +            goto gethvmcontext_out;
> +        }
> +
> +        /* Allocate our own marshalling buffer */
> +        if ( (c.data = xmalloc_bytes(c.size)) == NULL )
> +        {
> +            rc = -ENOMEM;
> +            goto gethvmcontext_out;
> +        }
> +
> +        domain_pause(d);
> +        rc = hvm_save(d, &c);
> +        domain_unpause(d);
> +
> +        domctl->u.hvmcontext.size = c.cur;
> +        if ( copy_to_guest(domctl->u.hvmcontext.buffer, c.data, c.size) != 0 )
> +            rc = -EFAULT;
> +        else
> +            rc = 0;
> +
> +    gethvmcontext_out:
> +        copyback = 1;
> +
> +        if ( c.data != NULL )
> +            xfree(c.data);
> +    }
> +    break;

Actually now that I have read the patch, I think that rather than
duplicating the code, we should just move the x86 implementation of
XEN_DOMCTL_sethvmcontext and XEN_DOMCTL_gethvmcontext to common code.
The differences are negligible.


>     case XEN_DOMCTL_getpageframeinfo3:
>      {
>          unsigned int i;
> diff --git a/xen/arch/arm/save.c b/xen/arch/arm/save.c
> new file mode 100644
> index 0000000..6a1934b
> --- /dev/null
> +++ b/xen/arch/arm/save.c
> @@ -0,0 +1,53 @@
> +/*
> + * hvm/save.c: Save and restore HVM guest's emulated hardware state for ARM.
> + *
> + * Copyright (c) 2013, Samsung Electronics.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <asm/hvm/support.h>
> +#include <public/hvm/save.h>
> +
> +void arch_hvm_save(struct domain *d, struct hvm_save_header *hdr)
> +{
> +}
> +
> +int arch_hvm_load(struct domain *d, struct hvm_save_header *hdr)
> +{
> +    if ( hdr->magic != HVM_FILE_MAGIC )
> +    {
> +        printk(XENLOG_G_ERR "HVM%d restore: bad magic number %#"PRIx32"\n",
> +               d->domain_id, hdr->magic);
> +        return -1;
> +    }
> +
> +    if ( hdr->version != HVM_FILE_VERSION )
> +    {
> +        printk(XENLOG_G_ERR "HVM%d restore: unsupported version %u\n",
> +               d->domain_id, hdr->version);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 3547c8e..ce8078f 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -67,6 +67,8 @@ obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o kernel.o memory.o multicall
>  
>  subdir-$(x86_64) += hvm
>  
> +subdir-$(CONFIG_ARM) += hvm
> +
>  subdir-$(coverage) += gcov
>  
>  subdir-y += libelf
> diff --git a/xen/include/asm-arm/hvm/support.h b/xen/include/asm-arm/hvm/support.h
> new file mode 100644
> index 0000000..842d8a3
> --- /dev/null
> +++ b/xen/include/asm-arm/hvm/support.h
> @@ -0,0 +1,25 @@
> +/*
> + * support.h: HVM support routines used by ARM.
> + *
> + * Copyright (c) 2013, Samsung Electronics
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __ASM_ARM_HVM_SUPPORT_H__
> +#define __ASM_ARM_HVM_SUPPORT_H__
> +
> +#include <xen/sched.h>
> +#include <xen/hvm/save.h>
> +
> +#endif /* __ASM_ARM_HVM_SUPPORT_H__ */

Interestingly I can compile Xen on x86_64 just fine removing the
#include <asm/hvm/support.h> in xen/common/hvm/save.c. I think that is a
better option than introducing this empty header file.


> diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h
> index 75b8e65..5f4de94 100644
> --- a/xen/include/public/arch-arm/hvm/save.h
> +++ b/xen/include/public/arch-arm/hvm/save.h
> @@ -26,6 +26,23 @@
>  #ifndef __XEN_PUBLIC_HVM_SAVE_ARM_H__
>  #define __XEN_PUBLIC_HVM_SAVE_ARM_H__
>  
> +#define HVM_FILE_MAGIC   0x92385520

Out of curiousity, how did you come up with the number? Just making sure
there is no secret message in there.


> +#define HVM_FILE_VERSION 0x00000001
> +
> +struct hvm_save_header
> +{
> +    uint32_t magic;             /* Must be HVM_FILE_MAGIC */
> +    uint32_t version;           /* File format version */
> +    uint64_t changeset;         /* Version of Xen that saved this file */
> +};
> +
> +DECLARE_HVM_SAVE_TYPE(HEADER, 1, struct hvm_save_header);
> +
> +/*
> + * Largest type-code in use
> + */
> +#define HVM_SAVE_CODE_MAX 1
> +
>  #endif
>  
>  /*
> -- 
> 2.6.1
> 

Re: [PATCH RFC XEN v1 05/14] xen: arm: Implement basic XEN_DOMCTL_{set, get}hvmcontext support

[Ian Campbell]

On Tue, 2015-12-15 at 18:00 +0000, Stefano Stabellini wrote:
> On Wed, 9 Dec 2015, Ian Campbell wrote:
> > This is just the minimally required basic infra and header, no actual
> > useful data is saved yet.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/Makefile                  |  1 +
> >  xen/arch/arm/domctl.c                  | 74
> > ++++++++++++++++++++++++++++++++++
> >  xen/arch/arm/save.c                    | 53 ++++++++++++++++++++++++
> >  xen/common/Makefile                    |  2 +
> >  xen/include/asm-arm/hvm/support.h      | 25 ++++++++++++
> >  xen/include/public/arch-arm/hvm/save.h | 17 ++++++++
> >  6 files changed, 172 insertions(+)
> >  create mode 100644 xen/arch/arm/save.c
> >  create mode 100644 xen/include/asm-arm/hvm/support.h
> > 
> > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> > index 4ac5edd..5a96486 100644
> > --- a/xen/arch/arm/Makefile
> > +++ b/xen/arch/arm/Makefile
> > @@ -40,6 +40,7 @@ obj-y += device.o
> >  obj-y += decode.o
> >  obj-y += processor.o
> >  obj-y += smc.o
> > +obj-y += save.o
> >  
> >  #obj-bin-y += ....o
> >  
> > diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> > index d3459ee..3403e23 100644
> > --- a/xen/arch/arm/domctl.c
> > +++ b/xen/arch/arm/domctl.c
> > @@ -12,6 +12,7 @@
> >  #include <xen/hypercall.h>
> >  #include <xen/iocap.h>
> >  #include <xsm/xsm.h>
> > +#include <xen/hvm/save.h>
> >  #include <xen/guest_access.h>
> >  #include <public/domctl.h>
> >  
> > @@ -134,6 +135,79 @@ long arch_do_domctl(struct xen_domctl *domctl,
> > struct domain *d,
> >          break;
> >      }
> >  
> > +    case XEN_DOMCTL_sethvmcontext:
> > +    {
> > +        struct hvm_domain_context c = { .size = domctl-
> > >u.hvmcontext.size };
> 
> if ( d == currend->domain )
>     return some_kind_of_error;
> ?

I'd be inclined to put that into the common code so either hvm_load, or
with your suggestion later to make this common (which sounds doable) here.

Not sure about the get case, there might be reasons other than migration to
want to get at this state? (Maybe nothing other than debug hacking I guess)

[...]
> Actually now that I have read the patch, I think that rather than
> duplicating the code, we should just move the x86 implementation of
> XEN_DOMCTL_sethvmcontext and XEN_DOMCTL_gethvmcontext to common code.
> The differences are negligible.

Yes, I'll give that a go.

> 
> > +#ifndef __ASM_ARM_HVM_SUPPORT_H__
> > +#define __ASM_ARM_HVM_SUPPORT_H__
> > +
> > +#include <xen/sched.h>
> > +#include <xen/hvm/save.h>
> > +
> > +#endif /* __ASM_ARM_HVM_SUPPORT_H__ */
> 
> Interestingly I can compile Xen on x86_64 just fine removing the
> #include <asm/hvm/support.h> in xen/common/hvm/save.c. I think that is a
> better option than introducing this empty header file.

I'll give that a go, it might be invalidated by the consolidation you
suggest above, but in that case the empty header is the lesser evil.

> > diff --git a/xen/include/public/arch-arm/hvm/save.h
> > b/xen/include/public/arch-arm/hvm/save.h
> > index 75b8e65..5f4de94 100644
> > --- a/xen/include/public/arch-arm/hvm/save.h
> > +++ b/xen/include/public/arch-arm/hvm/save.h
> > @@ -26,6 +26,23 @@
> >  #ifndef __XEN_PUBLIC_HVM_SAVE_ARM_H__
> >  #define __XEN_PUBLIC_HVM_SAVE_ARM_H__
> >  
> > +#define HVM_FILE_MAGIC   0x92385520
> 
> Out of curiousity, how did you come up with the number? Just making sure
> there is no secret message in there.

I appear to have gotten it from the original Samsung patches.

If there is a secret message then I can't spot it, it's nothing in ascii at
least (<INVALID>8U<SPACE>).

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH RFC XEN v1 06/14] xen: arm: Add some basic platform info to save header

[Ian Campbell]

These correspond to the content of struct xen_arch_domainconfig.

On restore various things are checked, mostly to ensure they match the
hardcoded things of the restoring Xen.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/save.c                    | 44 ++++++++++++++++++++++++++++++++++
 xen/include/public/arch-arm/hvm/save.h |  6 +++++
 2 files changed, 50 insertions(+)

diff --git a/xen/arch/arm/save.c b/xen/arch/arm/save.c
index 6a1934b..7b34782 100644
--- a/xen/arch/arm/save.c
+++ b/xen/arch/arm/save.c
@@ -21,6 +21,17 @@
 
 void arch_hvm_save(struct domain *d, struct hvm_save_header *hdr)
 {
+    switch ( d->arch.vgic.version )
+    {
+    case GIC_V2: hdr->gic_version = XEN_DOMCTL_CONFIG_GIC_V2; break;
+    case GIC_V3: hdr->gic_version = XEN_DOMCTL_CONFIG_GIC_V3; break;
+    default: BUG();
+    }
+
+    hdr->nr_spis = d->arch.vgic.nr_spis;
+    hdr->clock_frequency = cpu_khz;
+
+    hdr->evtchn_irq = GUEST_EVTCHN_PPI;
 }
 
 int arch_hvm_load(struct domain *d, struct hvm_save_header *hdr)
@@ -39,6 +50,39 @@ int arch_hvm_load(struct domain *d, struct hvm_save_header *hdr)
         return -1;
     }
 
+    switch ( hdr->gic_version )
+    {
+    case XEN_DOMCTL_CONFIG_GIC_V2: d->arch.vgic.version = GIC_V2; break;
+    case XEN_DOMCTL_CONFIG_GIC_V3: d->arch.vgic.version = GIC_V3; break;
+    default:
+        printk(XENLOG_G_ERR "HVM%d restore: unsupported gic version %u\n",
+               d->domain_id, hdr->gic_version);
+        return -1;
+    }
+
+    if ( hdr->nr_spis )
+    {
+        printk(XENLOG_G_ERR "HVM%d restore: cannot support nr_spis != 0, %u\n",
+               d->domain_id, hdr->nr_spis);
+        return -1;
+    }
+
+    if ( hdr->clock_frequency != cpu_khz )
+    {
+        printk(XENLOG_G_ERR
+            "HVM%d restore: unsupported guest clock %"PRId32"kHz on host @ %ldkHz\n",
+            d->domain_id, hdr->clock_frequency, cpu_khz);
+        return -1;
+    }
+
+    if ( hdr->evtchn_irq != GUEST_EVTCHN_PPI )
+    {
+        printk(XENLOG_G_ERR
+            "HVM%d restore: unsupported guest evtchn IRQ%u host uses IRQ%u\n",
+            d->domain_id, hdr->evtchn_irq, GUEST_EVTCHN_PPI);
+        return -1;
+    }
+
     return 0;
 }
 
diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h
index 5f4de94..6f1be37 100644
--- a/xen/include/public/arch-arm/hvm/save.h
+++ b/xen/include/public/arch-arm/hvm/save.h
@@ -34,6 +34,12 @@ struct hvm_save_header
     uint32_t magic;             /* Must be HVM_FILE_MAGIC */
     uint32_t version;           /* File format version */
     uint64_t changeset;         /* Version of Xen that saved this file */
+
+    uint8_t  gic_version;       /* XEN_DOMCTL_CONFIG_GIC_v* (_NOT_ _NATIVE) */
+    uint32_t nr_spis;           /* Currently must be 0 */
+    uint32_t clock_frequency;   /* kHz */
+
+    uint32_t evtchn_irq;
 };
 
 DECLARE_HVM_SAVE_TYPE(HEADER, 1, struct hvm_save_header);
-- 
2.6.1

Re: [PATCH RFC XEN v1 06/14] xen: arm: Add some basic platform info to save header

[Stefano Stabellini]

On Wed, 9 Dec 2015, Ian Campbell wrote:
> These correspond to the content of struct xen_arch_domainconfig.
> 
> On restore various things are checked, mostly to ensure they match the
> hardcoded things of the restoring Xen.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/save.c                    | 44 ++++++++++++++++++++++++++++++++++
>  xen/include/public/arch-arm/hvm/save.h |  6 +++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/xen/arch/arm/save.c b/xen/arch/arm/save.c
> index 6a1934b..7b34782 100644
> --- a/xen/arch/arm/save.c
> +++ b/xen/arch/arm/save.c
> @@ -21,6 +21,17 @@
>  
>  void arch_hvm_save(struct domain *d, struct hvm_save_header *hdr)
>  {
> +    switch ( d->arch.vgic.version )
> +    {
> +    case GIC_V2: hdr->gic_version = XEN_DOMCTL_CONFIG_GIC_V2; break;
> +    case GIC_V3: hdr->gic_version = XEN_DOMCTL_CONFIG_GIC_V3; break;
> +    default: BUG();
> +    }
> +
> +    hdr->nr_spis = d->arch.vgic.nr_spis;
> +    hdr->clock_frequency = cpu_khz;
> +
> +    hdr->evtchn_irq = GUEST_EVTCHN_PPI;
>  }
>  
>  int arch_hvm_load(struct domain *d, struct hvm_save_header *hdr)
> @@ -39,6 +50,39 @@ int arch_hvm_load(struct domain *d, struct hvm_save_header *hdr)
>          return -1;
>      }
>  
> +    switch ( hdr->gic_version )
> +    {
> +    case XEN_DOMCTL_CONFIG_GIC_V2: d->arch.vgic.version = GIC_V2; break;
> +    case XEN_DOMCTL_CONFIG_GIC_V3: d->arch.vgic.version = GIC_V3; break;
> +    default:
> +        printk(XENLOG_G_ERR "HVM%d restore: unsupported gic version %u\n",
> +               d->domain_id, hdr->gic_version);
> +        return -1;
> +    }
> +
> +    if ( hdr->nr_spis )
> +    {
> +        printk(XENLOG_G_ERR "HVM%d restore: cannot support nr_spis != 0, %u\n",
> +               d->domain_id, hdr->nr_spis);
> +        return -1;
> +    }
> +
> +    if ( hdr->clock_frequency != cpu_khz )
> +    {
> +        printk(XENLOG_G_ERR
> +            "HVM%d restore: unsupported guest clock %"PRId32"kHz on host @ %ldkHz\n",
> +            d->domain_id, hdr->clock_frequency, cpu_khz);
> +        return -1;
> +    }
> +
> +    if ( hdr->evtchn_irq != GUEST_EVTCHN_PPI )
> +    {
> +        printk(XENLOG_G_ERR
> +            "HVM%d restore: unsupported guest evtchn IRQ%u host uses IRQ%u\n",
> +            d->domain_id, hdr->evtchn_irq, GUEST_EVTCHN_PPI);
> +        return -1;
> +    }
> +
>      return 0;
>  }
>  
> diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h
> index 5f4de94..6f1be37 100644
> --- a/xen/include/public/arch-arm/hvm/save.h
> +++ b/xen/include/public/arch-arm/hvm/save.h
> @@ -34,6 +34,12 @@ struct hvm_save_header
>      uint32_t magic;             /* Must be HVM_FILE_MAGIC */
>      uint32_t version;           /* File format version */
>      uint64_t changeset;         /* Version of Xen that saved this file */
> +
> +    uint8_t  gic_version;       /* XEN_DOMCTL_CONFIG_GIC_v* (_NOT_ _NATIVE) */
> +    uint32_t nr_spis;           /* Currently must be 0 */
> +    uint32_t clock_frequency;   /* kHz */
> +
> +    uint32_t evtchn_irq;

I would prefer if you added these in the previous patch, or maybe
better, you introduced HVM_FILE_VERSION only with the last patch.
Otherwise theoretically you would have to bump HVM_FILE_VERSION with
this patch.

The rest looks good.


>  };
>  
>  DECLARE_HVM_SAVE_TYPE(HEADER, 1, struct hvm_save_header);
> -- 
> 2.6.1
> 

Re: [PATCH RFC XEN v1 06/14] xen: arm: Add some basic platform info to save header

[Ian Campbell]

On Tue, 2015-12-15 at 18:37 +0000, Stefano Stabellini wrote:
> On Wed, 9 Dec 2015, Ian Campbell wrote:
> > These correspond to the content of struct xen_arch_domainconfig.
> > 
> > On restore various things are checked, mostly to ensure they match the
> > hardcoded things of the restoring Xen.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/save.c                    | 44
> > ++++++++++++++++++++++++++++++++++
> >  xen/include/public/arch-arm/hvm/save.h |  6 +++++
> >  2 files changed, 50 insertions(+)
> > 
> > diff --git a/xen/arch/arm/save.c b/xen/arch/arm/save.c
> > index 6a1934b..7b34782 100644
> > --- a/xen/arch/arm/save.c
> > +++ b/xen/arch/arm/save.c
> > @@ -21,6 +21,17 @@
> >  
> >  void arch_hvm_save(struct domain *d, struct hvm_save_header *hdr)
> >  {
> > +    switch ( d->arch.vgic.version )
> > +    {
> > +    case GIC_V2: hdr->gic_version = XEN_DOMCTL_CONFIG_GIC_V2; break;
> > +    case GIC_V3: hdr->gic_version = XEN_DOMCTL_CONFIG_GIC_V3; break;
> > +    default: BUG();
> > +    }
> > +
> > +    hdr->nr_spis = d->arch.vgic.nr_spis;
> > +    hdr->clock_frequency = cpu_khz;
> > +
> > +    hdr->evtchn_irq = GUEST_EVTCHN_PPI;
> >  }
> >  
> >  int arch_hvm_load(struct domain *d, struct hvm_save_header *hdr)
> > @@ -39,6 +50,39 @@ int arch_hvm_load(struct domain *d, struct
> > hvm_save_header *hdr)
> >          return -1;
> >      }
> >  
> > +    switch ( hdr->gic_version )
> > +    {
> > +    case XEN_DOMCTL_CONFIG_GIC_V2: d->arch.vgic.version = GIC_V2;
> > break;
> > +    case XEN_DOMCTL_CONFIG_GIC_V3: d->arch.vgic.version = GIC_V3;
> > break;
> > +    default:
> > +        printk(XENLOG_G_ERR "HVM%d restore: unsupported gic version
> > %u\n",
> > +               d->domain_id, hdr->gic_version);
> > +        return -1;
> > +    }
> > +
> > +    if ( hdr->nr_spis )
> > +    {
> > +        printk(XENLOG_G_ERR "HVM%d restore: cannot support nr_spis !=
> > 0, %u\n",
> > +               d->domain_id, hdr->nr_spis);
> > +        return -1;
> > +    }
> > +
> > +    if ( hdr->clock_frequency != cpu_khz )
> > +    {
> > +        printk(XENLOG_G_ERR
> > +            "HVM%d restore: unsupported guest clock %"PRId32"kHz on
> > host @ %ldkHz\n",
> > +            d->domain_id, hdr->clock_frequency, cpu_khz);
> > +        return -1;
> > +    }
> > +
> > +    if ( hdr->evtchn_irq != GUEST_EVTCHN_PPI )
> > +    {
> > +        printk(XENLOG_G_ERR
> > +            "HVM%d restore: unsupported guest evtchn IRQ%u host uses
> > IRQ%u\n",
> > +            d->domain_id, hdr->evtchn_irq, GUEST_EVTCHN_PPI);
> > +        return -1;
> > +    }
> > +
> >      return 0;
> >  }
> >  
> > diff --git a/xen/include/public/arch-arm/hvm/save.h
> > b/xen/include/public/arch-arm/hvm/save.h
> > index 5f4de94..6f1be37 100644
> > --- a/xen/include/public/arch-arm/hvm/save.h
> > +++ b/xen/include/public/arch-arm/hvm/save.h
> > @@ -34,6 +34,12 @@ struct hvm_save_header
> >      uint32_t magic;             /* Must be HVM_FILE_MAGIC */
> >      uint32_t version;           /* File format version */
> >      uint64_t changeset;         /* Version of Xen that saved this file
> > */
> > +
> > +    uint8_t  gic_version;       /* XEN_DOMCTL_CONFIG_GIC_v* (_NOT_
> > _NATIVE) */
> > +    uint32_t nr_spis;           /* Currently must be 0 */
> > +    uint32_t clock_frequency;   /* kHz */
> > +
> > +    uint32_t evtchn_irq;
> 
> I would prefer if you added these in the previous patch, or maybe
> better, you introduced HVM_FILE_VERSION only with the last patch.
> Otherwise theoretically you would have to bump HVM_FILE_VERSION with
> this patch.

I'm not terribly concerned with save file versioning in the midst of the
series which implements save/restore, it's ludicrous to suggest we might be
required to support save/restore with this series half applied, but I'll
fold it in anyway.

> The rest looks good.

Thanks.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH RFC XEN v1 07/14] xen: arm: Save and restore basic per-VCPU state.

[Ian Campbell]

XXX TBD No support for arm64 (or even 32-bit guest on arm64).
XXX In particular the handling of save/restore of VFP state doesn't
XXX even compile for arm32. I need to investigate the best way to
XXX reflect the differing possible VFB states in the save record.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/hvm.c                     | 167 +++++++++++++++++++++++++++++++++
 xen/include/public/arch-arm/hvm/save.h |  38 +++++++-
 2 files changed, 204 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 5fd0753..3c59e63 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -7,6 +7,7 @@
 
 #include <xsm/xsm.h>
 
+#include <xen/hvm/save.h>
 #include <public/xen.h>
 #include <public/hvm/params.h>
 #include <public/hvm/hvm_op.h>
@@ -65,3 +66,169 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     return rc;
 }
+
+static int cpu_save(struct domain *d, hvm_domain_context_t *h)
+{
+    struct hvm_hw_cpu ctxt;
+    struct vcpu *v;
+
+    /* Save the state of CPU */
+    for_each_vcpu( d, v )
+    {
+        /*
+         * We don't need to save state for a vcpu that is down; the restore
+         * code will leave it down if there is nothing saved.
+         */
+        if ( test_bit(_VPF_down, &v->pause_flags) )
+            continue;
+
+        memset(&ctxt, 0, sizeof(ctxt));
+        ctxt.sctlr = v->arch.sctlr;
+        ctxt.ttbr0 = v->arch.ttbr0;
+        ctxt.ttbr1 = v->arch.ttbr1;
+        ctxt.ttbcr = v->arch.ttbcr;
+
+        ctxt.dacr = v->arch.dacr;
+#ifdef CONFIG_ARM_32 /* XXX 32on64 */
+        ctxt.ifar = v->arch.ifar;
+        ctxt.ifsr = v->arch.ifsr;
+        ctxt.dfar = v->arch.dfar;
+        ctxt.dfsr = v->arch.dfsr;
+#else
+        /* XXX 64-bit */
+#endif
+
+#ifdef CONFIG_ARM_32
+        ctxt.mair0 = v->arch.mair0;
+        ctxt.mair1 = v->arch.mair1;
+#else
+        ctxt.mair0 = v->arch.mair;
+#endif
+        /* Control Registers */
+        ctxt.actlr = v->arch.actlr;
+        ctxt.sctlr = v->arch.sctlr;
+        ctxt.cpacr = v->arch.cpacr;
+
+        ctxt.contextidr = v->arch.contextidr;
+        ctxt.tpidr_el0 = v->arch.tpidr_el0;
+        ctxt.tpidr_el1 = v->arch.tpidr_el1;
+        ctxt.tpidrro_el0 = v->arch.tpidrro_el0;
+
+        /* CP 15 */
+        ctxt.csselr = v->arch.csselr;
+        ctxt.mpidr = v->arch.vmpidr;
+
+        ctxt.afsr0 = v->arch.afsr0;
+        ctxt.afsr1 = v->arch.afsr1;
+        ctxt.vbar = v->arch.vbar;
+        ctxt.par = v->arch.par;
+        ctxt.teecr = v->arch.teecr;
+        ctxt.teehbr = v->arch.teehbr;
+#ifdef CONFIG_ARM_32 /* XXX 32on64 */
+        ctxt.joscr = v->arch.joscr;
+        ctxt.jmcr = v->arch.jmcr;
+#endif
+
+        memset(&ctxt.core_regs, 0, sizeof(ctxt.core_regs));
+
+        /* get guest core registers */
+        vcpu_regs_hyp_to_user(v, &ctxt.core_regs);
+
+        /* check VFP state size before dumping */
+        BUILD_BUG_ON(sizeof(v->arch.vfp) > sizeof (ctxt.vfp));
+        memcpy((void*) &ctxt.vfp, (void*) &v->arch.vfp, sizeof(v->arch.vfp));
+
+        if ( hvm_save_entry(VCPU, v->vcpu_id, h, &ctxt) != 0 )
+            return 1;
+    }
+    return 0;
+}
+
+static int cpu_load(struct domain *d, hvm_domain_context_t *h)
+{
+    int vcpuid;
+    struct hvm_hw_cpu ctxt;
+    struct vcpu *v;
+
+    /* Which vcpu is this? */
+    vcpuid = hvm_load_instance(h);
+    if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
+    {
+        dprintk(XENLOG_G_ERR, "HVM restore: dom%u has no vcpu%u\n",
+                d->domain_id, vcpuid);
+        return -EINVAL;
+    }
+
+    if ( hvm_load_entry(VCPU, h, &ctxt) != 0 )
+        return -EINVAL;
+
+    v->arch.sctlr = ctxt.sctlr;
+    v->arch.ttbr0 = ctxt.ttbr0;
+    v->arch.ttbr1 = ctxt.ttbr1;
+    v->arch.ttbcr = ctxt.ttbcr;
+
+    v->arch.dacr = ctxt.dacr;
+#ifdef CONFIG_ARM_32 /* XXX 32on64 */
+    v->arch.ifar = ctxt.ifar;
+    v->arch.ifsr = ctxt.ifsr;
+    v->arch.dfar = ctxt.dfar;
+    v->arch.dfsr = ctxt.dfsr;
+#else
+    /* XXX 64-bit */
+#endif
+
+#ifdef CONFIG_ARM_32
+    v->arch.mair0 = ctxt.mair0;
+    v->arch.mair1 = ctxt.mair1;
+#else
+    v->arch.mair = ctxt.mair0;
+#endif
+
+    /* Control Registers */
+    v->arch.actlr = ctxt.actlr;
+    v->arch.cpacr = ctxt.cpacr;
+    v->arch.contextidr = ctxt.contextidr;
+    v->arch.tpidr_el0 = ctxt.tpidr_el0;
+    v->arch.tpidr_el1 = ctxt.tpidr_el1;
+    v->arch.tpidrro_el0 = ctxt.tpidrro_el0;
+
+    /* CP 15 */
+    v->arch.csselr = ctxt.csselr;
+    v->arch.vmpidr = ctxt.mpidr;
+
+    v->arch.afsr0 = ctxt.afsr0;
+    v->arch.afsr1 = ctxt.afsr1;
+    v->arch.vbar = ctxt.vbar;
+    v->arch.par = ctxt.par;
+    v->arch.teecr = ctxt.teecr;
+    v->arch.teehbr = ctxt.teehbr;
+#ifdef CONFIG_ARM_32 /* XXX 32on64 */
+    v->arch.joscr = ctxt.joscr;
+    v->arch.jmcr = ctxt.jmcr;
+#endif
+
+    /* set guest core registers */
+    vcpu_regs_user_to_hyp(v, &ctxt.core_regs);
+
+    /* restore VFP */
+    BUILD_BUG_ON(sizeof(v->arch.vfp) > sizeof (ctxt.vfp));
+    memcpy(&v->arch.vfp, &ctxt.vfp,  sizeof(v->arch.vfp));
+
+    v->is_initialised = 1;
+    clear_bit(_VPF_down, &v->pause_flags);
+
+    /* we don't need vcpu_wake(v) here */
+    return 0;
+}
+
+HVM_REGISTER_SAVE_RESTORE(VCPU, cpu_save, cpu_load, 1, HVMSR_PER_VCPU);
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h
index 6f1be37..72474e5 100644
--- a/xen/include/public/arch-arm/hvm/save.h
+++ b/xen/include/public/arch-arm/hvm/save.h
@@ -44,10 +44,46 @@ struct hvm_save_header
 
 DECLARE_HVM_SAVE_TYPE(HEADER, 1, struct hvm_save_header);
 
+struct hvm_hw_cpu
+{
+    uint64_t vfp[34]; /* Vector floating pointer */
+    /* VFP v3 state is 34x64 bit, VFP v4 is not yet supported */
+
+    /* Guest core registers */
+    struct vcpu_guest_core_regs core_regs;
+
+    uint32_t sctlr, ttbcr;
+    uint64_t ttbr0, ttbr1;
+
+    uint32_t ifar, dfar;
+    uint32_t ifsr, dfsr;
+    uint32_t dacr;
+    uint64_t par;
+
+    uint64_t mair0, mair1;
+    uint64_t tpidr_el0;
+    uint64_t tpidr_el1;
+    uint64_t tpidrro_el0;
+    uint64_t vbar;
+
+    /* Control Registers */
+    uint32_t actlr;
+    uint32_t cpacr;
+    uint32_t afsr0, afsr1;
+    uint32_t contextidr;
+    uint32_t teecr, teehbr; /* ThumbEE, 32-bit guests only */
+    uint32_t joscr, jmcr;
+    /* CP 15 */
+    uint32_t csselr;
+    uint64_t mpidr;
+};
+
+DECLARE_HVM_SAVE_TYPE(VCPU, 2, struct hvm_hw_cpu);
+
 /*
  * Largest type-code in use
  */
-#define HVM_SAVE_CODE_MAX 1
+#define HVM_SAVE_CODE_MAX 2
 
 #endif
 
-- 
2.6.1

Re: [PATCH RFC XEN v1 07/14] xen: arm: Save and restore basic per-VCPU state.

[Stefano Stabellini]

On Wed, 9 Dec 2015, Ian Campbell wrote:
> XXX TBD No support for arm64 (or even 32-bit guest on arm64).
> XXX In particular the handling of save/restore of VFP state doesn't
> XXX even compile for arm32. I need to investigate the best way to
> XXX reflect the differing possible VFB states in the save record.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/hvm.c                     | 167 +++++++++++++++++++++++++++++++++
>  xen/include/public/arch-arm/hvm/save.h |  38 +++++++-
>  2 files changed, 204 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index 5fd0753..3c59e63 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -7,6 +7,7 @@
>  
>  #include <xsm/xsm.h>
>  
> +#include <xen/hvm/save.h>
>  #include <public/xen.h>
>  #include <public/hvm/params.h>
>  #include <public/hvm/hvm_op.h>
> @@ -65,3 +66,169 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>      return rc;
>  }
> +
> +static int cpu_save(struct domain *d, hvm_domain_context_t *h)
> +{
> +    struct hvm_hw_cpu ctxt;
> +    struct vcpu *v;

I wonder whether we could refactor the code to share the same underlying
function between cpu_save and ctxt_switch_from.


> +    /* Save the state of CPU */
> +    for_each_vcpu( d, v )
> +    {
> +        /*
> +         * We don't need to save state for a vcpu that is down; the restore
> +         * code will leave it down if there is nothing saved.
> +         */
> +        if ( test_bit(_VPF_down, &v->pause_flags) )
> +            continue;
> +
> +        memset(&ctxt, 0, sizeof(ctxt));
> +        ctxt.sctlr = v->arch.sctlr;
> +        ctxt.ttbr0 = v->arch.ttbr0;
> +        ctxt.ttbr1 = v->arch.ttbr1;
> +        ctxt.ttbcr = v->arch.ttbcr;
> +
> +        ctxt.dacr = v->arch.dacr;

Shouldn't this be 32bit only too?


> +#ifdef CONFIG_ARM_32 /* XXX 32on64 */
> +        ctxt.ifar = v->arch.ifar;
> +        ctxt.ifsr = v->arch.ifsr;
> +        ctxt.dfar = v->arch.dfar;
> +        ctxt.dfsr = v->arch.dfsr;
> +#else
> +        /* XXX 64-bit */

far, esr


> +#endif
> +
> +#ifdef CONFIG_ARM_32
> +        ctxt.mair0 = v->arch.mair0;
> +        ctxt.mair1 = v->arch.mair1;

amair0 and amair1?


> +#else
> +        ctxt.mair0 = v->arch.mair;

amair


> +#endif
> +        /* Control Registers */
> +        ctxt.actlr = v->arch.actlr;
> +        ctxt.sctlr = v->arch.sctlr;

duplicate


> +        ctxt.cpacr = v->arch.cpacr;
> +
> +        ctxt.contextidr = v->arch.contextidr;
> +        ctxt.tpidr_el0 = v->arch.tpidr_el0;
> +        ctxt.tpidr_el1 = v->arch.tpidr_el1;
> +        ctxt.tpidrro_el0 = v->arch.tpidrro_el0;
> +
> +        /* CP 15 */
> +        ctxt.csselr = v->arch.csselr;
> +        ctxt.mpidr = v->arch.vmpidr;
> +
> +        ctxt.afsr0 = v->arch.afsr0;
> +        ctxt.afsr1 = v->arch.afsr1;
> +        ctxt.vbar = v->arch.vbar;
> +        ctxt.par = v->arch.par;
> +        ctxt.teecr = v->arch.teecr;
> +        ctxt.teehbr = v->arch.teehbr;

teecr and teehbr are 32-bit only


> +#ifdef CONFIG_ARM_32 /* XXX 32on64 */
> +        ctxt.joscr = v->arch.joscr;
> +        ctxt.jmcr = v->arch.jmcr;
> +#endif
> +
> +        memset(&ctxt.core_regs, 0, sizeof(ctxt.core_regs));
> +
> +        /* get guest core registers */
> +        vcpu_regs_hyp_to_user(v, &ctxt.core_regs);
> +
> +        /* check VFP state size before dumping */
> +        BUILD_BUG_ON(sizeof(v->arch.vfp) > sizeof (ctxt.vfp));
> +        memcpy((void*) &ctxt.vfp, (void*) &v->arch.vfp, sizeof(v->arch.vfp));
> +
> +        if ( hvm_save_entry(VCPU, v->vcpu_id, h, &ctxt) != 0 )
> +            return 1;
> +    }
> +    return 0;
> +}
> +
> +static int cpu_load(struct domain *d, hvm_domain_context_t *h)
> +{
> +    int vcpuid;
> +    struct hvm_hw_cpu ctxt;
> +    struct vcpu *v;
> +
> +    /* Which vcpu is this? */
> +    vcpuid = hvm_load_instance(h);
> +    if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
> +    {
> +        dprintk(XENLOG_G_ERR, "HVM restore: dom%u has no vcpu%u\n",
> +                d->domain_id, vcpuid);
> +        return -EINVAL;
> +    }
> +
> +    if ( hvm_load_entry(VCPU, h, &ctxt) != 0 )
> +        return -EINVAL;
> +
> +    v->arch.sctlr = ctxt.sctlr;
> +    v->arch.ttbr0 = ctxt.ttbr0;
> +    v->arch.ttbr1 = ctxt.ttbr1;
> +    v->arch.ttbcr = ctxt.ttbcr;
> +
> +    v->arch.dacr = ctxt.dacr;
> +#ifdef CONFIG_ARM_32 /* XXX 32on64 */
> +    v->arch.ifar = ctxt.ifar;
> +    v->arch.ifsr = ctxt.ifsr;
> +    v->arch.dfar = ctxt.dfar;
> +    v->arch.dfsr = ctxt.dfsr;
> +#else
> +    /* XXX 64-bit */
> +#endif
> +
> +#ifdef CONFIG_ARM_32
> +    v->arch.mair0 = ctxt.mair0;
> +    v->arch.mair1 = ctxt.mair1;
> +#else
> +    v->arch.mair = ctxt.mair0;
> +#endif
> +
> +    /* Control Registers */
> +    v->arch.actlr = ctxt.actlr;
> +    v->arch.cpacr = ctxt.cpacr;
> +    v->arch.contextidr = ctxt.contextidr;
> +    v->arch.tpidr_el0 = ctxt.tpidr_el0;
> +    v->arch.tpidr_el1 = ctxt.tpidr_el1;
> +    v->arch.tpidrro_el0 = ctxt.tpidrro_el0;
> +
> +    /* CP 15 */
> +    v->arch.csselr = ctxt.csselr;
> +    v->arch.vmpidr = ctxt.mpidr;
> +
> +    v->arch.afsr0 = ctxt.afsr0;
> +    v->arch.afsr1 = ctxt.afsr1;
> +    v->arch.vbar = ctxt.vbar;
> +    v->arch.par = ctxt.par;
> +    v->arch.teecr = ctxt.teecr;
> +    v->arch.teehbr = ctxt.teehbr;
> +#ifdef CONFIG_ARM_32 /* XXX 32on64 */
> +    v->arch.joscr = ctxt.joscr;
> +    v->arch.jmcr = ctxt.jmcr;
> +#endif
> +
> +    /* set guest core registers */
> +    vcpu_regs_user_to_hyp(v, &ctxt.core_regs);
> +
> +    /* restore VFP */
> +    BUILD_BUG_ON(sizeof(v->arch.vfp) > sizeof (ctxt.vfp));
> +    memcpy(&v->arch.vfp, &ctxt.vfp,  sizeof(v->arch.vfp));
> +
> +    v->is_initialised = 1;
> +    clear_bit(_VPF_down, &v->pause_flags);
> +
> +    /* we don't need vcpu_wake(v) here */
> +    return 0;
> +}
> +
> +HVM_REGISTER_SAVE_RESTORE(VCPU, cpu_save, cpu_load, 1, HVMSR_PER_VCPU);
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h
> index 6f1be37..72474e5 100644
> --- a/xen/include/public/arch-arm/hvm/save.h
> +++ b/xen/include/public/arch-arm/hvm/save.h
> @@ -44,10 +44,46 @@ struct hvm_save_header
>  
>  DECLARE_HVM_SAVE_TYPE(HEADER, 1, struct hvm_save_header);
>  
> +struct hvm_hw_cpu
> +{
> +    uint64_t vfp[34]; /* Vector floating pointer */
> +    /* VFP v3 state is 34x64 bit, VFP v4 is not yet supported */
> +
> +    /* Guest core registers */
> +    struct vcpu_guest_core_regs core_regs;
> +
> +    uint32_t sctlr, ttbcr;
> +    uint64_t ttbr0, ttbr1;
> +
> +    uint32_t ifar, dfar;
> +    uint32_t ifsr, dfsr;
> +    uint32_t dacr;
> +    uint64_t par;
> +
> +    uint64_t mair0, mair1;
> +    uint64_t tpidr_el0;
> +    uint64_t tpidr_el1;
> +    uint64_t tpidrro_el0;
> +    uint64_t vbar;
> +
> +    /* Control Registers */
> +    uint32_t actlr;
> +    uint32_t cpacr;
> +    uint32_t afsr0, afsr1;
> +    uint32_t contextidr;
> +    uint32_t teecr, teehbr; /* ThumbEE, 32-bit guests only */
> +    uint32_t joscr, jmcr;
> +    /* CP 15 */
> +    uint32_t csselr;
> +    uint64_t mpidr;
> +};
> +
> +DECLARE_HVM_SAVE_TYPE(VCPU, 2, struct hvm_hw_cpu);
> +
>  /*
>   * Largest type-code in use
>   */
> -#define HVM_SAVE_CODE_MAX 1
> +#define HVM_SAVE_CODE_MAX 2
>  
>  #endif
>  
> -- 
> 2.6.1
> 

Re: [PATCH RFC XEN v1 07/14] xen: arm: Save and restore basic per-VCPU state.

[Ian Campbell]

On Wed, 2015-12-16 at 14:55 +0000, Stefano Stabellini wrote:
> On Wed, 9 Dec 2015, Ian Campbell wrote:
> > XXX TBD No support for arm64 (or even 32-bit guest on arm64).
> > XXX In particular the handling of save/restore of VFP state doesn't
> > XXX even compile for arm32. I need to investigate the best way to
> > XXX reflect the differing possible VFB states in the save record.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/hvm.c                     | 167
> > +++++++++++++++++++++++++++++++++
> >  xen/include/public/arch-arm/hvm/save.h |  38 +++++++-
> >  2 files changed, 204 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> > index 5fd0753..3c59e63 100644
> > --- a/xen/arch/arm/hvm.c
> > +++ b/xen/arch/arm/hvm.c
> > @@ -7,6 +7,7 @@
> >  
> >  #include <xsm/xsm.h>
> >  
> > +#include <xen/hvm/save.h>
> >  #include <public/xen.h>
> >  #include <public/hvm/params.h>
> >  #include <public/hvm/hvm_op.h>
> > @@ -65,3 +66,169 @@ long do_hvm_op(unsigned long op,
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >  
> >      return rc;
> >  }
> > +
> > +static int cpu_save(struct domain *d, hvm_domain_context_t *h)
> > +{
> > +    struct hvm_hw_cpu ctxt;
> > +    struct vcpu *v;
> 
> I wonder whether we could refactor the code to share the same underlying
> function between cpu_save and ctxt_switch_from.

One is writing to a public ABI struct (or at least a semi-public ABI) while
the other is completely internal. Other than exposing the internal struct
or use of MACROS I'm not sure it would be possible.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH RFC XEN v1 08/14] xen: arm: Save and restore arch timer state.

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/vtimer.c                  | 72 ++++++++++++++++++++++++++++++++++
 xen/include/public/arch-arm/hvm/save.h | 15 ++++++-
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 629feb4..9dfc699 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -22,6 +22,7 @@
 #include <xen/timer.h>
 #include <xen/sched.h>
 #include <xen/perfc.h>
+#include <xen/hvm/save.h>
 #include <asm/div64.h>
 #include <asm/irq.h>
 #include <asm/time.h>
@@ -355,6 +356,77 @@ int vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr)
     }
 }
 
+static int timer_save_one(hvm_domain_context_t *h,
+                          struct vcpu *v, struct vtimer *t,
+                          int type, uint64_t offset)
+{
+   struct hvm_hw_timer ctxt;
+
+   ctxt.cval = t->cval;
+   ctxt.ctl = t->ctl;
+   ctxt.vtb_offset = offset;
+   ctxt.type = type;
+   if ( hvm_save_entry(A15_TIMER, v->vcpu_id, h, &ctxt) != 0 )
+       return 1;
+   return 0;
+}
+
+static int timer_save(struct domain *d, hvm_domain_context_t *h)
+{
+    struct vcpu *v;
+
+    /* Save the state of vtimer and ptimer */
+    for_each_vcpu( d, v )
+    {
+        timer_save_one(h, v, &v->arch.phys_timer,
+                       HVM_SAVE_TIMER_TYPE_PHYS, d->arch.phys_timer_base.offset);
+        timer_save_one(h, v, &v->arch.virt_timer,
+                       HVM_SAVE_TIMER_TYPE_VIRT, d->arch.virt_timer_base.offset);
+    }
+    return 0;
+}
+
+static int timer_load(struct domain *d, hvm_domain_context_t *h)
+{
+    int vcpuid;
+    struct hvm_hw_timer ctxt;
+    struct vcpu *v;
+    struct vtimer *t = NULL;
+
+    /* Which vcpu is this? */
+    vcpuid = hvm_load_instance(h);
+
+    if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
+    {
+        dprintk(XENLOG_G_ERR, "HVM restore: dom%u has no vcpu%u\n",
+                d->domain_id, vcpuid);
+        return -EINVAL;
+    }
+
+    if ( hvm_load_entry(A15_TIMER, h, &ctxt) != 0 )
+        return -EINVAL;
+
+    if ( ctxt.type == HVM_SAVE_TIMER_TYPE_VIRT )
+    {
+        t = &v->arch.virt_timer;
+        d->arch.virt_timer_base.offset = ctxt.vtb_offset;
+
+    }
+    else
+    {
+        t = &v->arch.phys_timer;
+        d->arch.phys_timer_base.offset = ctxt.vtb_offset;
+    }
+
+    t->cval = ctxt.cval;
+    t->ctl = ctxt.ctl;
+    t->v = v;
+
+    return 0;
+}
+
+HVM_REGISTER_SAVE_RESTORE(A15_TIMER, timer_save, timer_load, 2, HVMSR_PER_VCPU);
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h
index 72474e5..7b92c9c 100644
--- a/xen/include/public/arch-arm/hvm/save.h
+++ b/xen/include/public/arch-arm/hvm/save.h
@@ -80,10 +80,23 @@ struct hvm_hw_cpu
 
 DECLARE_HVM_SAVE_TYPE(VCPU, 2, struct hvm_hw_cpu);
 
+#define HVM_SAVE_TIMER_TYPE_VIRT 0
+#define HVM_SAVE_TIMER_TYPE_PHYS 1
+
+struct hvm_hw_timer
+{
+    uint64_t vtb_offset; /* XXX Should be abs time since guest booted */
+    uint32_t ctl;
+    uint64_t cval;
+    uint32_t type;
+};
+
+DECLARE_HVM_SAVE_TYPE(A15_TIMER, 3, struct hvm_hw_timer);
+
 /*
  * Largest type-code in use
  */
-#define HVM_SAVE_CODE_MAX 2
+#define HVM_SAVE_CODE_MAX 3
 
 #endif
 
-- 
2.6.1

Re: [PATCH RFC XEN v1 08/14] xen: arm: Save and restore arch timer state.

[Stefano Stabellini]

On Wed, 9 Dec 2015, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/vtimer.c                  | 72 ++++++++++++++++++++++++++++++++++
>  xen/include/public/arch-arm/hvm/save.h | 15 ++++++-
>  2 files changed, 86 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index 629feb4..9dfc699 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -22,6 +22,7 @@
>  #include <xen/timer.h>
>  #include <xen/sched.h>
>  #include <xen/perfc.h>
> +#include <xen/hvm/save.h>
>  #include <asm/div64.h>
>  #include <asm/irq.h>
>  #include <asm/time.h>
> @@ -355,6 +356,77 @@ int vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr)
>      }
>  }
>  
> +static int timer_save_one(hvm_domain_context_t *h,
> +                          struct vcpu *v, struct vtimer *t,
> +                          int type, uint64_t offset)
> +{
> +   struct hvm_hw_timer ctxt;
> +
> +   ctxt.cval = t->cval;
> +   ctxt.ctl = t->ctl;
> +   ctxt.vtb_offset = offset;
> +   ctxt.type = type;
> +   if ( hvm_save_entry(A15_TIMER, v->vcpu_id, h, &ctxt) != 0 )
> +       return 1;
> +   return 0;
> +}
> +
> +static int timer_save(struct domain *d, hvm_domain_context_t *h)
> +{
> +    struct vcpu *v;
> +
> +    /* Save the state of vtimer and ptimer */
> +    for_each_vcpu( d, v )
> +    {
> +        timer_save_one(h, v, &v->arch.phys_timer,
> +                       HVM_SAVE_TIMER_TYPE_PHYS, d->arch.phys_timer_base.offset);
> +        timer_save_one(h, v, &v->arch.virt_timer,
> +                       HVM_SAVE_TIMER_TYPE_VIRT, d->arch.virt_timer_base.offset);
> +    }

I don't think we should save phys_timer_base.offset and
virt_timer_base.offset: they represent host specific offsets. I think we
need to save:

phys_timer:
    ctxt.offset = NOW() - d->arch.phys_timer_base.offset;
virt_timer:
    ctxt.offset = READ_SYSREG64(CNTPCT_EL0) - d->arch.virt_timer_base.offset


> +    return 0;
> +}
> +
> +static int timer_load(struct domain *d, hvm_domain_context_t *h)
> +{
> +    int vcpuid;
> +    struct hvm_hw_timer ctxt;
> +    struct vcpu *v;
> +    struct vtimer *t = NULL;
> +
> +    /* Which vcpu is this? */
> +    vcpuid = hvm_load_instance(h);
> +
> +    if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
> +    {
> +        dprintk(XENLOG_G_ERR, "HVM restore: dom%u has no vcpu%u\n",
> +                d->domain_id, vcpuid);
> +        return -EINVAL;
> +    }
> +
> +    if ( hvm_load_entry(A15_TIMER, h, &ctxt) != 0 )
> +        return -EINVAL;
> +
> +    if ( ctxt.type == HVM_SAVE_TIMER_TYPE_VIRT )
> +    {
> +        t = &v->arch.virt_timer;
> +        d->arch.virt_timer_base.offset = ctxt.vtb_offset;
> +
> +    }
> +    else
> +    {
> +        t = &v->arch.phys_timer;
> +        d->arch.phys_timer_base.offset = ctxt.vtb_offset;
> +    }

The setting of phys_timer_base.offset and virt_timer_base.offset look
wrong. I think they should be:

virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0) - ctxt.offset;
phys_timer_base.offset = NOW() - ctxt.offset;

You can test this by rebooting the physical machine between save and
restore.


> +    t->cval = ctxt.cval;
> +    t->ctl = ctxt.ctl;
> +    t->v = v;
> +
> +    return 0;
> +}
> +
> +HVM_REGISTER_SAVE_RESTORE(A15_TIMER, timer_save, timer_load, 2, HVMSR_PER_VCPU);
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h
> index 72474e5..7b92c9c 100644
> --- a/xen/include/public/arch-arm/hvm/save.h
> +++ b/xen/include/public/arch-arm/hvm/save.h
> @@ -80,10 +80,23 @@ struct hvm_hw_cpu
>  
>  DECLARE_HVM_SAVE_TYPE(VCPU, 2, struct hvm_hw_cpu);
>  
> +#define HVM_SAVE_TIMER_TYPE_VIRT 0
> +#define HVM_SAVE_TIMER_TYPE_PHYS 1
> +
> +struct hvm_hw_timer
> +{
> +    uint64_t vtb_offset; /* XXX Should be abs time since guest booted */

I would call this simply "offset" with a comment


> +    uint32_t ctl;
> +    uint64_t cval;
> +    uint32_t type;

Isn't a uint32_t a bit too much for one bit?
Don't we need to save and restore the PPIs too?


> +};
> +
> +DECLARE_HVM_SAVE_TYPE(A15_TIMER, 3, struct hvm_hw_timer);
> +
>  /*
>   * Largest type-code in use
>   */
> -#define HVM_SAVE_CODE_MAX 2
> +#define HVM_SAVE_CODE_MAX 3
>  
>  #endif
>  
> -- 
> 2.6.1
> 

Re: [PATCH RFC XEN v1 08/14] xen: arm: Save and restore arch timer state.

[Ian Campbell]

On Wed, 2015-12-16 at 15:53 +0000, Stefano Stabellini wrote:
> On Wed, 9 Dec 2015, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/vtimer.c                  | 72
> > ++++++++++++++++++++++++++++++++++
> >  xen/include/public/arch-arm/hvm/save.h | 15 ++++++-
> >  2 files changed, 86 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> > index 629feb4..9dfc699 100644
> > --- a/xen/arch/arm/vtimer.c
> > +++ b/xen/arch/arm/vtimer.c
> > @@ -22,6 +22,7 @@
> >  #include <xen/timer.h>
> >  #include <xen/sched.h>
> >  #include <xen/perfc.h>
> > +#include <xen/hvm/save.h>
> >  #include <asm/div64.h>
> >  #include <asm/irq.h>
> >  #include <asm/time.h>
> > @@ -355,6 +356,77 @@ int vtimer_emulate(struct cpu_user_regs *regs,
> > union hsr hsr)
> >      }
> >  }
> >  
> > +static int timer_save_one(hvm_domain_context_t *h,
> > +                          struct vcpu *v, struct vtimer *t,
> > +                          int type, uint64_t offset)
> > +{
> > +   struct hvm_hw_timer ctxt;
> > +
> > +   ctxt.cval = t->cval;
> > +   ctxt.ctl = t->ctl;
> > +   ctxt.vtb_offset = offset;
> > +   ctxt.type = type;
> > +   if ( hvm_save_entry(A15_TIMER, v->vcpu_id, h, &ctxt) != 0 )
> > +       return 1;
> > +   return 0;
> > +}
> > +
> > +static int timer_save(struct domain *d, hvm_domain_context_t *h)
> > +{
> > +    struct vcpu *v;
> > +
> > +    /* Save the state of vtimer and ptimer */
> > +    for_each_vcpu( d, v )
> > +    {
> > +        timer_save_one(h, v, &v->arch.phys_timer,
> > +                       HVM_SAVE_TIMER_TYPE_PHYS, d-
> > >arch.phys_timer_base.offset);
> > +        timer_save_one(h, v, &v->arch.virt_timer,
> > +                       HVM_SAVE_TIMER_TYPE_VIRT, d-
> > >arch.virt_timer_base.offset);
> > +    }
> 
> I don't think we should save phys_timer_base.offset and
> virt_timer_base.offset: they represent host specific offsets. I think we
> need to save:
> 
> phys_timer:
>     ctxt.offset = NOW() - d->arch.phys_timer_base.offset;
> virt_timer:
>     ctxt.offset = READ_SYSREG64(CNTPCT_EL0) - d-
> >arch.virt_timer_base.offset

Yes, I meant to come back to this (and the restore side) with a bigger
thinking cap on and forgot.
 
> > +#define HVM_SAVE_TIMER_TYPE_VIRT 0
> > +#define HVM_SAVE_TIMER_TYPE_PHYS 1
> > +
> > +struct hvm_hw_timer
> > +{
> > +    uint64_t vtb_offset; /* XXX Should be abs time since guest booted
> > */
> 
> I would call this simply "offset" with a comment

OK. The comment here was supposed to remind me about the bigger thinking
cap. Oops.

> > +    uint32_t ctl;
> > +    uint64_t cval;
> > +    uint32_t type;
> 
> Isn't a uint32_t a bit too much for one bit?
> Don't we need to save and restore the PPIs too?

This is an ABI, so I wanted to avoid bit fields etc, I could make it a
uint8_t but then array alignment gets interesting. Really I guess this is
actually a uint8_t and 3 bytes of padding.

I don't think the save record format has density of the information as a
particularly high priority though.

The PPI state is saved by the GIC. Perhaps we should save the actual number
used by the guest here though.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH RFC XEN v1 08/14] xen: arm: Save and restore arch timer state.

[Julien Grall]

Hi,

On 16/12/15 16:02, Ian Campbell wrote:
> The PPI state is saved by the GIC. Perhaps we should save the actual number
> used by the guest here though.

I think we have to save the actual number as we don't guarantee that the
interrupt (and memory layout BTW) for the guest won't change between 2
versions of Xen.

Regards,

-- 
Julien Grall

Re: [PATCH RFC XEN v1 08/14] xen: arm: Save and restore arch timer state.

[Ian Campbell]

On Wed, 2015-12-16 at 16:17 +0000, Julien Grall wrote:
> Hi,
> 
> On 16/12/15 16:02, Ian Campbell wrote:
> > The PPI state is saved by the GIC. Perhaps we should save the actual
> > number
> > used by the guest here though.
> 
> I think we have to save the actual number as we don't guarantee that the
> interrupt (and memory layout BTW) for the guest won't change between 2
> versions of Xen.

Yes. As with other things which are saved here that are currently platform
constants the restore side will become a check against the hardcoded number
for now (i.e. no compat code until it becomes needed),

Ian.

Re: [PATCH RFC XEN v1 08/14] xen: arm: Save and restore arch timer state.

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3664 bytes --]

On Wed, 16 Dec 2015, Ian Campbell wrote:
> On Wed, 2015-12-16 at 15:53 +0000, Stefano Stabellini wrote:
> > On Wed, 9 Dec 2015, Ian Campbell wrote:
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > ---
> > >  xen/arch/arm/vtimer.c                  | 72
> > > ++++++++++++++++++++++++++++++++++
> > >  xen/include/public/arch-arm/hvm/save.h | 15 ++++++-
> > >  2 files changed, 86 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> > > index 629feb4..9dfc699 100644
> > > --- a/xen/arch/arm/vtimer.c
> > > +++ b/xen/arch/arm/vtimer.c
> > > @@ -22,6 +22,7 @@
> > >  #include <xen/timer.h>
> > >  #include <xen/sched.h>
> > >  #include <xen/perfc.h>
> > > +#include <xen/hvm/save.h>
> > >  #include <asm/div64.h>
> > >  #include <asm/irq.h>
> > >  #include <asm/time.h>
> > > @@ -355,6 +356,77 @@ int vtimer_emulate(struct cpu_user_regs *regs,
> > > union hsr hsr)
> > >      }
> > >  }
> > >  
> > > +static int timer_save_one(hvm_domain_context_t *h,
> > > +                          struct vcpu *v, struct vtimer *t,
> > > +                          int type, uint64_t offset)
> > > +{
> > > +   struct hvm_hw_timer ctxt;
> > > +
> > > +   ctxt.cval = t->cval;
> > > +   ctxt.ctl = t->ctl;
> > > +   ctxt.vtb_offset = offset;
> > > +   ctxt.type = type;
> > > +   if ( hvm_save_entry(A15_TIMER, v->vcpu_id, h, &ctxt) != 0 )
> > > +       return 1;
> > > +   return 0;
> > > +}
> > > +
> > > +static int timer_save(struct domain *d, hvm_domain_context_t *h)
> > > +{
> > > +    struct vcpu *v;
> > > +
> > > +    /* Save the state of vtimer and ptimer */
> > > +    for_each_vcpu( d, v )
> > > +    {
> > > +        timer_save_one(h, v, &v->arch.phys_timer,
> > > +                       HVM_SAVE_TIMER_TYPE_PHYS, d-
> > > >arch.phys_timer_base.offset);
> > > +        timer_save_one(h, v, &v->arch.virt_timer,
> > > +                       HVM_SAVE_TIMER_TYPE_VIRT, d-
> > > >arch.virt_timer_base.offset);
> > > +    }
> >
> > I don't think we should save phys_timer_base.offset and
> > virt_timer_base.offset: they represent host specific offsets. I think we
> > need to save:
> >
> > phys_timer:
> >     ctxt.offset = NOW() - d->arch.phys_timer_base.offset;
> > virt_timer:
> >     ctxt.offset = READ_SYSREG64(CNTPCT_EL0) - d-
> > >arch.virt_timer_base.offset
>
> Yes, I meant to come back to this (and the restore side) with a bigger
> thinking cap on and forgot.
>  
> > > +#define HVM_SAVE_TIMER_TYPE_VIRT 0
> > > +#define HVM_SAVE_TIMER_TYPE_PHYS 1
> > > +
> > > +struct hvm_hw_timer
> > > +{
> > > +    uint64_t vtb_offset; /* XXX Should be abs time since guest booted
> > > */
> >
> > I would call this simply "offset" with a comment
>
> OK. The comment here was supposed to remind me about the bigger thinking
> cap. Oops.
>
> > > +    uint32_t ctl;
> > > +    uint64_t cval;
> > > +    uint32_t type;
> >
> > Isn't a uint32_t a bit too much for one bit?
> > Don't we need to save and restore the PPIs too?
>
> This is an ABI, so I wanted to avoid bit fields etc, I could make it a
> uint8_t but then array alignment gets interesting. Really I guess this is
> actually a uint8_t and 3 bytes of padding.

Having to deal with types != from uint32_t and uint64_t is unavoidable.
In fact other structs used in save/restore in other patches of this
series already use uint8_t.

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH RFC XEN v1 08/14] xen: arm: Save and restore arch timer state.

[Ian Campbell]

On Wed, 2015-12-16 at 18:05 +0000, Stefano Stabellini wrote:
> On Wed, 16 Dec 2015, Ian Campbell wrote:
> > On Wed, 2015-12-16 at 15:53 +0000, Stefano Stabellini wrote:
> > > On Wed, 9 Dec 2015, Ian Campbell wrote:
> > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > > ---
> > > >  xen/arch/arm/vtimer.c                  | 72
> > > > ++++++++++++++++++++++++++++++++++
> > > >  xen/include/public/arch-arm/hvm/save.h | 15 ++++++-
> > > >  2 files changed, 86 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> > > > index 629feb4..9dfc699 100644
> > > > --- a/xen/arch/arm/vtimer.c
> > > > +++ b/xen/arch/arm/vtimer.c
> > > > @@ -22,6 +22,7 @@
> > > >  #include <xen/timer.h>
> > > >  #include <xen/sched.h>
> > > >  #include <xen/perfc.h>
> > > > +#include <xen/hvm/save.h>
> > > >  #include <asm/div64.h>
> > > >  #include <asm/irq.h>
> > > >  #include <asm/time.h>
> > > > @@ -355,6 +356,77 @@ int vtimer_emulate(struct cpu_user_regs *regs,
> > > > union hsr hsr)
> > > >      }
> > > >  }
> > > >  
> > > > +static int timer_save_one(hvm_domain_context_t *h,
> > > > +                          struct vcpu *v, struct vtimer *t,
> > > > +                          int type, uint64_t offset)
> > > > +{
> > > > +   struct hvm_hw_timer ctxt;
> > > > +
> > > > +   ctxt.cval = t->cval;
> > > > +   ctxt.ctl = t->ctl;
> > > > +   ctxt.vtb_offset = offset;
> > > > +   ctxt.type = type;
> > > > +   if ( hvm_save_entry(A15_TIMER, v->vcpu_id, h, &ctxt) != 0 )
> > > > +       return 1;
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +static int timer_save(struct domain *d, hvm_domain_context_t *h)
> > > > +{
> > > > +    struct vcpu *v;
> > > > +
> > > > +    /* Save the state of vtimer and ptimer */
> > > > +    for_each_vcpu( d, v )
> > > > +    {
> > > > +        timer_save_one(h, v, &v->arch.phys_timer,
> > > > +                       HVM_SAVE_TIMER_TYPE_PHYS, d-
> > > > > arch.phys_timer_base.offset);
> > > > +        timer_save_one(h, v, &v->arch.virt_timer,
> > > > +                       HVM_SAVE_TIMER_TYPE_VIRT, d-
> > > > > arch.virt_timer_base.offset);
> > > > +    }
> > > 
> > > I don't think we should save phys_timer_base.offset and
> > > virt_timer_base.offset: they represent host specific offsets. I think
> > > we
> > > need to save:
> > > 
> > > phys_timer:
> > >     ctxt.offset = NOW() - d->arch.phys_timer_base.offset;
> > > virt_timer:
> > >     ctxt.offset = READ_SYSREG64(CNTPCT_EL0) - d-
> > > > arch.virt_timer_base.offset
> > 
> > Yes, I meant to come back to this (and the restore side) with a bigger
> > thinking cap on and forgot.
> >  
> > > > +#define HVM_SAVE_TIMER_TYPE_VIRT 0
> > > > +#define HVM_SAVE_TIMER_TYPE_PHYS 1
> > > > +
> > > > +struct hvm_hw_timer
> > > > +{
> > > > +    uint64_t vtb_offset; /* XXX Should be abs time since guest
> > > > booted
> > > > */
> > > 
> > > I would call this simply "offset" with a comment
> > 
> > OK. The comment here was supposed to remind me about the bigger
> > thinking
> > cap. Oops.
> > 
> > > > +    uint32_t ctl;
> > > > +    uint64_t cval;
> > > > +    uint32_t type;
> > > 
> > > Isn't a uint32_t a bit too much for one bit?
> > > Don't we need to save and restore the PPIs too?
> > 
> > This is an ABI, so I wanted to avoid bit fields etc, I could make it a
> > uint8_t but then array alignment gets interesting. Really I guess this
> > is
> > actually a uint8_t and 3 bytes of padding.
> 
> Having to deal with types != from uint32_t and uint64_t is unavoidable.
> In fact other structs used in save/restore in other patches of this
> series already use uint8_t.

Right, I just don't really see a whole lot of point from an ABI point of
view right now, unless you are arguing against padding the struct to an 4
byte aligned size?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH RFC XEN v1 09/14] xen: arm: Save and restore GIC state.

[Ian Campbell]

Currently only GICv2 support is implemented.

Given the differing architectural state between the GICv2 and v3 I
ended up with separate save records. I'm not sure if this is the best
plan. I have also split the state into GICD (per domain) and GICC (per
vcpu, although also including banked GICD state).

There are some restrictions on the guest behaviour (since PV suspend
is cooperative this is OK). These are added to arch-arm.h.

The primary requirement is that there be no active interrupts, which
can be achieved on the guest side with some sort of CPU rendezvous
with IRQs disabled (as is done in Linux's stop_machine framework).

It is already a feature of the PV suspend protocol that the event
channel state is not saved and the guest is expected to rebind any
event channels, therefore losing a pending evtchn upcall is harmless.

Right now there is no support for SPIs at all, but this is OK since
such things are only exposed to guests via passthrough, which is
incompatible with migration.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/vgic-v2.c                 | 135 +++++++++++++++++++++++++++++++++
 xen/arch/arm/vgic-v3.c                 |  35 +++++++++
 xen/arch/arm/vgic.c                    | 123 ++++++++++++++++++++++++++++++
 xen/include/asm-arm/vgic.h             |   5 ++
 xen/include/public/arch-arm.h          |  13 ++++
 xen/include/public/arch-arm/hvm/save.h |  43 ++++++++++-
 6 files changed, 353 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 2c73133..e02edcf 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -25,6 +25,7 @@
 #include <xen/irq.h>
 #include <xen/sched.h>
 #include <xen/sizes.h>
+#include <xen/hvm/save.h>
 
 #include <asm/current.h>
 
@@ -721,6 +722,140 @@ int vgic_v2_init(struct domain *d)
     return 0;
 }
 
+
+static int gic_v2_gicd_save(struct domain *d, hvm_domain_context_t *h)
+{
+    struct hvm_hw_gic_v2_gicd ctxt;
+
+    if ( d->arch.vgic.version != GIC_V2 )
+        return 0;
+
+    if ( d->arch.vgic.nr_spis )
+        return -ENOSYS;
+
+    ctxt.ctlr = d->arch.vgic.ctlr;
+
+    if ( hvm_save_entry(GICV2_GICD, 0, h, &ctxt) != 0 )
+        return -EFAULT;
+
+    return 0;
+}
+
+static int gic_v2_gicd_load(struct domain *d, hvm_domain_context_t *h)
+{
+    struct hvm_hw_gic_v2_gicd ctxt;
+
+    if ( d->arch.vgic.version != GIC_V2 )
+    {
+        dprintk(XENLOG_G_ERR,
+            "HVM%d restore: Cannot restore GIC v2 state into domain with %s\n",
+            d->domain_id, gic_hw_desc(d->arch.vgic.version));
+        return -EINVAL;
+    }
+
+    if ( hvm_load_entry(GICV2_GICD, h, &ctxt) != 0 )
+        return -EINVAL;
+
+    d->arch.vgic.ctlr = ctxt.ctlr;
+
+    return 0;
+}
+
+HVM_REGISTER_SAVE_RESTORE(GICV2_GICD,
+                          gic_v2_gicd_save, gic_v2_gicd_load,
+                          1, HVMSR_PER_DOM);
+
+static int gic_v2_gicc_save(struct domain *d, hvm_domain_context_t *h)
+{
+    struct hvm_hw_gic_v2_gicc ctxt;
+    struct vcpu *v;
+    int rc, i;
+
+    if ( d->arch.vgic.version != GIC_V2 )
+        return 0;
+
+    /* Save the state of GICs */
+    for_each_vcpu( d, v )
+    {
+        ctxt.vmcr = v->arch.gic.v2.vmcr;
+        ctxt.apr = v->arch.gic.v2.apr;
+
+        /* Save PPI and SGI states (per-CPU) */
+        spin_lock(&v->arch.vgic.lock);
+        for ( i = 0; i < NR_GIC_LOCAL_IRQS ; i++ )
+        {
+            struct hvm_hw_irq *s = &ctxt.local_irqs[i];
+
+            if ( (rc = vgic_irq_save_core(v, i, s)) < 0 )
+            {
+                spin_unlock(&v->arch.vgic.lock);
+                goto err;
+            }
+        }
+        spin_unlock(&v->arch.vgic.lock);
+
+        if ( (rc = hvm_save_entry(GICV2_GICC, v->vcpu_id, h, &ctxt)) != 0 )
+            goto err;
+    }
+
+    rc = 0;
+err:
+    return rc;
+}
+
+static int gic_v2_gicc_load(struct domain *d, hvm_domain_context_t *h)
+{
+    struct hvm_hw_gic_v2_gicc ctxt;
+    int vcpuid;
+    struct vcpu *v;
+    int i, rc;
+
+    if ( d->arch.vgic.version != GIC_V2 )
+    {
+        dprintk(XENLOG_G_ERR,
+                "HVM%d restore: Cannot restore GIC v2 state into domain using gic %d\n",
+                d->domain_id, d->arch.vgic.version);
+        return -EINVAL;
+    }
+
+    /* Which vcpu is this? */
+    vcpuid = hvm_load_instance(h);
+    if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
+    {
+        dprintk(XENLOG_G_ERR, "HVM restore: dom%u has no vcpu%u\n",
+                d->domain_id, vcpuid);
+        return -EINVAL;
+    }
+
+    if ( hvm_load_entry(GICV2_GICC, h, &ctxt) != 0 )
+        return -EINVAL;
+
+    v->arch.gic.v2.vmcr = ctxt.vmcr;
+    v->arch.gic.v2.apr = ctxt.apr;
+
+    /* Restore PPI and SGI states (per-CPU) */
+    spin_lock(&v->arch.vgic.lock);
+
+    for ( i = 0; i < NR_GIC_LOCAL_IRQS ; i++ )
+    {
+        struct hvm_hw_irq *s = &ctxt.local_irqs[i];
+
+        ASSERT(s->irq == i);
+
+        if ( (rc = vgic_irq_restore_core(v, s)) < 0 )
+            goto err;
+    }
+
+    gic_dump_info(v);
+
+err:
+    spin_unlock(&v->arch.vgic.lock);
+    return rc;
+}
+
+HVM_REGISTER_SAVE_RESTORE(GICV2_GICC, gic_v2_gicc_save, gic_v2_gicc_load,
+                          1, HVMSR_PER_VCPU);
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 985e866..af418ae 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -26,6 +26,7 @@
 #include <xen/irq.h>
 #include <xen/sched.h>
 #include <xen/sizes.h>
+#include <xen/hvm/save.h>
 #include <asm/current.h>
 #include <asm/mmio.h>
 #include <asm/gic_v3_defs.h>
@@ -1495,6 +1496,40 @@ int vgic_v3_init(struct domain *d)
     return 0;
 }
 
+
+static int gic_v3_save(struct domain *d, hvm_domain_context_t *h)
+{
+    if ( d->arch.vgic.version != GIC_V3 )
+        return 0;
+
+    if ( d->arch.vgic.nr_spis )
+        return -ENOSYS;
+
+    dprintk(XENLOG_G_ERR, "HVM%d save: Cannot save GIC v3\n",
+            d->domain_id);
+
+    return -ENOSYS;
+
+}
+
+static int gic_v3_load(struct domain *d, hvm_domain_context_t *h)
+{
+    if ( d->arch.vgic.version != GIC_V3 )
+    {
+        dprintk(XENLOG_G_ERR,
+            "HVM%d restore: Cannot restore GIC v3 state into domain with %s\n",
+            d->domain_id, gic_hw_desc(d->arch.vgic.version));
+        return -EINVAL;
+    }
+
+    dprintk(XENLOG_G_ERR, "HVM%d restore: Cannot restore GIC v3\n",
+            d->domain_id);
+
+    return -ENOSYS;
+}
+
+HVM_REGISTER_SAVE_RESTORE(GICV3, gic_v3_save, gic_v3_load, 1, HVMSR_PER_VCPU);
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 531ce5d..b3cebac 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -25,6 +25,7 @@
 #include <xen/irq.h>
 #include <xen/sched.h>
 #include <xen/perfc.h>
+#include <xen/hvm/save.h>
 
 #include <asm/current.h>
 
@@ -570,6 +571,128 @@ void vgic_free_virq(struct domain *d, unsigned int virq)
     clear_bit(virq, d->arch.vgic.allocated_irqs);
 }
 
+int vgic_irq_save_core(struct vcpu *v, unsigned irq, struct hvm_hw_irq *s)
+{
+    struct pending_irq *p = irq_to_pending(v, irq);
+    struct vgic_irq_rank *rank = vgic_rank_irq(v, p->irq);
+
+    const bool enabled = test_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
+    const bool queued = test_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
+    const bool visible = test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
+
+    spin_lock(&rank->lock);
+
+    ASSERT(p->irq < 32); /* SPI not supported yet */
+
+    s->irq = p->irq;
+    s->state = 0;
+    s->priority = rank->priority[p->irq % 32];
+
+    if ( enabled )
+        s->state |= HVM_HW_IRQ_STATE_ENABLED;
+
+    /*
+     * If it is queued then it is pending as far as the guest is
+     * concerned, even if we've not managed to find an LR to expose
+     * this.
+     */
+    if ( queued )
+        s->state |= HVM_HW_IRQ_STATE_PENDING;
+
+    /*
+     * If the IRQ is visible in an LR then there may be state which
+     * p->status is not aware of.
+     */
+    if ( visible )
+    {
+        struct gic_lr lr;
+
+        gic_vcpu_saved_lr(v, p->lr, &lr);
+
+        if ( lr.state & GICH_LR_PENDING )
+            s->state |= HVM_HW_IRQ_STATE_PENDING;
+
+        /*
+         * HVM_HW_IRQ_STATE_ACTIVE: We currently do not handle save/restore
+         * with active interrupts.
+         */
+        if ( lr.state & GICH_LR_ACTIVE )
+        {
+            dprintk(XENLOG_G_ERR,
+                    "HVM%d save: Cannot save active local IRQ%u\n",
+                    v->domain->domain_id, irq);
+            return -EINVAL;
+        }
+    }
+
+    /*
+     * HVM_HW_IRQ_STATE_EDGE: we implement ICFG[0] and [1] as RAZ/WI.
+     */
+
+    spin_unlock(&rank->lock);
+
+    return 0;
+}
+
+int vgic_irq_restore_core(struct vcpu *v, struct hvm_hw_irq *s)
+{
+    struct pending_irq *p = irq_to_pending(v, s->irq);
+    struct vgic_irq_rank *rank = vgic_rank_irq(v, s->irq);
+    bool reraise = 0;
+
+    /*
+     * Level/Edge, we implement ICFG for SGI/PPI as RAZ/WI, so this bit cannot
+     * be set.
+     */
+    if ( s->state & HVM_HW_IRQ_STATE_EDGE )
+    {
+        dprintk(XENLOG_G_ERR,
+                "HVM%d restore: Cannot restore an edge triggered local IRQ%u\n",
+                v->domain->domain_id, s->irq);
+        return -EINVAL;
+    }
+
+    if ( s->state & HVM_HW_IRQ_STATE_ACTIVE )
+    {
+        dprintk(XENLOG_G_ERR,
+                "HVM%d restore: Cannot restore active local IRQ%u\n",
+                v->domain->domain_id, s->irq);
+        return -EINVAL;
+    }
+
+    spin_lock(&rank->lock);
+
+    ASSERT(s->irq < 32); /* SPI not supported yet */
+
+    rank->priority[s->irq % 32] = s->priority;
+
+    if ( s->state & HVM_HW_IRQ_STATE_ENABLED )
+        set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
+
+    /* If the IRQ was either PENDING or ACTIVE, then QUEUE it to be reinjected. */
+    if ( s->state & (HVM_HW_IRQ_STATE_PENDING|HVM_HW_IRQ_STATE_ACTIVE) )
+    {
+        reraise = 1;
+        set_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
+    }
+
+    /* This will check for enabled etc */
+    if ( reraise ) /* v != current, so will add to lr_pending */
+    {
+        /*
+         * This uses the current IPRIORITYR, which may differ from
+         * when the IRQ was actually made pending. h/w spec probably
+         * allows this, XXXX check
+         */
+        gic_raise_guest_irq(v, s->irq, s->priority);
+    }
+
+    spin_unlock(&rank->lock);
+
+    return 0;
+}
+
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 005f822..bfa9c60 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -333,6 +333,11 @@ static inline int vgic_allocate_spi(struct domain *d)
 
 extern void vgic_free_virq(struct domain *d, unsigned int virq);
 
+struct hvm_hw_irq;
+extern int vgic_irq_save_core(struct vcpu *v, unsigned irq,
+                              struct hvm_hw_irq *s);
+extern int vgic_irq_restore_core(struct vcpu *v, struct hvm_hw_irq *s);
+
 void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
                       paddr_t vbase, uint32_t aliased_offset);
 
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 6322548..8df80ca 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -163,6 +163,19 @@
  *   at Documentation/devicetree/bindings/arm/xen.txt.
  */
 
+/*
+ * Requirements for Save/Restore/Migrate.
+ *
+ * When we are suspending we require the VM to have quiesced itself
+ * before calling HYPERVISOR_suspend(). This means:
+ *
+ * - There must be no active interrupt (SGI, PPI or SPI).
+ * - All VCPUs must have interrupts masked.
+ *
+ * Upon restore any event channel upcall (via the event channel PPI)
+ * which was pending upon save will be lost.
+ */
+
 #define XEN_HYPERCALL_TAG   0XEA1
 
 #define  int64_aligned_t  int64_t __attribute__((aligned(8)))
diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h
index 7b92c9c..db916b1 100644
--- a/xen/include/public/arch-arm/hvm/save.h
+++ b/xen/include/public/arch-arm/hvm/save.h
@@ -93,10 +93,51 @@ struct hvm_hw_timer
 
 DECLARE_HVM_SAVE_TYPE(A15_TIMER, 3, struct hvm_hw_timer);
 
+
+/* Domain global GICD state */
+struct hvm_hw_gic_v2_gicd
+{
+    uint32_t ctlr;
+};
+DECLARE_HVM_SAVE_TYPE(GICV2_GICD, 4, struct hvm_hw_gic_v2_gicd);
+
+struct hvm_hw_irq
+{
+    uint32_t irq;
+#define HVM_HW_IRQ_STATE_ENABLED (1UL << 0)
+#define HVM_HW_IRQ_STATE_PENDING (1UL << 1)
+#define HVM_HW_IRQ_STATE_ACTIVE  (1UL << 2)
+#define HVM_HW_IRQ_STATE_EDGE    (1UL << 3)
+    uint8_t state;
+    uint8_t priority;
+};
+
+/* Per-vcpu GICC state (and per-VCPU GICD, e.g. SGI+PPI) */
+struct hvm_hw_gic_v2_gicc
+{
+    /* GICC state */
+    uint32_t vmcr; /* GICC_{PMR,BPR,ABPR,CTLR} state */
+    uint32_t apr; /* Active priorities */
+
+    /*
+     * SGI + PPI state.
+     */
+    struct hvm_hw_irq local_irqs[32];
+};
+
+DECLARE_HVM_SAVE_TYPE(GICV2_GICC, 5, struct hvm_hw_gic_v2_gicc);
+
+struct hvm_hw_gic_v3
+{
+    /* TODO */
+};
+
+DECLARE_HVM_SAVE_TYPE(GICV3, 6, struct hvm_hw_gic_v3);
+
 /*
  * Largest type-code in use
  */
-#define HVM_SAVE_CODE_MAX 3
+#define HVM_SAVE_CODE_MAX 6
 
 #endif
 
-- 
2.6.1

Re: [PATCH RFC XEN v1 09/14] xen: arm: Save and restore GIC state.

[Stefano Stabellini]

On Wed, 9 Dec 2015, Ian Campbell wrote:
> Currently only GICv2 support is implemented.
> 
> Given the differing architectural state between the GICv2 and v3 I
> ended up with separate save records. I'm not sure if this is the best
> plan. I have also split the state into GICD (per domain) and GICC (per
> vcpu, although also including banked GICD state).

I think it makes sense to have a per-vcpu and a per-domain save/restore
function pair.


> There are some restrictions on the guest behaviour (since PV suspend
> is cooperative this is OK). These are added to arch-arm.h.
> 
> The primary requirement is that there be no active interrupts, which
> can be achieved on the guest side with some sort of CPU rendezvous
> with IRQs disabled (as is done in Linux's stop_machine framework).
> 
> It is already a feature of the PV suspend protocol that the event
> channel state is not saved and the guest is expected to rebind any
> event channels, therefore losing a pending evtchn upcall is harmless.
> 
> Right now there is no support for SPIs at all, but this is OK since
> such things are only exposed to guests via passthrough, which is
> incompatible with migration.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/vgic-v2.c                 | 135 +++++++++++++++++++++++++++++++++
>  xen/arch/arm/vgic-v3.c                 |  35 +++++++++
>  xen/arch/arm/vgic.c                    | 123 ++++++++++++++++++++++++++++++
>  xen/include/asm-arm/vgic.h             |   5 ++
>  xen/include/public/arch-arm.h          |  13 ++++
>  xen/include/public/arch-arm/hvm/save.h |  43 ++++++++++-
>  6 files changed, 353 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 2c73133..e02edcf 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -25,6 +25,7 @@
>  #include <xen/irq.h>
>  #include <xen/sched.h>
>  #include <xen/sizes.h>
> +#include <xen/hvm/save.h>
>  
>  #include <asm/current.h>
>  
> @@ -721,6 +722,140 @@ int vgic_v2_init(struct domain *d)
>      return 0;
>  }
>  
> +
> +static int gic_v2_gicd_save(struct domain *d, hvm_domain_context_t *h)
> +{
> +    struct hvm_hw_gic_v2_gicd ctxt;
> +
> +    if ( d->arch.vgic.version != GIC_V2 )
> +        return 0;
> +
> +    if ( d->arch.vgic.nr_spis )
> +        return -ENOSYS;
> +
> +    ctxt.ctlr = d->arch.vgic.ctlr;

Do we need to save/restore vgic.dbase?


> +    if ( hvm_save_entry(GICV2_GICD, 0, h, &ctxt) != 0 )
> +        return -EFAULT;
> +
> +    return 0;
> +}
> +
> +static int gic_v2_gicd_load(struct domain *d, hvm_domain_context_t *h)
> +{
> +    struct hvm_hw_gic_v2_gicd ctxt;
> +
> +    if ( d->arch.vgic.version != GIC_V2 )
> +    {
> +        dprintk(XENLOG_G_ERR,
> +            "HVM%d restore: Cannot restore GIC v2 state into domain with %s\n",
> +            d->domain_id, gic_hw_desc(d->arch.vgic.version));
> +        return -EINVAL;
> +    }
> +
> +    if ( hvm_load_entry(GICV2_GICD, h, &ctxt) != 0 )
> +        return -EINVAL;
> +
> +    d->arch.vgic.ctlr = ctxt.ctlr;
> +
> +    return 0;
> +}
> +
> +HVM_REGISTER_SAVE_RESTORE(GICV2_GICD,
> +                          gic_v2_gicd_save, gic_v2_gicd_load,
> +                          1, HVMSR_PER_DOM);
> +
> +static int gic_v2_gicc_save(struct domain *d, hvm_domain_context_t *h)
> +{
> +    struct hvm_hw_gic_v2_gicc ctxt;
> +    struct vcpu *v;
> +    int rc, i;
> +
> +    if ( d->arch.vgic.version != GIC_V2 )
> +        return 0;
> +
> +    /* Save the state of GICs */
> +    for_each_vcpu( d, v )
> +    {
> +        ctxt.vmcr = v->arch.gic.v2.vmcr;
> +        ctxt.apr = v->arch.gic.v2.apr;
> +
> +        /* Save PPI and SGI states (per-CPU) */
> +        spin_lock(&v->arch.vgic.lock);

vgic_lock

Is the domain already paused at this point? If so, maybe we could get
away without locking?


> +        for ( i = 0; i < NR_GIC_LOCAL_IRQS ; i++ )
> +        {
> +            struct hvm_hw_irq *s = &ctxt.local_irqs[i];
> +
> +            if ( (rc = vgic_irq_save_core(v, i, s)) < 0 )
> +            {
> +                spin_unlock(&v->arch.vgic.lock);
> +                goto err;
> +            }
> +        }
> +        spin_unlock(&v->arch.vgic.lock);
> +
> +        if ( (rc = hvm_save_entry(GICV2_GICC, v->vcpu_id, h, &ctxt)) != 0 )
> +            goto err;
> +    }
> +
> +    rc = 0;
> +err:
> +    return rc;
> +}
> +
> +static int gic_v2_gicc_load(struct domain *d, hvm_domain_context_t *h)
> +{
> +    struct hvm_hw_gic_v2_gicc ctxt;
> +    int vcpuid;
> +    struct vcpu *v;
> +    int i, rc;
> +
> +    if ( d->arch.vgic.version != GIC_V2 )
> +    {
> +        dprintk(XENLOG_G_ERR,
> +                "HVM%d restore: Cannot restore GIC v2 state into domain using gic %d\n",
> +                d->domain_id, d->arch.vgic.version);
> +        return -EINVAL;
> +    }
> +
> +    /* Which vcpu is this? */
> +    vcpuid = hvm_load_instance(h);
> +    if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
> +    {
> +        dprintk(XENLOG_G_ERR, "HVM restore: dom%u has no vcpu%u\n",
> +                d->domain_id, vcpuid);
> +        return -EINVAL;
> +    }
> +
> +    if ( hvm_load_entry(GICV2_GICC, h, &ctxt) != 0 )
> +        return -EINVAL;
> +
> +    v->arch.gic.v2.vmcr = ctxt.vmcr;
> +    v->arch.gic.v2.apr = ctxt.apr;
> +
> +    /* Restore PPI and SGI states (per-CPU) */
> +    spin_lock(&v->arch.vgic.lock);

same here


> +    for ( i = 0; i < NR_GIC_LOCAL_IRQS ; i++ )
> +    {
> +        struct hvm_hw_irq *s = &ctxt.local_irqs[i];
> +
> +        ASSERT(s->irq == i);
> +
> +        if ( (rc = vgic_irq_restore_core(v, s)) < 0 )
> +            goto err;
> +    }
> +
> +    gic_dump_info(v);
> +
> +err:
> +    spin_unlock(&v->arch.vgic.lock);
> +    return rc;
> +}
> +
> +HVM_REGISTER_SAVE_RESTORE(GICV2_GICC, gic_v2_gicc_save, gic_v2_gicc_load,
> +                          1, HVMSR_PER_VCPU);
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 985e866..af418ae 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -26,6 +26,7 @@
>  #include <xen/irq.h>
>  #include <xen/sched.h>
>  #include <xen/sizes.h>
> +#include <xen/hvm/save.h>
>  #include <asm/current.h>
>  #include <asm/mmio.h>
>  #include <asm/gic_v3_defs.h>
> @@ -1495,6 +1496,40 @@ int vgic_v3_init(struct domain *d)
>      return 0;
>  }
>  
> +
> +static int gic_v3_save(struct domain *d, hvm_domain_context_t *h)
> +{
> +    if ( d->arch.vgic.version != GIC_V3 )
> +        return 0;
> +
> +    if ( d->arch.vgic.nr_spis )
> +        return -ENOSYS;
> +
> +    dprintk(XENLOG_G_ERR, "HVM%d save: Cannot save GIC v3\n",
> +            d->domain_id);
> +
> +    return -ENOSYS;
> +
> +}
> +
> +static int gic_v3_load(struct domain *d, hvm_domain_context_t *h)
> +{
> +    if ( d->arch.vgic.version != GIC_V3 )
> +    {
> +        dprintk(XENLOG_G_ERR,
> +            "HVM%d restore: Cannot restore GIC v3 state into domain with %s\n",
> +            d->domain_id, gic_hw_desc(d->arch.vgic.version));
> +        return -EINVAL;
> +    }
> +
> +    dprintk(XENLOG_G_ERR, "HVM%d restore: Cannot restore GIC v3\n",
> +            d->domain_id);
> +
> +    return -ENOSYS;
> +}
> +
> +HVM_REGISTER_SAVE_RESTORE(GICV3, gic_v3_save, gic_v3_load, 1, HVMSR_PER_VCPU);
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 531ce5d..b3cebac 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -25,6 +25,7 @@
>  #include <xen/irq.h>
>  #include <xen/sched.h>
>  #include <xen/perfc.h>
> +#include <xen/hvm/save.h>
>  
>  #include <asm/current.h>
>  
> @@ -570,6 +571,128 @@ void vgic_free_virq(struct domain *d, unsigned int virq)
>      clear_bit(virq, d->arch.vgic.allocated_irqs);
>  }
>  
> +int vgic_irq_save_core(struct vcpu *v, unsigned irq, struct hvm_hw_irq *s)
> +{
> +    struct pending_irq *p = irq_to_pending(v, irq);
> +    struct vgic_irq_rank *rank = vgic_rank_irq(v, p->irq);
> +
> +    const bool enabled = test_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> +    const bool queued = test_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
> +    const bool visible = test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> +
> +    spin_lock(&rank->lock);

vgic_rank_lock. Maybe we need to take the vgic lock because there might
be incoming hardware interrupts for the domain but I don't think we need
the rank lock, do we?


> +    ASSERT(p->irq < 32); /* SPI not supported yet */
> +
> +    s->irq = p->irq;
> +    s->state = 0;
> +    s->priority = rank->priority[p->irq % 32];
> +
> +    if ( enabled )
> +        s->state |= HVM_HW_IRQ_STATE_ENABLED;
> +
> +    /*
> +     * If it is queued then it is pending as far as the guest is
> +     * concerned, even if we've not managed to find an LR to expose
> +     * this.
> +     */
> +    if ( queued )
> +        s->state |= HVM_HW_IRQ_STATE_PENDING;
> +
> +    /*
> +     * If the IRQ is visible in an LR then there may be state which
> +     * p->status is not aware of.
> +     */
> +    if ( visible )
> +    {
> +        struct gic_lr lr;
> +
> +        gic_vcpu_saved_lr(v, p->lr, &lr);
> +
> +        if ( lr.state & GICH_LR_PENDING )
> +            s->state |= HVM_HW_IRQ_STATE_PENDING;

This is unnecessary: given that the LRs are updated on hypervisor entry,
you can be sure at this point they are up to date.  If an irq is
GIC_IRQ_GUEST_VISIBLE, then it means that is PENDING or ACTIVE (or
PENDING and ACTIVE). Given that we have mandated that there cannot be
any ACTIVE interrupts, then we could skip all this and just

if ( visible )
    s->state |= HVM_HW_IRQ_STATE_PENDING;


> +        /*
> +         * HVM_HW_IRQ_STATE_ACTIVE: We currently do not handle save/restore
> +         * with active interrupts.
> +         */
> +        if ( lr.state & GICH_LR_ACTIVE )
> +        {
> +            dprintk(XENLOG_G_ERR,
> +                    "HVM%d save: Cannot save active local IRQ%u\n",
> +                    v->domain->domain_id, irq);
> +            return -EINVAL;
> +        }
> +    }
> +
> +    /*
> +     * HVM_HW_IRQ_STATE_EDGE: we implement ICFG[0] and [1] as RAZ/WI.
> +     */
> +
> +    spin_unlock(&rank->lock);
> +
> +    return 0;
> +}

We need to save/restore the target vcpu for each virq.


> +int vgic_irq_restore_core(struct vcpu *v, struct hvm_hw_irq *s)
> +{
> +    struct pending_irq *p = irq_to_pending(v, s->irq);
> +    struct vgic_irq_rank *rank = vgic_rank_irq(v, s->irq);
> +    bool reraise = 0;
> +
> +    /*
> +     * Level/Edge, we implement ICFG for SGI/PPI as RAZ/WI, so this bit cannot
> +     * be set.
> +     */
> +    if ( s->state & HVM_HW_IRQ_STATE_EDGE )
> +    {
> +        dprintk(XENLOG_G_ERR,
> +                "HVM%d restore: Cannot restore an edge triggered local IRQ%u\n",
> +                v->domain->domain_id, s->irq);
> +        return -EINVAL;
> +    }
> +
> +    if ( s->state & HVM_HW_IRQ_STATE_ACTIVE )
> +    {
> +        dprintk(XENLOG_G_ERR,
> +                "HVM%d restore: Cannot restore active local IRQ%u\n",
> +                v->domain->domain_id, s->irq);
> +        return -EINVAL;
> +    }
> +
> +    spin_lock(&rank->lock);

same comment on the rank lock


> +    ASSERT(s->irq < 32); /* SPI not supported yet */
> +
> +    rank->priority[s->irq % 32] = s->priority;

we need to restore the target vcpu


> +    if ( s->state & HVM_HW_IRQ_STATE_ENABLED )
> +        set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> +
> +    /* If the IRQ was either PENDING or ACTIVE, then QUEUE it to be reinjected. */
> +    if ( s->state & (HVM_HW_IRQ_STATE_PENDING|HVM_HW_IRQ_STATE_ACTIVE) )
> +    {
> +        reraise = 1;
> +        set_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
> +    }
> +
> +    /* This will check for enabled etc */
> +    if ( reraise ) /* v != current, so will add to lr_pending */

You don't need reraise: just add the call to gic_raise_guest_irq under
the previous if statement.


> +    {
> +        /*
> +         * This uses the current IPRIORITYR, which may differ from
> +         * when the IRQ was actually made pending. h/w spec probably
> +         * allows this, XXXX check
> +         */

>From this VM point of view the IPRIORITYR is s->priority. I would remove
the comment.


> +        gic_raise_guest_irq(v, s->irq, s->priority);
> +    }
> +
> +    spin_unlock(&rank->lock);
> +
> +    return 0;
> +}
> +
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 005f822..bfa9c60 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -333,6 +333,11 @@ static inline int vgic_allocate_spi(struct domain *d)
>  
>  extern void vgic_free_virq(struct domain *d, unsigned int virq);
>  
> +struct hvm_hw_irq;
> +extern int vgic_irq_save_core(struct vcpu *v, unsigned irq,
> +                              struct hvm_hw_irq *s);
> +extern int vgic_irq_restore_core(struct vcpu *v, struct hvm_hw_irq *s);
> +
>  void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
>                        paddr_t vbase, uint32_t aliased_offset);
>  
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 6322548..8df80ca 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -163,6 +163,19 @@
>   *   at Documentation/devicetree/bindings/arm/xen.txt.
>   */
>  
> +/*
> + * Requirements for Save/Restore/Migrate.
> + *
> + * When we are suspending we require the VM to have quiesced itself
> + * before calling HYPERVISOR_suspend(). This means:
> + *
> + * - There must be no active interrupt (SGI, PPI or SPI).
> + * - All VCPUs must have interrupts masked.
> + *
> + * Upon restore any event channel upcall (via the event channel PPI)
> + * which was pending upon save will be lost.
> + */
> +
>  #define XEN_HYPERCALL_TAG   0XEA1
>  
>  #define  int64_aligned_t  int64_t __attribute__((aligned(8)))
> diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h
> index 7b92c9c..db916b1 100644
> --- a/xen/include/public/arch-arm/hvm/save.h
> +++ b/xen/include/public/arch-arm/hvm/save.h
> @@ -93,10 +93,51 @@ struct hvm_hw_timer
>  
>  DECLARE_HVM_SAVE_TYPE(A15_TIMER, 3, struct hvm_hw_timer);
>  
> +
> +/* Domain global GICD state */
> +struct hvm_hw_gic_v2_gicd
> +{
> +    uint32_t ctlr;
> +};
> +DECLARE_HVM_SAVE_TYPE(GICV2_GICD, 4, struct hvm_hw_gic_v2_gicd);
> +
> +struct hvm_hw_irq
> +{
> +    uint32_t irq;
> +#define HVM_HW_IRQ_STATE_ENABLED (1UL << 0)
> +#define HVM_HW_IRQ_STATE_PENDING (1UL << 1)
> +#define HVM_HW_IRQ_STATE_ACTIVE  (1UL << 2)
> +#define HVM_HW_IRQ_STATE_EDGE    (1UL << 3)
> +    uint8_t state;
> +    uint8_t priority;
> +};
> +
> +/* Per-vcpu GICC state (and per-VCPU GICD, e.g. SGI+PPI) */
> +struct hvm_hw_gic_v2_gicc
> +{
> +    /* GICC state */
> +    uint32_t vmcr; /* GICC_{PMR,BPR,ABPR,CTLR} state */
> +    uint32_t apr; /* Active priorities */
> +
> +    /*
> +     * SGI + PPI state.
> +     */
> +    struct hvm_hw_irq local_irqs[32];
> +};
> +
> +DECLARE_HVM_SAVE_TYPE(GICV2_GICC, 5, struct hvm_hw_gic_v2_gicc);
> +
> +struct hvm_hw_gic_v3
> +{
> +    /* TODO */
> +};
> +
> +DECLARE_HVM_SAVE_TYPE(GICV3, 6, struct hvm_hw_gic_v3);
> +
>  /*
>   * Largest type-code in use
>   */
> -#define HVM_SAVE_CODE_MAX 3
> +#define HVM_SAVE_CODE_MAX 6
>  
>  #endif
>  
> -- 
> 2.6.1
> 

Re: [PATCH RFC XEN v1 09/14] xen: arm: Save and restore GIC state.

[Ian Campbell]

On Wed, 2015-12-16 at 18:30 +0000, Stefano Stabellini wrote:
> On Wed, 9 Dec 2015, Ian Campbell wrote:
> > Currently only GICv2 support is implemented.
> > 
> > Given the differing architectural state between the GICv2 and v3 I
> > ended up with separate save records. I'm not sure if this is the best
> > plan. I have also split the state into GICD (per domain) and GICC (per
> > vcpu, although also including banked GICD state).
> 
> I think it makes sense to have a per-vcpu and a per-domain save/restore
> function pair.

I agree. The main question I was asking above was whether there should a
separate struct and pair of functions for each version of the gic. At the
time I wasn't so sure, but having slept on it a bit I think it is best this
way.

> > +
> > +    ctxt.ctlr = d->arch.vgic.ctlr;
> 
> Do we need to save/restore vgic.dbase?

Yes, in fact we should save everything which comes from public/arch-arm.h
GUEST_* #defines and confirm on restore that they match the hardcoded
values in the new Xen (and if they ever change we'll need compat support).
+
> > +        /* Save PPI and SGI states (per-CPU) */
> > +        spin_lock(&v->arch.vgic.lock);
> 
> vgic_lock

Ah, yes, this function was originally in save.c so didn't have access, but
now it is in the right place I should use all the correct functions.

> Is the domain already paused at this point? If so, maybe we could get
> away without locking?

Maybe we could think about it hard enough to rationalise that, but it seems
safer and easier to just follow the locking rules regardless.

> > +int vgic_irq_save_core(struct vcpu *v, unsigned irq, struct hvm_hw_irq
> > *s)
> > +{
> > +    struct pending_irq *p = irq_to_pending(v, irq);
> > +    struct vgic_irq_rank *rank = vgic_rank_irq(v, p->irq);
> > +
> > +    const bool enabled = test_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> > +    const bool queued = test_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
> > +    const bool visible = test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> > +
> > +    spin_lock(&rank->lock);
> 
> vgic_rank_lock. Maybe we need to take the vgic lock because there might
> be incoming hardware interrupts for the domain but I don't think we need
> the rank lock, do we?

As above I think it is simpler and safer to just follow the usual rules
rather than to special case things.
> 
> > +        if ( lr.state & GICH_LR_PENDING )
> > +            s->state |= HVM_HW_IRQ_STATE_PENDING;
> 
> This is unnecessary: given that the LRs are updated on hypervisor entry,
> you can be sure at this point they are up to date.  If an irq is
> GIC_IRQ_GUEST_VISIBLE, then it means that is PENDING or ACTIVE (or
> PENDING and ACTIVE). Given that we have mandated that there cannot be
> any ACTIVE interrupts, then we could skip all this and just
> 
> if ( visible )
>     s->state |= HVM_HW_IRQ_STATE_PENDING;

Ah, the update on entry was the bit I had missed here which allows this.
I'll replace the logic as you suggest and add a comment.

> 
> 
> > +        /*
> > +         * HVM_HW_IRQ_STATE_ACTIVE: We currently do not handle
> > save/restore
> > +         * with active interrupts.
> > +         */
> > +        if ( lr.state & GICH_LR_ACTIVE )
> > +        {
> > +            dprintk(XENLOG_G_ERR,
> > +                    "HVM%d save: Cannot save active local IRQ%u\n",
> > +                    v->domain->domain_id, irq);
> > +            return -EINVAL;
> > +        }
> > +    }
> > +
> > +    /*
> > +     * HVM_HW_IRQ_STATE_EDGE: we implement ICFG[0] and [1] as RAZ/WI.
> > +     */
> > +
> > +    spin_unlock(&rank->lock);
> > +
> > +    return 0;
> > +}
> 
> We need to save/restore the target vcpu for each virq.

We only support PPIs and SGIs here, and ITARGETSR and IROUTERR aren't
writeable for those. If/when PPIs are added (which would be if/when we add
HVM style guests) we would really need to do so.

However, for the sake of ABI compatibility in the future, I think adding
the fields to the structs and initialising them 1:1 with the vcpus and
checking that on restore does make sense, so I'll do that.
> 
[...]
> > +    ASSERT(s->irq < 32); /* SPI not supported yet */
> > +
> > +    rank->priority[s->irq % 32] = s->priority;
> 
> we need to restore the target vcpu

We just need to check it is 1:1 since it can't be an SPI.

> 
> 
> > +    if ( s->state & HVM_HW_IRQ_STATE_ENABLED )
> > +        set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> > +
> > +    /* If the IRQ was either PENDING or ACTIVE, then QUEUE it to be
> > reinjected. */
> > +    if ( s->state & (HVM_HW_IRQ_STATE_PENDING|HVM_HW_IRQ_STATE_ACTIVE)
> > )
> > +    {
> > +        reraise = 1;
> > +        set_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
> > +    }
> > +
> > +    /* This will check for enabled etc */
> > +    if ( reraise ) /* v != current, so will add to lr_pending */
> 
> You don't need reraise: just add the call to gic_raise_guest_irq under
> the previous if statement.

This was a vestige of there being other reasons to reraise which turned out
not to be needed, I'll move.

> +    {
> > +        /*
> > +         * This uses the current IPRIORITYR, which may differ from
> > +         * when the IRQ was actually made pending. h/w spec probably
> > +         * allows this, XXXX check
> > +         */
> 
> From this VM point of view the IPRIORITYR is s->priority. I would remove
> the comment.

If you have an IRQ<A> with priority 55 which fires and then while it is
pending the priority is changed to 75 then I'm not sure what impact that
has on the priority which the hardware considers the already pending
interrupt to have.

To make it more complex consider if there was a second interrupt IRQ<B>
with priority 65, if the handler for that was the one changed IRQ<A>'s
priority should it expect to get immediately preempted by IRQ<A>

The comment is there because I'm not sure what behaviour the spec requires.

In terms of our code without the save restore I think the pending IRQ<A>
would remain at priority 55 and the guest would get IRQ<A> when IRQ<B>
EOIs. Whereas upon restore we would reraise it with priority 75 and IRQ<A>
would appear to preempt IRQ<B>, which would be an interesting difference in
behaviour from the PoV of the guest.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH RFC XEN v1 09/14] xen: arm: Save and restore GIC state.

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3264 bytes --]

On Thu, 17 Dec 2015, Ian Campbell wrote:
> > > +        /* Save PPI and SGI states (per-CPU) */
> > > +        spin_lock(&v->arch.vgic.lock);
> >
> > vgic_lock
>
> Ah, yes, this function was originally in save.c so didn't have access, but
> now it is in the right place I should use all the correct functions.
>
> > Is the domain already paused at this point? If so, maybe we could get
> > away without locking?
>
> Maybe we could think about it hard enough to rationalise that, but it seems
> safer and easier to just follow the locking rules regardless.
>
> > > +int vgic_irq_save_core(struct vcpu *v, unsigned irq, struct hvm_hw_irq
> > > *s)
> > > +{
> > > +    struct pending_irq *p = irq_to_pending(v, irq);
> > > +    struct vgic_irq_rank *rank = vgic_rank_irq(v, p->irq);
> > > +
> > > +    const bool enabled = test_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> > > +    const bool queued = test_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
> > > +    const bool visible = test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> > > +
> > > +    spin_lock(&rank->lock);
> >
> > vgic_rank_lock. Maybe we need to take the vgic lock because there might
> > be incoming hardware interrupts for the domain but I don't think we need
> > the rank lock, do we?
>
> As above I think it is simpler and safer to just follow the usual rules
> rather than to special case things.

I don't think we take the rank lock with the vgic lock held anywhere in
the code though. It would be good to avoid nested locking unless
necessary.


> > +    {
> > > +        /*
> > > +         * This uses the current IPRIORITYR, which may differ from
> > > +         * when the IRQ was actually made pending. h/w spec probably
> > > +         * allows this, XXXX check
> > > +         */
> >
> > From this VM point of view the IPRIORITYR is s->priority. I would remove
> > the comment.
>
> If you have an IRQ<A> with priority 55 which fires and then while it is
> pending the priority is changed to 75 then I'm not sure what impact that
> has on the priority which the hardware considers the already pending
> interrupt to have.
>
> To make it more complex consider if there was a second interrupt IRQ<B>
> with priority 65, if the handler for that was the one changed IRQ<A>'s
> priority should it expect to get immediately preempted by IRQ<A>
>
> The comment is there because I'm not sure what behaviour the spec requires.

The spec says:

It is IMPLEMENTATION DEFINED whether changing the value of a priority
field changes the priority of an active interrupt.

We could infer that changing the priority of pending interrupts is
supposed to be supported.

I would only write:

   * This uses the current IPRIORITYR, which may differ from
   * when the IRQ was actually made pending.


> In terms of our code without the save restore I think the pending IRQ<A>
> would remain at priority 55 and the guest would get IRQ<A> when IRQ<B>
> EOIs. Whereas upon restore we would reraise it with priority 75 and IRQ<A>
> would appear to preempt IRQ<B>, which would be an interesting difference in
> behaviour from the PoV of the guest.


[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH RFC XEN v1 10/14] tools: Switch a few CONFIG_MIGRATE features to CONFIG_X86

[Ian Campbell]

offline_page and compression are under a CONFIG_MIGRATE (out of
context) and along with xen-hptool these seem to rely on features
currently implemented only on x86.

I am shortly going to be enabling CONFIG_MIGRATE for ARM, but these
features are not yet available there.

I'm not sure if these are intrinsically x86 or if they deserve their
own options.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: andyhhp
---
xc_compression.c seems to actually be unused, possibly since migration
v2, perhaps pending colo?
---
 tools/libxc/Makefile | 2 +-
 tools/misc/Makefile  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index 818f2e4..cd52d77 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -63,7 +63,7 @@ GUEST_SRCS-$(CONFIG_X86) += xc_sr_save_x86_pv.c
 GUEST_SRCS-$(CONFIG_X86) += xc_sr_save_x86_hvm.c
 GUEST_SRCS-y += xc_sr_restore.c
 GUEST_SRCS-y += xc_sr_save.c
-GUEST_SRCS-y += xc_offline_page.c xc_compression.c
+GUEST_SRCS-$(CONFIG_X86) += xc_offline_page.c xc_compression.c
 else
 GUEST_SRCS-y += xc_nomigrate.c
 endif
diff --git a/tools/misc/Makefile b/tools/misc/Makefile
index c4490f3..285795c 100644
--- a/tools/misc/Makefile
+++ b/tools/misc/Makefile
@@ -18,7 +18,7 @@ INSTALL_BIN += $(INSTALL_BIN-y)
 INSTALL_SBIN                   += gtracestat
 INSTALL_SBIN                   += gtraceview
 INSTALL_SBIN                   += xen-bugtool
-INSTALL_SBIN-$(CONFIG_MIGRATE) += xen-hptool
+INSTALL_SBIN-$(CONFIG_X86)     += xen-hptool
 INSTALL_SBIN-$(CONFIG_X86)     += xen-hvmcrash
 INSTALL_SBIN-$(CONFIG_X86)     += xen-hvmctx
 INSTALL_SBIN-$(CONFIG_X86)     += xen-lowmemd
-- 
2.6.1

Re: [PATCH RFC XEN v1 10/14] tools: Switch a few CONFIG_MIGRATE features to CONFIG_X86

[Andrew Cooper]

On 09/12/15 14:32, Ian Campbell wrote:
> offline_page and compression are under a CONFIG_MIGRATE (out of
> context) and along with xen-hptool these seem to rely on features
> currently implemented only on x86.
>
> I am shortly going to be enabling CONFIG_MIGRATE for ARM, but these
> features are not yet available there.
>
> I'm not sure if these are intrinsically x86 or if they deserve their
> own options.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: andyhhp
> ---
> xc_compression.c seems to actually be unused, possibly since migration
> v2, perhaps pending colo?

Yes - migration v2 didn't add the page compression support.

I should have included this in the "Areas for improvement" in the
feature document.

xc_offline_page has nothing to do with migration, although it used to
share some infrastructure, which I suspect is why the two are linked.

xen-hptool (which I keep on mistaking for a brand of hardware vendor) is
a command line utility which uses some of xc_offline_page.c

~Andrew

[PATCH RFC XEN v1 11/14] tools: migrate: refactor selection of save/restore ops to be arch specific

[Ian Campbell]

I wasn't sure of the best way to achieve this, but a pair of per-arch
hooks seemed to be preferable to ifdeffery.

I also wasn't sure about the change to guest_type for save. The
restore half of the ctxt already has such a field but since the save
side treats it as an input to the process as opposed to the restore
side which determines it from the stream it seemed like keeping them
separate was best.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: andyhhp
---
 tools/libxc/xc_sr_common.h     | 15 +++++++++++++++
 tools/libxc/xc_sr_common_x86.c | 22 ++++++++++++++++++++++
 tools/libxc/xc_sr_restore.c    | 15 +++------------
 tools/libxc/xc_sr_save.c       | 22 +++++++---------------
 4 files changed, 47 insertions(+), 27 deletions(-)

diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
index 64f6082..0d36c8d 100644
--- a/tools/libxc/xc_sr_common.h
+++ b/tools/libxc/xc_sr_common.h
@@ -174,6 +174,9 @@ struct xc_sr_context
             struct xc_sr_save_ops ops;
             struct save_callbacks *callbacks;
 
+            /* For Domain Header */
+            uint32_t guest_type;
+
             /* Live migrate vs non live suspend. */
             bool live;
 
@@ -317,9 +320,21 @@ struct xc_sr_context
 
 extern struct xc_sr_save_ops save_ops_x86_pv;
 extern struct xc_sr_save_ops save_ops_x86_hvm;
+extern struct xc_sr_save_ops save_ops_arm;
 
 extern struct xc_sr_restore_ops restore_ops_x86_pv;
 extern struct xc_sr_restore_ops restore_ops_x86_hvm;
+extern struct xc_sr_restore_ops restore_ops_arm;
+
+/*
+ * Arch function to select the correct ctx.{save,restore}.ops
+ * implementation for the guest. Will update the appropriate ops
+ * pointer.
+ *
+ * _save must also set ctx->save.guest_type.
+ */
+void xc_sr_select_restore_ops(struct xc_sr_context *ctx);
+void xc_sr_select_save_ops(struct xc_sr_context *ctx);
 
 struct xc_sr_record
 {
diff --git a/tools/libxc/xc_sr_common_x86.c b/tools/libxc/xc_sr_common_x86.c
index 98f1cef..151bb0a 100644
--- a/tools/libxc/xc_sr_common_x86.c
+++ b/tools/libxc/xc_sr_common_x86.c
@@ -43,6 +43,28 @@ int handle_tsc_info(struct xc_sr_context *ctx, struct xc_sr_record *rec)
     return 0;
 }
 
+void xc_sr_select_save_ops(struct xc_sr_context *ctx)
+{
+    if ( ctx->dominfo.hvm )
+    {
+        ctx->save.guest_type = DHDR_TYPE_X86_HVM;
+        ctx->save.ops = save_ops_x86_hvm;
+    }
+    else
+    {
+        ctx->guest_type = DHDR_TYPE_X86_PV;
+        ctx->save.ops = save_ops_x86_pv;
+    }
+}
+
+void xc_sr_select_restore_ops(struct xc_sr_context *ctx)
+{
+    if ( ctx->dominfo.hvm )
+        ctx->restore.ops = restore_ops_x86_hvm;
+    else
+        ctx->restore.ops = restore_ops_x86_pv;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
index 05159bb..80f6bc5 100644
--- a/tools/libxc/xc_sr_restore.c
+++ b/tools/libxc/xc_sr_restore.c
@@ -763,18 +763,9 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
     if ( read_headers(&ctx) )
         return -1;
 
-    if ( ctx.dominfo.hvm )
-    {
-        ctx.restore.ops = restore_ops_x86_hvm;
-        if ( restore(&ctx) )
-            return -1;
-    }
-    else
-    {
-        ctx.restore.ops = restore_ops_x86_pv;
-        if ( restore(&ctx) )
-            return -1;
-    }
+    xc_sr_select_restore_ops(&ctx);
+    if ( restore(&ctx) )
+        return -1;
 
     IPRINTF("XenStore: mfn %#"PRIpfn", dom %d, evt %u",
             ctx.restore.xenstore_gfn,
diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index 0c12e56..e6e659d 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -6,7 +6,7 @@
 /*
  * Writes an Image header and Domain header into the stream.
  */
-static int write_headers(struct xc_sr_context *ctx, uint16_t guest_type)
+static int write_headers(struct xc_sr_context *ctx)
 {
     xc_interface *xch = ctx->xch;
     int32_t xen_version = xc_version(xch, XENVER_version, NULL);
@@ -19,7 +19,7 @@ static int write_headers(struct xc_sr_context *ctx, uint16_t guest_type)
         };
     struct xc_sr_dhdr dhdr =
         {
-            .type       = guest_type,
+            .type       = ctx->save.guest_type,
             .page_shift = XC_PAGE_SHIFT,
             .xen_major  = (xen_version >> 16) & 0xffff,
             .xen_minor  = (xen_version)       & 0xffff,
@@ -724,13 +724,13 @@ static void cleanup(struct xc_sr_context *ctx)
 /*
  * Save a domain.
  */
-static int save(struct xc_sr_context *ctx, uint16_t guest_type)
+static int save(struct xc_sr_context *ctx)
 {
     xc_interface *xch = ctx->xch;
     int rc, saved_rc = 0, saved_errno = 0;
 
     IPRINTF("Saving domain %d, type %s",
-            ctx->domid, dhdr_type_to_str(guest_type));
+            ctx->domid, dhdr_type_to_str(ctx->save.guest_type));
 
     rc = setup(ctx);
     if ( rc )
@@ -738,7 +738,7 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type)
 
     xc_report_progress_single(xch, "Start of stream");
 
-    rc = write_headers(ctx, guest_type);
+    rc = write_headers(ctx);
     if ( rc )
         goto err;
 
@@ -884,16 +884,8 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom,
         return -1;
     }
 
-    if ( ctx.dominfo.hvm )
-    {
-        ctx.save.ops = save_ops_x86_hvm;
-        return save(&ctx, DHDR_TYPE_X86_HVM);
-    }
-    else
-    {
-        ctx.save.ops = save_ops_x86_pv;
-        return save(&ctx, DHDR_TYPE_X86_PV);
-    }
+    xc_sr_select_save_ops(&ctx);
+    return save(&ctx);
 }
 
 /*
-- 
2.6.1

Re: [PATCH RFC XEN v1 11/14] tools: migrate: refactor selection of save/restore ops to be arch specific

[Andrew Cooper]

On 09/12/15 14:32, Ian Campbell wrote:
> I wasn't sure of the best way to achieve this, but a pair of per-arch
> hooks seemed to be preferable to ifdeffery.

I agree.  The patch looks good.

>
> I also wasn't sure about the change to guest_type for save. The
> restore half of the ctxt already has such a field but since the save
> side treats it as an input to the process as opposed to the restore
> side which determines it from the stream it seemed like keeping them
> separate was best.

I would recommend introducing a const uint32_t field in xc_sr_save_ops,
to make it harder to accidentally get the guest type out of sync with
the ops in use.

The save/restore difference is because of the asymetry in their use. 
The save side needs to select the correct one to used, based on the
domid, while the restore side gets one in the migration stream and has
to verify it against the domain it is restoring into.

>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: andyhhp

I don't think that is quite enough routing information to find me ;)

> ---
>  tools/libxc/xc_sr_common.h     | 15 +++++++++++++++
>  tools/libxc/xc_sr_common_x86.c | 22 ++++++++++++++++++++++
>  tools/libxc/xc_sr_restore.c    | 15 +++------------
>  tools/libxc/xc_sr_save.c       | 22 +++++++---------------
>  4 files changed, 47 insertions(+), 27 deletions(-)
>
> diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
> index 64f6082..0d36c8d 100644
> --- a/tools/libxc/xc_sr_common.h
> +++ b/tools/libxc/xc_sr_common.h
> @@ -174,6 +174,9 @@ struct xc_sr_context
>              struct xc_sr_save_ops ops;
>              struct save_callbacks *callbacks;
>  
> +            /* For Domain Header */
> +            uint32_t guest_type;
> +
>              /* Live migrate vs non live suspend. */
>              bool live;
>  
> @@ -317,9 +320,21 @@ struct xc_sr_context
>  
>  extern struct xc_sr_save_ops save_ops_x86_pv;
>  extern struct xc_sr_save_ops save_ops_x86_hvm;
> +extern struct xc_sr_save_ops save_ops_arm;
>  
>  extern struct xc_sr_restore_ops restore_ops_x86_pv;
>  extern struct xc_sr_restore_ops restore_ops_x86_hvm;
> +extern struct xc_sr_restore_ops restore_ops_arm;

These two externs look like they want to be in patch 13.

~Andrew

Re: [PATCH RFC XEN v1 11/14] tools: migrate: refactor selection of save/restore ops to be arch specific

[Ian Campbell]

On Wed, 2015-12-09 at 15:26 +0000, Andrew Cooper wrote:
> On 09/12/15 14:32, Ian Campbell wrote:
> > I wasn't sure of the best way to achieve this, but a pair of per-arch
> > hooks seemed to be preferable to ifdeffery.
> 
> I agree.  The patch looks good.
> 
> > 
> > I also wasn't sure about the change to guest_type for save. The
> > restore half of the ctxt already has such a field but since the save
> > side treats it as an input to the process as opposed to the restore
> > side which determines it from the stream it seemed like keeping them
> > separate was best.
> 
> I would recommend introducing a const uint32_t field in xc_sr_save_ops,
> to make it harder to accidentally get the guest type out of sync with
> the ops in use.

Good idea. It seems unlikely that we would end up with the same set of
xc_sr_*_ops for different types of guest, and if we do then dupping the
struct seems reasonable.

Maybe as a stretch it would be interesting to have such a field in the
restore ops and have an array of possible ops which some generic code can
iterate over. Given just x86 PV vs HVM it maybe isn't worth it though.

> The save/restore difference is because of the asymetry in their use. 
> The save side needs to select the correct one to used, based on the
> domid, while the restore side gets one in the migration stream and has
> to verify it against the domain it is restoring into.

Right, that's what I said, I think.
> 
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: andyhhp
> 
> I don't think that is quite enough routing information to find me ;)

It was an experiment to see if git send-email would consult ~/.gitaliases.
TL:DR; it doesn't (also I forgot I'd done this and --cc'd you anyway...).

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH RFC XEN v1 12/14] tools: libxc: implement modify_returncode for ARM.

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_resume.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
index 87d4324..fa16c3e 100644
--- a/tools/libxc/xc_resume.c
+++ b/tools/libxc/xc_resume.c
@@ -81,6 +81,24 @@ static int modify_returncode(xc_interface *xch, uint32_t domid)
     return 0;
 }
 
+#elif defined (__arm__) || defined(__aarch64__)
+
+static int modify_returncode(xc_interface *xch, uint32_t domid)
+{
+    vcpu_guest_context_any_t ctxt;
+    int rc;
+
+    if ( (rc = xc_vcpu_getcontext(xch, domid, 0, &ctxt)) != 0 )
+        return rc;
+
+    ctxt.c.user_regs.x0 = 1;
+
+    if ( (rc = xc_vcpu_setcontext(xch, domid, 0, &ctxt)) != 0 )
+        return rc;
+
+    return 0;
+}
+
 #else
 
 static int modify_returncode(xc_interface *xch, uint32_t domid)
-- 
2.6.1

Re: [PATCH RFC XEN v1 12/14] tools: libxc: implement modify_returncode for ARM.

[Stefano Stabellini]

On Wed, 9 Dec 2015, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxc/xc_resume.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
> index 87d4324..fa16c3e 100644
> --- a/tools/libxc/xc_resume.c
> +++ b/tools/libxc/xc_resume.c
> @@ -81,6 +81,24 @@ static int modify_returncode(xc_interface *xch, uint32_t domid)
>      return 0;
>  }
>  
> +#elif defined (__arm__) || defined(__aarch64__)
> +
> +static int modify_returncode(xc_interface *xch, uint32_t domid)
> +{
> +    vcpu_guest_context_any_t ctxt;
> +    int rc;


Should we call xc_domain_getinfo and check for info.shutdown_reason !=
SHUTDOWN_suspend, like x86 does?


> +    if ( (rc = xc_vcpu_getcontext(xch, domid, 0, &ctxt)) != 0 )
> +        return rc;
> +
> +    ctxt.c.user_regs.x0 = 1;
> +
> +    if ( (rc = xc_vcpu_setcontext(xch, domid, 0, &ctxt)) != 0 )
> +        return rc;
> +
> +    return 0;
> +}
> +
>  #else
>  
>  static int modify_returncode(xc_interface *xch, uint32_t domid)
> -- 
> 2.6.1
> 

Re: [PATCH RFC XEN v1 12/14] tools: libxc: implement modify_returncode for ARM.

[Ian Campbell]

On Wed, 2015-12-16 at 16:22 +0000, Stefano Stabellini wrote:
> On Wed, 9 Dec 2015, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  tools/libxc/xc_resume.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
> > index 87d4324..fa16c3e 100644
> > --- a/tools/libxc/xc_resume.c
> > +++ b/tools/libxc/xc_resume.c
> > @@ -81,6 +81,24 @@ static int modify_returncode(xc_interface *xch,
> > uint32_t domid)
> >      return 0;
> >  }
> >  
> > +#elif defined (__arm__) || defined(__aarch64__)
> > +
> > +static int modify_returncode(xc_interface *xch, uint32_t domid)
> > +{
> > +    vcpu_guest_context_any_t ctxt;
> > +    int rc;
> 
> 
> Should we call xc_domain_getinfo and check for info.shutdown_reason !=
> SHUTDOWN_suspend, like x86 does?

Yes absolutely, I'd missed that aspect and thought x86 was only looking for
HVM vs PV.

Actually, I bet I can refactor a bunch of this into common code with only a
small arch specific core.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH RFC XEN v1 13/14] tools: libxc: wire up migration for ARM

[Ian Campbell]

This seems almost too easy.

It's possible there is some scope for sharing more with the x86/HVM
side.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: andyhhp
---
 config/arm32.mk                 |   1 +
 config/arm64.mk                 |   1 +
 tools/libxc/Makefile            |   3 +
 tools/libxc/xc_sr_common.h      |  19 ++++
 tools/libxc/xc_sr_common_arm.c  |  22 ++++
 tools/libxc/xc_sr_common_arm.h  |  15 +++
 tools/libxc/xc_sr_restore_arm.c | 225 ++++++++++++++++++++++++++++++++++++++++
 tools/libxc/xc_sr_save_arm.c    | 183 ++++++++++++++++++++++++++++++++
 8 files changed, 469 insertions(+)
 create mode 100644 tools/libxc/xc_sr_common_arm.c
 create mode 100644 tools/libxc/xc_sr_common_arm.h
 create mode 100644 tools/libxc/xc_sr_restore_arm.c
 create mode 100644 tools/libxc/xc_sr_save_arm.c

diff --git a/config/arm32.mk b/config/arm32.mk
index cd97e42..5389429 100644
--- a/config/arm32.mk
+++ b/config/arm32.mk
@@ -1,6 +1,7 @@
 CONFIG_ARM := y
 CONFIG_ARM_32 := y
 CONFIG_ARM_$(XEN_OS) := y
+CONFIG_MIGRATE := y
 
 CONFIG_XEN_INSTALL_SUFFIX :=
 
diff --git a/config/arm64.mk b/config/arm64.mk
index c5deb4e..6d38ef9 100644
--- a/config/arm64.mk
+++ b/config/arm64.mk
@@ -1,6 +1,7 @@
 CONFIG_ARM := y
 CONFIG_ARM_64 := y
 CONFIG_ARM_$(XEN_OS) := y
+CONFIG_MIGRATE := y
 
 CONFIG_XEN_INSTALL_SUFFIX :=
 
diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index cd52d77..d43a060 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -61,6 +61,9 @@ GUEST_SRCS-$(CONFIG_X86) += xc_sr_restore_x86_pv.c
 GUEST_SRCS-$(CONFIG_X86) += xc_sr_restore_x86_hvm.c
 GUEST_SRCS-$(CONFIG_X86) += xc_sr_save_x86_pv.c
 GUEST_SRCS-$(CONFIG_X86) += xc_sr_save_x86_hvm.c
+GUEST_SRCS-$(CONFIG_ARM) += xc_sr_common_arm.c
+GUEST_SRCS-$(CONFIG_ARM) += xc_sr_save_arm.c
+GUEST_SRCS-$(CONFIG_ARM) += xc_sr_restore_arm.c
 GUEST_SRCS-y += xc_sr_restore.c
 GUEST_SRCS-y += xc_sr_save.c
 GUEST_SRCS-$(CONFIG_X86) += xc_offline_page.c xc_compression.c
diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
index 0d36c8d..9cc8be3 100644
--- a/tools/libxc/xc_sr_common.h
+++ b/tools/libxc/xc_sr_common.h
@@ -315,6 +315,25 @@ struct xc_sr_context
                 } restore;
             };
         } x86_hvm;
+
+        struct /* ARM guest. */
+        {
+            union
+            {
+                struct
+                {
+                } save;
+
+                struct
+                {
+                    /* HVM context blob. */
+                    void *context;
+                    size_t contextsz;
+
+                } restore;
+            };
+        } arm;
+
     };
 };
 
diff --git a/tools/libxc/xc_sr_common_arm.c b/tools/libxc/xc_sr_common_arm.c
new file mode 100644
index 0000000..d157bfe
--- /dev/null
+++ b/tools/libxc/xc_sr_common_arm.c
@@ -0,0 +1,22 @@
+#include "xc_sr_common_arm.h"
+
+void xc_sr_select_save_ops(struct xc_sr_context *ctx)
+{
+    ctx->save.guest_type = DHDR_TYPE_ARM;
+    ctx->save.ops = save_ops_arm;
+}
+
+void xc_sr_select_restore_ops(struct xc_sr_context *ctx)
+{
+    ctx->restore.ops = restore_ops_arm;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libxc/xc_sr_common_arm.h b/tools/libxc/xc_sr_common_arm.h
new file mode 100644
index 0000000..efbea70
--- /dev/null
+++ b/tools/libxc/xc_sr_common_arm.h
@@ -0,0 +1,15 @@
+#ifndef __COMMON_ARM__H
+#define __COMMON_ARM__H
+
+#include "xc_sr_common.h"
+
+#endif
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libxc/xc_sr_restore_arm.c b/tools/libxc/xc_sr_restore_arm.c
new file mode 100644
index 0000000..d62739e
--- /dev/null
+++ b/tools/libxc/xc_sr_restore_arm.c
@@ -0,0 +1,225 @@
+#include <assert.h>
+#include <arpa/inet.h>
+
+#include "xc_sr_common_arm.h"
+
+/*
+ * Process an HVM_CONTEXT record from the stream.
+ */
+static int handle_hvm_context(struct xc_sr_context *ctx,
+                              struct xc_sr_record *rec)
+{
+    xc_interface *xch = ctx->xch;
+    void *p;
+
+    p = malloc(rec->length);
+    if ( !p )
+    {
+        ERROR("Unable to allocate %u bytes for hvm context", rec->length);
+        return -1;
+    }
+
+    free(ctx->arm.restore.context);
+
+    ctx->arm.restore.context = memcpy(p, rec->data, rec->length);
+    ctx->arm.restore.contextsz = rec->length;
+
+    return 0;
+}
+
+/*
+ * Process an HVM_PARAMS record from the stream.
+ */
+static int handle_hvm_params(struct xc_sr_context *ctx,
+                             struct xc_sr_record *rec)
+{
+    xc_interface *xch = ctx->xch;
+    struct xc_sr_rec_hvm_params *hdr = rec->data;
+    struct xc_sr_rec_hvm_params_entry *entry = hdr->param;
+    unsigned int i;
+    int rc;
+
+    if ( rec->length < sizeof(*hdr)
+         || rec->length < sizeof(*hdr) + hdr->count * sizeof(*entry) )
+    {
+        ERROR("hvm_params record is too short");
+        return -1;
+    }
+
+    for ( i = 0; i < hdr->count; i++, entry++ )
+    {
+        switch ( entry->index )
+        {
+        case HVM_PARAM_CONSOLE_PFN:
+            ctx->restore.console_gfn = entry->value;
+            xc_clear_domain_page(xch, ctx->domid, entry->value);
+            break;
+        case HVM_PARAM_STORE_PFN:
+            ctx->restore.xenstore_gfn = entry->value;
+            xc_clear_domain_page(xch, ctx->domid, entry->value);
+            break;
+        }
+
+        rc = xc_hvm_param_set(xch, ctx->domid, entry->index, entry->value);
+        if ( rc < 0 )
+        {
+            PERROR("set HVM param %"PRId64" = 0x%016"PRIx64,
+                   entry->index, entry->value);
+            return rc;
+        }
+    }
+    return 0;
+}
+
+/* restore_ops function. */
+static bool arm_pfn_is_valid(const struct xc_sr_context *ctx, xen_pfn_t pfn)
+{
+    return true;
+}
+
+/* restore_ops function. */
+static xen_pfn_t arm_pfn_to_gfn(const struct xc_sr_context *ctx,
+                                    xen_pfn_t pfn)
+{
+    return pfn;
+}
+
+/* restore_ops function. */
+static void arm_set_gfn(struct xc_sr_context *ctx, xen_pfn_t pfn,
+                            xen_pfn_t gfn)
+{
+    /* no op */
+}
+
+/* restore_ops function. */
+static void arm_set_page_type(struct xc_sr_context *ctx,
+                                  xen_pfn_t pfn, xen_pfn_t type)
+{
+    /* no-op */
+}
+
+/* restore_ops function. */
+static int arm_localise_page(struct xc_sr_context *ctx,
+                                 uint32_t type, void *page)
+{
+    /* no-op */
+    return 0;
+}
+
+/*
+ * restore_ops function. Confirms the stream matches the domain.
+ */
+static int arm_setup(struct xc_sr_context *ctx)
+{
+    xc_interface *xch = ctx->xch;
+
+    if ( ctx->restore.guest_type != DHDR_TYPE_ARM )
+    {
+        ERROR("Unable to restore %s domain into an arm domain",
+              dhdr_type_to_str(ctx->restore.guest_type));
+        return -1;
+    }
+    else if ( ctx->restore.guest_page_size != PAGE_SIZE )
+    {
+        ERROR("Invalid page size %u for arm domains",
+              ctx->restore.guest_page_size);
+        return -1;
+    }
+
+    return 0;
+}
+
+/*
+ * restore_ops function.
+ */
+static int arm_process_record(struct xc_sr_context *ctx,
+                                  struct xc_sr_record *rec)
+{
+    switch ( rec->type )
+    {
+    case REC_TYPE_HVM_CONTEXT:
+        return handle_hvm_context(ctx, rec);
+
+    case REC_TYPE_HVM_PARAMS:
+        return handle_hvm_params(ctx, rec);
+
+    default:
+        return RECORD_NOT_PROCESSED;
+    }
+}
+
+/*
+ * restore_ops function.  Sets extra hvm parameters and seeds the grant table.
+ */
+static int arm_stream_complete(struct xc_sr_context *ctx)
+{
+    xc_interface *xch = ctx->xch;
+    int rc;
+
+    rc = xc_hvm_param_set(xch, ctx->domid, HVM_PARAM_STORE_EVTCHN,
+                          ctx->restore.xenstore_evtchn);
+    if ( rc )
+    {
+        PERROR("Failed to set HVM_PARAM_STORE_EVTCHN");
+        return rc;
+    }
+
+    rc = xc_hvm_param_set(xch, ctx->domid, HVM_PARAM_CONSOLE_EVTCHN,
+                          ctx->restore.console_evtchn);
+    if ( rc )
+    {
+        PERROR("Failed to set HVM_PARAM_CONSOLE_EVTCHN");
+        return rc;
+    }
+
+    rc = xc_domain_hvm_setcontext(xch, ctx->domid,
+                                  ctx->arm.restore.context,
+                                  ctx->arm.restore.contextsz);
+    if ( rc < 0 )
+    {
+        PERROR("Unable to restore HVM context");
+        return rc;
+    }
+
+    rc = xc_dom_gnttab_hvm_seed(xch, ctx->domid,
+                                ctx->restore.console_gfn,
+                                ctx->restore.xenstore_gfn,
+                                ctx->restore.console_domid,
+                                ctx->restore.xenstore_domid);
+    if ( rc )
+    {
+        PERROR("Failed to seed grant table");
+        return rc;
+    }
+
+    return rc;
+}
+
+static int arm_cleanup(struct xc_sr_context *ctx)
+{
+    /* no-op */
+    return 0;
+}
+
+struct xc_sr_restore_ops restore_ops_arm =
+{
+    .pfn_is_valid    = arm_pfn_is_valid,
+    .pfn_to_gfn      = arm_pfn_to_gfn,
+    .set_gfn         = arm_set_gfn,
+    .set_page_type   = arm_set_page_type,
+    .localise_page   = arm_localise_page,
+    .setup           = arm_setup,
+    .process_record  = arm_process_record,
+    .stream_complete = arm_stream_complete,
+    .cleanup         = arm_cleanup,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libxc/xc_sr_save_arm.c b/tools/libxc/xc_sr_save_arm.c
new file mode 100644
index 0000000..1442679
--- /dev/null
+++ b/tools/libxc/xc_sr_save_arm.c
@@ -0,0 +1,183 @@
+#include <assert.h>
+
+#include "xc_sr_common_arm.h"
+
+#include <xen/hvm/params.h>
+
+/*
+ * Query for the HVM context and write an HVM_CONTEXT record into the stream.
+ */
+static int write_hvm_context(struct xc_sr_context *ctx)
+{
+    xc_interface *xch = ctx->xch;
+    int rc, hvm_buf_size;
+    struct xc_sr_record hvm_rec =
+    {
+        .type = REC_TYPE_HVM_CONTEXT,
+    };
+
+    IPRINTF("Writing HVM context");
+    hvm_buf_size = xc_domain_hvm_getcontext(xch, ctx->domid, 0, 0);
+    if ( hvm_buf_size < 0 )
+    {
+        PERROR("Couldn't get HVM context size from Xen");
+        rc = -1;
+        goto out;
+    }
+
+    hvm_rec.data = malloc(hvm_buf_size);
+    if ( !hvm_rec.data )
+    {
+        PERROR("Couldn't allocate memory");
+        rc = -1;
+        goto out;
+    }
+
+    hvm_buf_size = xc_domain_hvm_getcontext(xch, ctx->domid,
+                                            hvm_rec.data, hvm_buf_size);
+    if ( hvm_buf_size < 0 )
+    {
+        PERROR("Couldn't get HVM context from Xen");
+        rc = -1;
+        goto out;
+    }
+
+    hvm_rec.length = hvm_buf_size;
+    rc = write_record(ctx, &hvm_rec);
+    if ( rc < 0 )
+    {
+        PERROR("error write HVM_CONTEXT record");
+        goto out;
+    }
+
+ out:
+    free(hvm_rec.data);
+    return rc;
+}
+
+/*
+ * Query for a range of HVM parameters and write an HVM_PARAMS record into the
+ * stream.
+ */
+static int write_hvm_params(struct xc_sr_context *ctx)
+{
+    static const unsigned int params[] = {
+        HVM_PARAM_STORE_PFN,
+        HVM_PARAM_CONSOLE_PFN,
+    };
+
+    xc_interface *xch = ctx->xch;
+    struct xc_sr_rec_hvm_params_entry entries[ARRAY_SIZE(params)];
+    struct xc_sr_rec_hvm_params hdr = {
+        .count = 0,
+    };
+    struct xc_sr_record rec = {
+        .type   = REC_TYPE_HVM_PARAMS,
+        .length = sizeof(hdr),
+        .data   = &hdr,
+    };
+    unsigned int i;
+    int rc;
+
+    for ( i = 0; i < ARRAY_SIZE(params); i++ )
+    {
+        uint32_t index = params[i];
+        uint64_t value;
+
+        rc = xc_hvm_param_get(xch, ctx->domid, index, &value);
+        if ( rc )
+        {
+            PERROR("Failed to get HVMPARAM at index %u", index);
+            return rc;
+        }
+
+        if ( value != 0 )
+        {
+            entries[hdr.count].index = index;
+            entries[hdr.count].value = value;
+            hdr.count++;
+        }
+    }
+
+    rc = write_split_record(ctx, &rec, entries, hdr.count * sizeof(*entries));
+    if ( rc )
+        PERROR("Failed to write HVM_PARAMS record");
+
+    return rc;
+}
+
+static xen_pfn_t arm_pfn_to_gfn(const struct xc_sr_context *ctx,
+                                    xen_pfn_t pfn)
+{
+    /* identity map */
+    return pfn;
+}
+
+static int arm_normalise_page(struct xc_sr_context *ctx,
+                                  xen_pfn_t type, void **page)
+{
+    /* no-op */
+    return 0;
+}
+
+static int arm_setup(struct xc_sr_context *ctx)
+{
+    /* no-op */
+    return 0;
+}
+
+static int arm_start_of_stream(struct xc_sr_context *ctx)
+{
+    /* no-op */
+    return 0;
+}
+
+static int arm_start_of_checkpoint(struct xc_sr_context *ctx)
+{
+    /* no-op */
+    return 0;
+}
+
+static int arm_end_of_checkpoint(struct xc_sr_context *ctx)
+{
+    int rc;
+
+    /* Write the HVM_CONTEXT record. */
+    rc = write_hvm_context(ctx);
+    if ( rc )
+        return rc;
+
+    /* Write HVM_PARAMS record contains applicable HVM params. */
+    rc = write_hvm_params(ctx);
+    if ( rc )
+        return rc;
+
+    return 0;
+}
+
+static int arm_cleanup(struct xc_sr_context *ctx)
+{
+    /* no-op */
+    return 0;
+}
+
+struct xc_sr_save_ops save_ops_arm =
+{
+    .pfn_to_gfn          = arm_pfn_to_gfn,
+    .normalise_page      = arm_normalise_page,
+    .setup               = arm_setup,
+    .start_of_stream     = arm_start_of_stream,
+    .start_of_checkpoint = arm_start_of_checkpoint,
+    .end_of_checkpoint   = arm_end_of_checkpoint,
+    .cleanup             = arm_cleanup,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.6.1

Re: [PATCH RFC XEN v1 13/14] tools: libxc: wire up migration for ARM

[Andrew Cooper]

On 09/12/15 14:32, Ian Campbell wrote:
> This seems almost too easy.

I am glad that a healthy quantity of time has passed since the 4.6 dev
window.  Easy is not how I would have described this back then!

> diff --git a/tools/libxc/xc_sr_common_arm.h b/tools/libxc/xc_sr_common_arm.h
> new file mode 100644
> index 0000000..efbea70
> --- /dev/null
> +++ b/tools/libxc/xc_sr_common_arm.h
> @@ -0,0 +1,15 @@
> +#ifndef __COMMON_ARM__H
> +#define __COMMON_ARM__H
> +
> +#include "xc_sr_common.h"
> +
> +#endif
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Given how small this is, I would just drop it and include xc_sr_common.h
directly.  It only exists on the x86 side because of the magic TSC
handling which is common between PV and HVM guests.

~Andrew

[PATCH RFC XEN v1 14/14] tools/libxl: BODGE ARM save/restore and (dead) migration.

[Ian Campbell]

*** NOT TO BE APPLIED ***

Currently no support (even in this series) for live migration so
removing LIBXL_HAVE_NO_SUSPEND_RESUME on ARM for real seems premature.

This bodge however lets xl save/restore work and causes xl migrate to
do a dead instead of live migration for testing.

Do not apply this nonsense.
---
 tools/libxl/libxl.h      | 3 ++-
 tools/libxl/xl_cmdimpl.c | 8 +++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6b73848..5516bbb 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -677,7 +677,8 @@ typedef struct libxl__ctx libxl_ctx;
  *  - libxl_domain_remus_start
  */
 #if defined(__arm__) || defined(__aarch64__)
-#define LIBXL_HAVE_NO_SUSPEND_RESUME 1
+/* XXX have suspend/resume, but not live migration */
+//#define LIBXL_HAVE_NO_SUSPEND_RESUME 1
 #endif
 
 /*
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 2b6371d..7104872 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4291,7 +4291,13 @@ static void migrate_domain(uint32_t domid, const char *rune, int debug,
     char *away_domname;
     char rc_buf;
     uint8_t *config_data;
-    int config_len, flags = LIBXL_SUSPEND_LIVE;
+    int config_len, flags =
+#if defined(__arm__) || defined(__aarch64__)
+        0
+#else
+        LIBXL_SUSPEND_LIVE
+#endif
+        ;
 
     save_domain_core_begin(domid, override_config_file,
                            &config_data, &config_len);
-- 
2.6.1

[PATCH RFC LINUX v1] xen: arm: enable migration on ARM.

[Ian Campbell]

Replace various stub functions with real functionality, including
reestablishing the shared info page and the per-vcpu info pages on
restore.

Reestablishing the vcpu info page is a little subtle. The
VCPUOP_register_vcpu_info hypercall can only be called on either the
current VCPU or on an offline different VCPU. Since migration occurs
with all VCPUS online they are all therefore online at the point of
resume.

Therefore we must perform a cross VCPU call to each non-boot VCPU,
which cannot be done in the xen_arch_post_suspend() callback since
that is run from stop_machine() with interrupts disabled.

Furthermore VCPUOP_register_vcpu_info can only be called once per-VCPU
in a given domain, so it must not be called after a cancelled suspend
(which resumes in the same domain).

Therefore xen_arch_resume() gains a suspend_cancelled parameter and we
resume the secondary VCPUs there only if needed.

The VCPU which is running the suspend is resumed earlier in the
xen_arch_post_suspend callback, again conditionally only for
non-cancelled suspends.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/arm/xen/Makefile    |  2 +-
 arch/arm/xen/enlighten.c | 54 +++++++++++++++++++++++++++++++-----------------
 arch/arm/xen/suspend.c   | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
 arch/arm/xen/xen-ops.h   |  9 ++++++++
 arch/x86/xen/suspend.c   |  2 +-
 drivers/xen/manage.c     |  2 +-
 include/xen/xen-ops.h    |  2 +-
 7 files changed, 102 insertions(+), 23 deletions(-)
 create mode 100644 arch/arm/xen/suspend.c
 create mode 100644 arch/arm/xen/xen-ops.h

diff --git a/arch/arm/xen/Makefile b/arch/arm/xen/Makefile
index 1296952..677022c 100644
--- a/arch/arm/xen/Makefile
+++ b/arch/arm/xen/Makefile
@@ -1 +1 @@
-obj-y		:= enlighten.o hypercall.o grant-table.o p2m.o mm.o
+obj-y	:= enlighten.o hypercall.o grant-table.o p2m.o mm.o suspend.o
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index eeeab07..72f314e 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -182,10 +182,41 @@ void __init xen_early_init(void)
 		add_preferred_console("hvc", 0, NULL);
 }
 
-static int __init xen_guest_init(void)
+static struct shared_info *shared_info_page;
+
+int xen_register_shared_info(void)
 {
 	struct xen_add_to_physmap xatp;
-	struct shared_info *shared_info_page = NULL;
+
+	/*
+         * This function is called on boot and on restore. On boot we
+         * allocate this page immediately before calling this function
+         * and bail on failure. On resume that allocation must have
+         * succeeded or we couldn't be doing a save/restore.
+         */
+	BUG_ON(!shared_info_page);
+
+	xatp.domid = DOMID_SELF;
+	xatp.idx = 0;
+	xatp.space = XENMAPSPACE_shared_info;
+	xatp.gpfn = __pa(shared_info_page) >> PAGE_SHIFT;
+	if (HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp))
+		BUG();
+
+	HYPERVISOR_shared_info = (struct shared_info *)shared_info_page;
+
+	return 0;
+}
+
+void xen_vcpu_restore(void)
+{
+	xen_percpu_init();
+
+	/* XXX TODO: xen_setup_runstate_info(cpu); */
+}
+
+static int __init xen_guest_init(void)
+{
 	struct resource res;
 	phys_addr_t grant_frames;
 
@@ -210,18 +241,12 @@ static int __init xen_guest_init(void)
 		pr_err("not enough memory\n");
 		return -ENOMEM;
 	}
-	xatp.domid = DOMID_SELF;
-	xatp.idx = 0;
-	xatp.space = XENMAPSPACE_shared_info;
-	xatp.gpfn = __pa(shared_info_page) >> PAGE_SHIFT;
-	if (HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp))
-		BUG();
 
-	HYPERVISOR_shared_info = (struct shared_info *)shared_info_page;
+	xen_register_shared_info();
 
 	/* xen_vcpu is a pointer to the vcpu_info struct in the shared_info
 	 * page, we use it in the event channel upcall and in some pvclock
-	 * related functions. 
+	 * related functions.
 	 * The shared info contains exactly 1 CPU (the boot CPU). The guest
 	 * is required to use VCPUOP_register_vcpu_info to place vcpu info
 	 * for secondary CPUs as they are brought up.
@@ -275,15 +300,6 @@ static int __init xen_pm_init(void)
 }
 late_initcall(xen_pm_init);
 
-
-/* empty stubs */
-void xen_arch_pre_suspend(void) { }
-void xen_arch_post_suspend(int suspend_cancelled) { }
-void xen_timer_resume(void) { }
-void xen_arch_resume(void) { }
-void xen_arch_suspend(void) { }
-
-
 /* In the hypervisor.S file. */
 EXPORT_SYMBOL_GPL(HYPERVISOR_event_channel_op);
 EXPORT_SYMBOL_GPL(HYPERVISOR_grant_table_op);
diff --git a/arch/arm/xen/suspend.c b/arch/arm/xen/suspend.c
new file mode 100644
index 0000000..b420758
--- /dev/null
+++ b/arch/arm/xen/suspend.c
@@ -0,0 +1,54 @@
+#include <linux/types.h>
+#include <linux/tick.h>
+
+#include <xen/interface/xen.h>
+
+#include <asm/xen/hypercall.h>
+
+#include "xen-ops.h"
+
+void xen_arch_pre_suspend(void) {
+	/* Nothing to do */
+}
+
+void xen_arch_post_suspend(int suspend_cancelled)
+{
+	xen_register_shared_info();
+	if (!suspend_cancelled)
+		xen_vcpu_restore();
+}
+
+static void xen_vcpu_notify_suspend(void *data)
+{
+	tick_suspend_local();
+}
+
+static void xen_vcpu_notify_resume(void *data)
+{
+	int suspend_cancelled = *(int *)data;
+
+	if (smp_processor_id() == 0)
+		return;
+
+	/* Boot processor done in post_suspend */
+	if (!suspend_cancelled)
+		xen_vcpu_restore();
+
+	/* Boot processor notified via generic timekeeping_resume() */
+	tick_resume_local();
+}
+
+void xen_arch_suspend(void)
+{
+	on_each_cpu(xen_vcpu_notify_suspend, NULL, 1);
+}
+
+void xen_arch_resume(int suspend_cancelled)
+{
+	on_each_cpu(xen_vcpu_notify_resume, &suspend_cancelled, 1);
+}
+
+void xen_timer_resume(void)
+{
+	/* Nothing to do */
+}
diff --git a/arch/arm/xen/xen-ops.h b/arch/arm/xen/xen-ops.h
new file mode 100644
index 0000000..de23e91
--- /dev/null
+++ b/arch/arm/xen/xen-ops.h
@@ -0,0 +1,9 @@
+#ifndef XEN_OPS_H
+#define XEN_OPS_H
+
+#include <xen/xen-ops.h>
+
+void xen_register_shared_info(void);
+void xen_vcpu_restore(void);
+
+#endif /* XEN_OPS_H */
diff --git a/arch/x86/xen/suspend.c b/arch/x86/xen/suspend.c
index feddabd..ce2545a 100644
--- a/arch/x86/xen/suspend.c
+++ b/arch/x86/xen/suspend.c
@@ -104,7 +104,7 @@ static void xen_vcpu_notify_suspend(void *data)
 	tick_suspend_local();
 }
 
-void xen_arch_resume(void)
+void xen_arch_resume(int suspend_cancelled)
 {
 	on_each_cpu(xen_vcpu_notify_restore, NULL, 1);
 }
diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index b6e4c40..a1a64bc 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -156,7 +156,7 @@ static void do_suspend(void)
 		si.cancelled = 1;
 	}
 
-	xen_arch_resume();
+	xen_arch_resume(si.cancelled);
 
 out_resume:
 	if (!si.cancelled)
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index e4e214a..d93da50 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -12,7 +12,7 @@ void xen_arch_pre_suspend(void);
 void xen_arch_post_suspend(int suspend_cancelled);
 
 void xen_timer_resume(void);
-void xen_arch_resume(void);
+void xen_arch_resume(int suspend_cancelled);
 void xen_arch_suspend(void);
 
 void xen_resume_notifier_register(struct notifier_block *nb);
-- 
2.1.4

Re: [PATCH RFC LINUX v1] xen: arm: enable migration on ARM.

[Stefano Stabellini]

Please CC linux-arm for the non-RFC patches

On Wed, 9 Dec 2015, Ian Campbell wrote:
> Replace various stub functions with real functionality, including
> reestablishing the shared info page and the per-vcpu info pages on
> restore.
> 
> Reestablishing the vcpu info page is a little subtle. The
> VCPUOP_register_vcpu_info hypercall can only be called on either the
> current VCPU or on an offline different VCPU. Since migration occurs
> with all VCPUS online they are all therefore online at the point of
> resume.
> 
> Therefore we must perform a cross VCPU call to each non-boot VCPU,
> which cannot be done in the xen_arch_post_suspend() callback since
> that is run from stop_machine() with interrupts disabled.
> 
> Furthermore VCPUOP_register_vcpu_info can only be called once per-VCPU
> in a given domain, so it must not be called after a cancelled suspend
> (which resumes in the same domain).
> 
> Therefore xen_arch_resume() gains a suspend_cancelled parameter and we
> resume the secondary VCPUs there only if needed.

This is a bit complex, maybe we could use a cpu notifier like we do on
x86?


> The VCPU which is running the suspend is resumed earlier in the
> xen_arch_post_suspend callback, again conditionally only for
> non-cancelled suspends.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  arch/arm/xen/Makefile    |  2 +-
>  arch/arm/xen/enlighten.c | 54 +++++++++++++++++++++++++++++++-----------------
>  arch/arm/xen/suspend.c   | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
>  arch/arm/xen/xen-ops.h   |  9 ++++++++
>  arch/x86/xen/suspend.c   |  2 +-
>  drivers/xen/manage.c     |  2 +-
>  include/xen/xen-ops.h    |  2 +-
>  7 files changed, 102 insertions(+), 23 deletions(-)
>  create mode 100644 arch/arm/xen/suspend.c
>  create mode 100644 arch/arm/xen/xen-ops.h
> 
> diff --git a/arch/arm/xen/Makefile b/arch/arm/xen/Makefile
> index 1296952..677022c 100644
> --- a/arch/arm/xen/Makefile
> +++ b/arch/arm/xen/Makefile
> @@ -1 +1 @@
> -obj-y		:= enlighten.o hypercall.o grant-table.o p2m.o mm.o
> +obj-y	:= enlighten.o hypercall.o grant-table.o p2m.o mm.o suspend.o
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index eeeab07..72f314e 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -182,10 +182,41 @@ void __init xen_early_init(void)
>  		add_preferred_console("hvc", 0, NULL);
>  }
>  
> -static int __init xen_guest_init(void)
> +static struct shared_info *shared_info_page;
> +
> +int xen_register_shared_info(void)
>  {
>  	struct xen_add_to_physmap xatp;
> -	struct shared_info *shared_info_page = NULL;
> +
> +	/*
> +         * This function is called on boot and on restore. On boot we
> +         * allocate this page immediately before calling this function
> +         * and bail on failure. On resume that allocation must have
> +         * succeeded or we couldn't be doing a save/restore.
> +         */
> +	BUG_ON(!shared_info_page);
> +
> +	xatp.domid = DOMID_SELF;
> +	xatp.idx = 0;
> +	xatp.space = XENMAPSPACE_shared_info;
> +	xatp.gpfn = __pa(shared_info_page) >> PAGE_SHIFT;
> +	if (HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp))
> +		BUG();
> +
> +	HYPERVISOR_shared_info = (struct shared_info *)shared_info_page;
> +
> +	return 0;
> +}
> +
> +void xen_vcpu_restore(void)
> +{
> +	xen_percpu_init();
> +
> +	/* XXX TODO: xen_setup_runstate_info(cpu); */
> +}
> +
> +static int __init xen_guest_init(void)
> +{
>  	struct resource res;
>  	phys_addr_t grant_frames;
>  
> @@ -210,18 +241,12 @@ static int __init xen_guest_init(void)
>  		pr_err("not enough memory\n");
>  		return -ENOMEM;
>  	}
> -	xatp.domid = DOMID_SELF;
> -	xatp.idx = 0;
> -	xatp.space = XENMAPSPACE_shared_info;
> -	xatp.gpfn = __pa(shared_info_page) >> PAGE_SHIFT;
> -	if (HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp))
> -		BUG();
>  
> -	HYPERVISOR_shared_info = (struct shared_info *)shared_info_page;
> +	xen_register_shared_info();
>  
>  	/* xen_vcpu is a pointer to the vcpu_info struct in the shared_info
>  	 * page, we use it in the event channel upcall and in some pvclock
> -	 * related functions. 
> +	 * related functions.
>  	 * The shared info contains exactly 1 CPU (the boot CPU). The guest
>  	 * is required to use VCPUOP_register_vcpu_info to place vcpu info
>  	 * for secondary CPUs as they are brought up.
> @@ -275,15 +300,6 @@ static int __init xen_pm_init(void)
>  }
>  late_initcall(xen_pm_init);
>  
> -
> -/* empty stubs */
> -void xen_arch_pre_suspend(void) { }
> -void xen_arch_post_suspend(int suspend_cancelled) { }
> -void xen_timer_resume(void) { }
> -void xen_arch_resume(void) { }
> -void xen_arch_suspend(void) { }
> -
> -
>  /* In the hypervisor.S file. */
>  EXPORT_SYMBOL_GPL(HYPERVISOR_event_channel_op);
>  EXPORT_SYMBOL_GPL(HYPERVISOR_grant_table_op);
> diff --git a/arch/arm/xen/suspend.c b/arch/arm/xen/suspend.c
> new file mode 100644
> index 0000000..b420758
> --- /dev/null
> +++ b/arch/arm/xen/suspend.c
> @@ -0,0 +1,54 @@
> +#include <linux/types.h>
> +#include <linux/tick.h>
> +
> +#include <xen/interface/xen.h>
> +
> +#include <asm/xen/hypercall.h>
> +
> +#include "xen-ops.h"
> +
> +void xen_arch_pre_suspend(void) {
> +	/* Nothing to do */
> +}
> +
> +void xen_arch_post_suspend(int suspend_cancelled)
> +{
> +	xen_register_shared_info();
> +	if (!suspend_cancelled)
> +		xen_vcpu_restore();
> +}
> +
> +static void xen_vcpu_notify_suspend(void *data)
> +{
> +	tick_suspend_local();
> +}
> +
> +static void xen_vcpu_notify_resume(void *data)
> +{
> +	int suspend_cancelled = *(int *)data;
> +
> +	if (smp_processor_id() == 0)
> +		return;
> +
> +	/* Boot processor done in post_suspend */
> +	if (!suspend_cancelled)
> +		xen_vcpu_restore();
> +
> +	/* Boot processor notified via generic timekeeping_resume() */
> +	tick_resume_local();
> +}
> +
> +void xen_arch_suspend(void)
> +{
> +	on_each_cpu(xen_vcpu_notify_suspend, NULL, 1);
> +}
> +
> +void xen_arch_resume(int suspend_cancelled)
> +{
> +	on_each_cpu(xen_vcpu_notify_resume, &suspend_cancelled, 1);
> +}
> +
> +void xen_timer_resume(void)
> +{
> +	/* Nothing to do */
> +}
> diff --git a/arch/arm/xen/xen-ops.h b/arch/arm/xen/xen-ops.h
> new file mode 100644
> index 0000000..de23e91
> --- /dev/null
> +++ b/arch/arm/xen/xen-ops.h
> @@ -0,0 +1,9 @@
> +#ifndef XEN_OPS_H
> +#define XEN_OPS_H
> +
> +#include <xen/xen-ops.h>
> +
> +void xen_register_shared_info(void);
> +void xen_vcpu_restore(void);
> +
> +#endif /* XEN_OPS_H */
> diff --git a/arch/x86/xen/suspend.c b/arch/x86/xen/suspend.c
> index feddabd..ce2545a 100644
> --- a/arch/x86/xen/suspend.c
> +++ b/arch/x86/xen/suspend.c
> @@ -104,7 +104,7 @@ static void xen_vcpu_notify_suspend(void *data)
>  	tick_suspend_local();
>  }
>  
> -void xen_arch_resume(void)
> +void xen_arch_resume(int suspend_cancelled)
>  {
>  	on_each_cpu(xen_vcpu_notify_restore, NULL, 1);
>  }
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index b6e4c40..a1a64bc 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -156,7 +156,7 @@ static void do_suspend(void)
>  		si.cancelled = 1;
>  	}
>  
> -	xen_arch_resume();
> +	xen_arch_resume(si.cancelled);
>  
>  out_resume:
>  	if (!si.cancelled)
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index e4e214a..d93da50 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -12,7 +12,7 @@ void xen_arch_pre_suspend(void);
>  void xen_arch_post_suspend(int suspend_cancelled);
>  
>  void xen_timer_resume(void);
> -void xen_arch_resume(void);
> +void xen_arch_resume(int suspend_cancelled);
>  void xen_arch_suspend(void);
>  
>  void xen_resume_notifier_register(struct notifier_block *nb);
> -- 
> 2.1.4
> 

Re: [PATCH RFC LINUX v1] xen: arm: enable migration on ARM.

[Stefano Stabellini]

On Wed, 6 Jan 2016, Stefano Stabellini wrote:
> Please CC linux-arm for the non-RFC patches
> 
> On Wed, 9 Dec 2015, Ian Campbell wrote:
> > Replace various stub functions with real functionality, including
> > reestablishing the shared info page and the per-vcpu info pages on
> > restore.
> > 
> > Reestablishing the vcpu info page is a little subtle. The
> > VCPUOP_register_vcpu_info hypercall can only be called on either the
> > current VCPU or on an offline different VCPU. Since migration occurs
> > with all VCPUS online they are all therefore online at the point of
> > resume.
> > 
> > Therefore we must perform a cross VCPU call to each non-boot VCPU,
> > which cannot be done in the xen_arch_post_suspend() callback since
> > that is run from stop_machine() with interrupts disabled.
> > 
> > Furthermore VCPUOP_register_vcpu_info can only be called once per-VCPU
> > in a given domain, so it must not be called after a cancelled suspend
> > (which resumes in the same domain).
> > 
> > Therefore xen_arch_resume() gains a suspend_cancelled parameter and we
> > resume the secondary VCPUs there only if needed.
> 
> This is a bit complex, maybe we could use a cpu notifier like we do on
> x86?

Sorry forget this email, it was sent by mistake.


> > The VCPU which is running the suspend is resumed earlier in the
> > xen_arch_post_suspend callback, again conditionally only for
> > non-cancelled suspends.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  arch/arm/xen/Makefile    |  2 +-
> >  arch/arm/xen/enlighten.c | 54 +++++++++++++++++++++++++++++++-----------------
> >  arch/arm/xen/suspend.c   | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  arch/arm/xen/xen-ops.h   |  9 ++++++++
> >  arch/x86/xen/suspend.c   |  2 +-
> >  drivers/xen/manage.c     |  2 +-
> >  include/xen/xen-ops.h    |  2 +-
> >  7 files changed, 102 insertions(+), 23 deletions(-)
> >  create mode 100644 arch/arm/xen/suspend.c
> >  create mode 100644 arch/arm/xen/xen-ops.h
> > 
> > diff --git a/arch/arm/xen/Makefile b/arch/arm/xen/Makefile
> > index 1296952..677022c 100644
> > --- a/arch/arm/xen/Makefile
> > +++ b/arch/arm/xen/Makefile
> > @@ -1 +1 @@
> > -obj-y		:= enlighten.o hypercall.o grant-table.o p2m.o mm.o
> > +obj-y	:= enlighten.o hypercall.o grant-table.o p2m.o mm.o suspend.o
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index eeeab07..72f314e 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -182,10 +182,41 @@ void __init xen_early_init(void)
> >  		add_preferred_console("hvc", 0, NULL);
> >  }
> >  
> > -static int __init xen_guest_init(void)
> > +static struct shared_info *shared_info_page;
> > +
> > +int xen_register_shared_info(void)
> >  {
> >  	struct xen_add_to_physmap xatp;
> > -	struct shared_info *shared_info_page = NULL;
> > +
> > +	/*
> > +         * This function is called on boot and on restore. On boot we
> > +         * allocate this page immediately before calling this function
> > +         * and bail on failure. On resume that allocation must have
> > +         * succeeded or we couldn't be doing a save/restore.
> > +         */
> > +	BUG_ON(!shared_info_page);
> > +
> > +	xatp.domid = DOMID_SELF;
> > +	xatp.idx = 0;
> > +	xatp.space = XENMAPSPACE_shared_info;
> > +	xatp.gpfn = __pa(shared_info_page) >> PAGE_SHIFT;
> > +	if (HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp))
> > +		BUG();
> > +
> > +	HYPERVISOR_shared_info = (struct shared_info *)shared_info_page;
> > +
> > +	return 0;
> > +}
> > +
> > +void xen_vcpu_restore(void)
> > +{
> > +	xen_percpu_init();
> > +
> > +	/* XXX TODO: xen_setup_runstate_info(cpu); */
> > +}
> > +
> > +static int __init xen_guest_init(void)
> > +{
> >  	struct resource res;
> >  	phys_addr_t grant_frames;
> >  
> > @@ -210,18 +241,12 @@ static int __init xen_guest_init(void)
> >  		pr_err("not enough memory\n");
> >  		return -ENOMEM;
> >  	}
> > -	xatp.domid = DOMID_SELF;
> > -	xatp.idx = 0;
> > -	xatp.space = XENMAPSPACE_shared_info;
> > -	xatp.gpfn = __pa(shared_info_page) >> PAGE_SHIFT;
> > -	if (HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp))
> > -		BUG();
> >  
> > -	HYPERVISOR_shared_info = (struct shared_info *)shared_info_page;
> > +	xen_register_shared_info();
> >  
> >  	/* xen_vcpu is a pointer to the vcpu_info struct in the shared_info
> >  	 * page, we use it in the event channel upcall and in some pvclock
> > -	 * related functions. 
> > +	 * related functions.
> >  	 * The shared info contains exactly 1 CPU (the boot CPU). The guest
> >  	 * is required to use VCPUOP_register_vcpu_info to place vcpu info
> >  	 * for secondary CPUs as they are brought up.
> > @@ -275,15 +300,6 @@ static int __init xen_pm_init(void)
> >  }
> >  late_initcall(xen_pm_init);
> >  
> > -
> > -/* empty stubs */
> > -void xen_arch_pre_suspend(void) { }
> > -void xen_arch_post_suspend(int suspend_cancelled) { }
> > -void xen_timer_resume(void) { }
> > -void xen_arch_resume(void) { }
> > -void xen_arch_suspend(void) { }
> > -
> > -
> >  /* In the hypervisor.S file. */
> >  EXPORT_SYMBOL_GPL(HYPERVISOR_event_channel_op);
> >  EXPORT_SYMBOL_GPL(HYPERVISOR_grant_table_op);
> > diff --git a/arch/arm/xen/suspend.c b/arch/arm/xen/suspend.c
> > new file mode 100644
> > index 0000000..b420758
> > --- /dev/null
> > +++ b/arch/arm/xen/suspend.c
> > @@ -0,0 +1,54 @@
> > +#include <linux/types.h>
> > +#include <linux/tick.h>
> > +
> > +#include <xen/interface/xen.h>
> > +
> > +#include <asm/xen/hypercall.h>
> > +
> > +#include "xen-ops.h"
> > +
> > +void xen_arch_pre_suspend(void) {
> > +	/* Nothing to do */
> > +}
> > +
> > +void xen_arch_post_suspend(int suspend_cancelled)
> > +{
> > +	xen_register_shared_info();
> > +	if (!suspend_cancelled)
> > +		xen_vcpu_restore();
> > +}
> > +
> > +static void xen_vcpu_notify_suspend(void *data)
> > +{
> > +	tick_suspend_local();
> > +}
> > +
> > +static void xen_vcpu_notify_resume(void *data)
> > +{
> > +	int suspend_cancelled = *(int *)data;
> > +
> > +	if (smp_processor_id() == 0)
> > +		return;
> > +
> > +	/* Boot processor done in post_suspend */
> > +	if (!suspend_cancelled)
> > +		xen_vcpu_restore();
> > +
> > +	/* Boot processor notified via generic timekeeping_resume() */
> > +	tick_resume_local();
> > +}
> > +
> > +void xen_arch_suspend(void)
> > +{
> > +	on_each_cpu(xen_vcpu_notify_suspend, NULL, 1);
> > +}
> > +
> > +void xen_arch_resume(int suspend_cancelled)
> > +{
> > +	on_each_cpu(xen_vcpu_notify_resume, &suspend_cancelled, 1);
> > +}
> > +
> > +void xen_timer_resume(void)
> > +{
> > +	/* Nothing to do */
> > +}
> > diff --git a/arch/arm/xen/xen-ops.h b/arch/arm/xen/xen-ops.h
> > new file mode 100644
> > index 0000000..de23e91
> > --- /dev/null
> > +++ b/arch/arm/xen/xen-ops.h
> > @@ -0,0 +1,9 @@
> > +#ifndef XEN_OPS_H
> > +#define XEN_OPS_H
> > +
> > +#include <xen/xen-ops.h>
> > +
> > +void xen_register_shared_info(void);
> > +void xen_vcpu_restore(void);
> > +
> > +#endif /* XEN_OPS_H */
> > diff --git a/arch/x86/xen/suspend.c b/arch/x86/xen/suspend.c
> > index feddabd..ce2545a 100644
> > --- a/arch/x86/xen/suspend.c
> > +++ b/arch/x86/xen/suspend.c
> > @@ -104,7 +104,7 @@ static void xen_vcpu_notify_suspend(void *data)
> >  	tick_suspend_local();
> >  }
> >  
> > -void xen_arch_resume(void)
> > +void xen_arch_resume(int suspend_cancelled)
> >  {
> >  	on_each_cpu(xen_vcpu_notify_restore, NULL, 1);
> >  }
> > diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> > index b6e4c40..a1a64bc 100644
> > --- a/drivers/xen/manage.c
> > +++ b/drivers/xen/manage.c
> > @@ -156,7 +156,7 @@ static void do_suspend(void)
> >  		si.cancelled = 1;
> >  	}
> >  
> > -	xen_arch_resume();
> > +	xen_arch_resume(si.cancelled);
> >  
> >  out_resume:
> >  	if (!si.cancelled)
> > diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> > index e4e214a..d93da50 100644
> > --- a/include/xen/xen-ops.h
> > +++ b/include/xen/xen-ops.h
> > @@ -12,7 +12,7 @@ void xen_arch_pre_suspend(void);
> >  void xen_arch_post_suspend(int suspend_cancelled);
> >  
> >  void xen_timer_resume(void);
> > -void xen_arch_resume(void);
> > +void xen_arch_resume(int suspend_cancelled);
> >  void xen_arch_suspend(void);
> >  
> >  void xen_resume_notifier_register(struct notifier_block *nb);
> > -- 
> > 2.1.4
> > 
> 

Re: [PATCH RFC LINUX v1] xen: arm: enable migration on ARM.

[Ian Campbell]

On Wed, 2016-01-06 at 17:57 +0000, Stefano Stabellini wrote:
> On Wed, 6 Jan 2016, Stefano Stabellini wrote:
> > Please CC linux-arm for the non-RFC patches
> > 
> > On Wed, 9 Dec 2015, Ian Campbell wrote:
> > > Replace various stub functions with real functionality, including
> > > reestablishing the shared info page and the per-vcpu info pages on
> > > restore.
> > > 
> > > Reestablishing the vcpu info page is a little subtle. The
> > > VCPUOP_register_vcpu_info hypercall can only be called on either the
> > > current VCPU or on an offline different VCPU. Since migration occurs
> > > with all VCPUS online they are all therefore online at the point of
> > > resume.
> > > 
> > > Therefore we must perform a cross VCPU call to each non-boot VCPU,
> > > which cannot be done in the xen_arch_post_suspend() callback since
> > > that is run from stop_machine() with interrupts disabled.
> > > 
> > > Furthermore VCPUOP_register_vcpu_info can only be called once per-
> > > VCPU
> > > in a given domain, so it must not be called after a cancelled suspend
> > > (which resumes in the same domain).
> > > 
> > > Therefore xen_arch_resume() gains a suspend_cancelled parameter and
> > > we
> > > resume the secondary VCPUs there only if needed.
> > 
> > This is a bit complex, maybe we could use a cpu notifier like we do on
> > x86?
> 
> Sorry forget this email, it was sent by mistake.

Too late, you may ignore my reply though ;-)

Re: [PATCH RFC LINUX v1] xen: arm: enable migration on ARM.

[Ian Campbell]

On Wed, 2016-01-06 at 17:47 +0000, Stefano Stabellini wrote:
> Please CC linux-arm for the non-RFC patches
> 
> On Wed, 9 Dec 2015, Ian Campbell wrote:
> > Replace various stub functions with real functionality, including
> > reestablishing the shared info page and the per-vcpu info pages on
> > restore.
> > 
> > Reestablishing the vcpu info page is a little subtle. The
> > VCPUOP_register_vcpu_info hypercall can only be called on either the
> > current VCPU or on an offline different VCPU. Since migration occurs
> > with all VCPUS online they are all therefore online at the point of
> > resume.
> > 
> > Therefore we must perform a cross VCPU call to each non-boot VCPU,
> > which cannot be done in the xen_arch_post_suspend() callback since
> > that is run from stop_machine() with interrupts disabled.
> > 
> > Furthermore VCPUOP_register_vcpu_info can only be called once per-VCPU
> > in a given domain, so it must not be called after a cancelled suspend
> > (which resumes in the same domain).
> > 
> > Therefore xen_arch_resume() gains a suspend_cancelled parameter and we
> > resume the secondary VCPUs there only if needed.
> 
> This is a bit complex, maybe we could use a cpu notifier like we do on
> x86?

I('m pretty sure I) tried that first but CPUs don't go offline in this kind
of migration, so there is no notification event.

The ARM VCPUs fall somewhere between the behaviour of x86/PV and x86/HVM
VCPUS and sadly AFAICT neither of those approaches works. In particular
x86/HVM doesn't do VCPU placement for >LEGACY_MAX_VCPUS which is needed by
ARM, I think the fix for x86/HVM would involve changes a lot like this one.

Ian.

Re: [PATCH RFC LINUX v1] xen: arm: enable migration on ARM.

[Stefano Stabellini]

Please CC linux-arm for the non-RFC patches

On Wed, 9 Dec 2015, Ian Campbell wrote:
> Replace various stub functions with real functionality, including
> reestablishing the shared info page and the per-vcpu info pages on
> restore.
> 
> Reestablishing the vcpu info page is a little subtle. The
> VCPUOP_register_vcpu_info hypercall can only be called on either the
> current VCPU or on an offline different VCPU. Since migration occurs
> with all VCPUS online they are all therefore online at the point of
> resume.
> 
> Therefore we must perform a cross VCPU call to each non-boot VCPU,
> which cannot be done in the xen_arch_post_suspend() callback since
> that is run from stop_machine() with interrupts disabled.
> 
> Furthermore VCPUOP_register_vcpu_info can only be called once per-VCPU
> in a given domain, so it must not be called after a cancelled suspend
> (which resumes in the same domain).
> 
> Therefore xen_arch_resume() gains a suspend_cancelled parameter and we
> resume the secondary VCPUs there only if needed.

It is a bit complex but it seems better than what we do on x86, which
is:

	for_each_possible_cpu(cpu) {
		bool other_cpu = (cpu != smp_processor_id());
		bool is_up = HYPERVISOR_vcpu_op(VCPUOP_is_up, cpu, NULL);

		if (other_cpu && is_up &&
		    HYPERVISOR_vcpu_op(VCPUOP_down, cpu, NULL))
			BUG();

		xen_setup_runstate_info(cpu);

		if (have_vcpu_info_placement)
			xen_vcpu_setup(cpu);

		if (other_cpu && is_up &&
		    HYPERVISOR_vcpu_op(VCPUOP_up, cpu, NULL))
			BUG();
	}


> The VCPU which is running the suspend is resumed earlier in the
> xen_arch_post_suspend callback, again conditionally only for
> non-cancelled suspends.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  arch/arm/xen/Makefile    |  2 +-
>  arch/arm/xen/enlighten.c | 54 +++++++++++++++++++++++++++++++-----------------
>  arch/arm/xen/suspend.c   | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
>  arch/arm/xen/xen-ops.h   |  9 ++++++++
>  arch/x86/xen/suspend.c   |  2 +-
>  drivers/xen/manage.c     |  2 +-
>  include/xen/xen-ops.h    |  2 +-
>  7 files changed, 102 insertions(+), 23 deletions(-)
>  create mode 100644 arch/arm/xen/suspend.c
>  create mode 100644 arch/arm/xen/xen-ops.h
> 
> diff --git a/arch/arm/xen/Makefile b/arch/arm/xen/Makefile
> index 1296952..677022c 100644
> --- a/arch/arm/xen/Makefile
> +++ b/arch/arm/xen/Makefile
> @@ -1 +1 @@
> -obj-y		:= enlighten.o hypercall.o grant-table.o p2m.o mm.o
> +obj-y	:= enlighten.o hypercall.o grant-table.o p2m.o mm.o suspend.o
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index eeeab07..72f314e 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -182,10 +182,41 @@ void __init xen_early_init(void)
>  		add_preferred_console("hvc", 0, NULL);
>  }
>  
> -static int __init xen_guest_init(void)
> +static struct shared_info *shared_info_page;
> +
> +int xen_register_shared_info(void)
>  {
>  	struct xen_add_to_physmap xatp;
> -	struct shared_info *shared_info_page = NULL;
> +
> +	/*
> +         * This function is called on boot and on restore. On boot we
> +         * allocate this page immediately before calling this function
> +         * and bail on failure. On resume that allocation must have
> +         * succeeded or we couldn't be doing a save/restore.
> +         */
> +	BUG_ON(!shared_info_page);
> +
> +	xatp.domid = DOMID_SELF;
> +	xatp.idx = 0;
> +	xatp.space = XENMAPSPACE_shared_info;
> +	xatp.gpfn = __pa(shared_info_page) >> PAGE_SHIFT;
> +	if (HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp))
> +		BUG();
> +
> +	HYPERVISOR_shared_info = (struct shared_info *)shared_info_page;
> +
> +	return 0;
> +}
> +
> +void xen_vcpu_restore(void)
> +{
> +	xen_percpu_init();
> +
> +	/* XXX TODO: xen_setup_runstate_info(cpu); */
> +}
> +
> +static int __init xen_guest_init(void)
> +{
>  	struct resource res;
>  	phys_addr_t grant_frames;
>  
> @@ -210,18 +241,12 @@ static int __init xen_guest_init(void)
>  		pr_err("not enough memory\n");
>  		return -ENOMEM;
>  	}
> -	xatp.domid = DOMID_SELF;
> -	xatp.idx = 0;
> -	xatp.space = XENMAPSPACE_shared_info;
> -	xatp.gpfn = __pa(shared_info_page) >> PAGE_SHIFT;
> -	if (HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp))
> -		BUG();
>  
> -	HYPERVISOR_shared_info = (struct shared_info *)shared_info_page;
> +	xen_register_shared_info();
>  
>  	/* xen_vcpu is a pointer to the vcpu_info struct in the shared_info
>  	 * page, we use it in the event channel upcall and in some pvclock
> -	 * related functions. 
> +	 * related functions.
>  	 * The shared info contains exactly 1 CPU (the boot CPU). The guest
>  	 * is required to use VCPUOP_register_vcpu_info to place vcpu info
>  	 * for secondary CPUs as they are brought up.
> @@ -275,15 +300,6 @@ static int __init xen_pm_init(void)
>  }
>  late_initcall(xen_pm_init);
>  
> -
> -/* empty stubs */
> -void xen_arch_pre_suspend(void) { }
> -void xen_arch_post_suspend(int suspend_cancelled) { }
> -void xen_timer_resume(void) { }
> -void xen_arch_resume(void) { }
> -void xen_arch_suspend(void) { }
> -
> -
>  /* In the hypervisor.S file. */
>  EXPORT_SYMBOL_GPL(HYPERVISOR_event_channel_op);
>  EXPORT_SYMBOL_GPL(HYPERVISOR_grant_table_op);
> diff --git a/arch/arm/xen/suspend.c b/arch/arm/xen/suspend.c
> new file mode 100644
> index 0000000..b420758
> --- /dev/null
> +++ b/arch/arm/xen/suspend.c
> @@ -0,0 +1,54 @@
> +#include <linux/types.h>
> +#include <linux/tick.h>
> +
> +#include <xen/interface/xen.h>
> +
> +#include <asm/xen/hypercall.h>
> +
> +#include "xen-ops.h"
> +
> +void xen_arch_pre_suspend(void) {
> +	/* Nothing to do */
> +}
> +
> +void xen_arch_post_suspend(int suspend_cancelled)
> +{
> +	xen_register_shared_info();
> +	if (!suspend_cancelled)
> +		xen_vcpu_restore();

could we wait and call xen_vcpu_restore for cpu0 from
xen_vcpu_notify_resume?


> +}
> +
> +static void xen_vcpu_notify_suspend(void *data)
> +{
> +	tick_suspend_local();
> +}
> +
> +static void xen_vcpu_notify_resume(void *data)
> +{
> +	int suspend_cancelled = *(int *)data;
> +
> +	if (smp_processor_id() == 0)
> +		return;
> +
> +	/* Boot processor done in post_suspend */
> +	if (!suspend_cancelled)
> +		xen_vcpu_restore();
> +
> +	/* Boot processor notified via generic timekeeping_resume() */
> +	tick_resume_local();
> +}
> +
> +void xen_arch_suspend(void)
> +{
> +	on_each_cpu(xen_vcpu_notify_suspend, NULL, 1);
> +}
> +
> +void xen_arch_resume(int suspend_cancelled)
> +{
> +	on_each_cpu(xen_vcpu_notify_resume, &suspend_cancelled, 1);
> +}
> +
> +void xen_timer_resume(void)
> +{
> +	/* Nothing to do */
> +}
> diff --git a/arch/arm/xen/xen-ops.h b/arch/arm/xen/xen-ops.h
> new file mode 100644
> index 0000000..de23e91
> --- /dev/null
> +++ b/arch/arm/xen/xen-ops.h
> @@ -0,0 +1,9 @@
> +#ifndef XEN_OPS_H
> +#define XEN_OPS_H
> +
> +#include <xen/xen-ops.h>
> +
> +void xen_register_shared_info(void);
> +void xen_vcpu_restore(void);
> +
> +#endif /* XEN_OPS_H */
> diff --git a/arch/x86/xen/suspend.c b/arch/x86/xen/suspend.c
> index feddabd..ce2545a 100644
> --- a/arch/x86/xen/suspend.c
> +++ b/arch/x86/xen/suspend.c
> @@ -104,7 +104,7 @@ static void xen_vcpu_notify_suspend(void *data)
>  	tick_suspend_local();
>  }
>  
> -void xen_arch_resume(void)
> +void xen_arch_resume(int suspend_cancelled)
>  {
>  	on_each_cpu(xen_vcpu_notify_restore, NULL, 1);
>  }
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index b6e4c40..a1a64bc 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -156,7 +156,7 @@ static void do_suspend(void)
>  		si.cancelled = 1;
>  	}
>  
> -	xen_arch_resume();
> +	xen_arch_resume(si.cancelled);
>  
>  out_resume:
>  	if (!si.cancelled)
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index e4e214a..d93da50 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -12,7 +12,7 @@ void xen_arch_pre_suspend(void);
>  void xen_arch_post_suspend(int suspend_cancelled);
>  
>  void xen_timer_resume(void);
> -void xen_arch_resume(void);
> +void xen_arch_resume(int suspend_cancelled);
>  void xen_arch_suspend(void);
>  
>  void xen_resume_notifier_register(struct notifier_block *nb);
> -- 
> 2.1.4
> 

Re: [PATCH RFC LINUX v1] xen: arm: enable migration on ARM.

[Ian Campbell]

On Wed, 2016-01-06 at 17:55 +0000, Stefano Stabellini wrote:
> Please CC linux-arm for the non-RFC patches
> 
> On Wed, 9 Dec 2015, Ian Campbell wrote:
> > Replace various stub functions with real functionality, including
> > reestablishing the shared info page and the per-vcpu info pages on
> > restore.
> > 
> > Reestablishing the vcpu info page is a little subtle. The
> > VCPUOP_register_vcpu_info hypercall can only be called on either the
> > current VCPU or on an offline different VCPU. Since migration occurs
> > with all VCPUS online they are all therefore online at the point of
> > resume.
> > 
> > Therefore we must perform a cross VCPU call to each non-boot VCPU,
> > which cannot be done in the xen_arch_post_suspend() callback since
> > that is run from stop_machine() with interrupts disabled.
> > 
> > Furthermore VCPUOP_register_vcpu_info can only be called once per-VCPU
> > in a given domain, so it must not be called after a cancelled suspend
> > (which resumes in the same domain).
> > 
> > Therefore xen_arch_resume() gains a suspend_cancelled parameter and we
> > resume the secondary VCPUs there only if needed.
> 
> It is a bit complex but it seems better than what we do on x86, which
> is:

We cannot do this on ARM in any case, since we can't do VCPUOP_{up,down}
and we don't want to expose this. We might be able to do it via PCSI but
TBH I dislike this anyway since it defeats the purpose of migrating with
all VCPUs online, we may as well just bring them down before and bring them
back up afterwards (as we used to before the tools supported multivcpu
migration, i.e. the $dom/control/platform-feature-multiprocessor-suspend
node in xs).
> 
> 	for_each_possible_cpu(cpu) {
> 		bool other_cpu = (cpu != smp_processor_id());
> 		bool is_up = HYPERVISOR_vcpu_op(VCPUOP_is_up, cpu, NULL);
> 
> 		if (other_cpu && is_up &&
> 		    HYPERVISOR_vcpu_op(VCPUOP_down, cpu, NULL))
> 			BUG();
> 
> 		xen_setup_runstate_info(cpu);
> 
> 		if (have_vcpu_info_placement)
> 			xen_vcpu_setup(cpu);
> 
> 		if (other_cpu && is_up &&
> 		    HYPERVISOR_vcpu_op(VCPUOP_up, cpu, NULL))
> 			BUG();
> 	}
> 
> 
> > +void xen_arch_post_suspend(int suspend_cancelled)
> > +{
> > +	xen_register_shared_info();
> > +	if (!suspend_cancelled)
> > +		xen_vcpu_restore();
> 
> could we wait and call xen_vcpu_restore for cpu0 from
> xen_vcpu_notify_resume?

I've been through almost every permutation of where this stuff can be done.
IIRC we need to take some events on this vcpu before then, else we get
wedged before the notify.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH RFC LINUX v1] xen: arm: enable migration on ARM.

[Stefano Stabellini]

On Wed, 9 Dec 2015, Ian Campbell wrote:
> Replace various stub functions with real functionality, including
> reestablishing the shared info page and the per-vcpu info pages on
> restore.
> 
> Reestablishing the vcpu info page is a little subtle. The
> VCPUOP_register_vcpu_info hypercall can only be called on either the
> current VCPU or on an offline different VCPU. Since migration occurs
> with all VCPUS online they are all therefore online at the point of
> resume.
> 
> Therefore we must perform a cross VCPU call to each non-boot VCPU,
> which cannot be done in the xen_arch_post_suspend() callback since
> that is run from stop_machine() with interrupts disabled.
> 
> Furthermore VCPUOP_register_vcpu_info can only be called once per-VCPU
> in a given domain, so it must not be called after a cancelled suspend
> (which resumes in the same domain).
> 
> Therefore xen_arch_resume() gains a suspend_cancelled parameter and we
> resume the secondary VCPUs there only if needed.
> 
> The VCPU which is running the suspend is resumed earlier in the
> xen_arch_post_suspend callback, again conditionally only for
> non-cancelled suspends.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  arch/arm/xen/Makefile    |  2 +-
>  arch/arm/xen/enlighten.c | 54 +++++++++++++++++++++++++++++++-----------------
>  arch/arm/xen/suspend.c   | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
>  arch/arm/xen/xen-ops.h   |  9 ++++++++
>  arch/x86/xen/suspend.c   |  2 +-
>  drivers/xen/manage.c     |  2 +-
>  include/xen/xen-ops.h    |  2 +-
>  7 files changed, 102 insertions(+), 23 deletions(-)
>  create mode 100644 arch/arm/xen/suspend.c
>  create mode 100644 arch/arm/xen/xen-ops.h
> 
> diff --git a/arch/arm/xen/Makefile b/arch/arm/xen/Makefile
> index 1296952..677022c 100644
> --- a/arch/arm/xen/Makefile
> +++ b/arch/arm/xen/Makefile
> @@ -1 +1 @@
> -obj-y		:= enlighten.o hypercall.o grant-table.o p2m.o mm.o
> +obj-y	:= enlighten.o hypercall.o grant-table.o p2m.o mm.o suspend.o
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index eeeab07..72f314e 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -182,10 +182,41 @@ void __init xen_early_init(void)
>  		add_preferred_console("hvc", 0, NULL);
>  }
>  
> -static int __init xen_guest_init(void)
> +static struct shared_info *shared_info_page;
> +
> +int xen_register_shared_info(void)
>  {
>  	struct xen_add_to_physmap xatp;
> -	struct shared_info *shared_info_page = NULL;
> +
> +	/*
> +         * This function is called on boot and on restore. On boot we
> +         * allocate this page immediately before calling this function
> +         * and bail on failure. On resume that allocation must have
> +         * succeeded or we couldn't be doing a save/restore.
> +         */
> +	BUG_ON(!shared_info_page);
> +
> +	xatp.domid = DOMID_SELF;
> +	xatp.idx = 0;
> +	xatp.space = XENMAPSPACE_shared_info;
> +	xatp.gpfn = __pa(shared_info_page) >> PAGE_SHIFT;
> +	if (HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp))
> +		BUG();
> +
> +	HYPERVISOR_shared_info = (struct shared_info *)shared_info_page;
> +
> +	return 0;
> +}
> +
> +void xen_vcpu_restore(void)
> +{
> +	xen_percpu_init();
> +
> +	/* XXX TODO: xen_setup_runstate_info(cpu); */
> +}
> +
> +static int __init xen_guest_init(void)
> +{
>  	struct resource res;
>  	phys_addr_t grant_frames;
>  
> @@ -210,18 +241,12 @@ static int __init xen_guest_init(void)
>  		pr_err("not enough memory\n");
>  		return -ENOMEM;
>  	}
> -	xatp.domid = DOMID_SELF;
> -	xatp.idx = 0;
> -	xatp.space = XENMAPSPACE_shared_info;
> -	xatp.gpfn = __pa(shared_info_page) >> PAGE_SHIFT;
> -	if (HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp))
> -		BUG();
>  
> -	HYPERVISOR_shared_info = (struct shared_info *)shared_info_page;
> +	xen_register_shared_info();
>  
>  	/* xen_vcpu is a pointer to the vcpu_info struct in the shared_info
>  	 * page, we use it in the event channel upcall and in some pvclock
> -	 * related functions. 
> +	 * related functions.
>  	 * The shared info contains exactly 1 CPU (the boot CPU). The guest
>  	 * is required to use VCPUOP_register_vcpu_info to place vcpu info
>  	 * for secondary CPUs as they are brought up.
> @@ -275,15 +300,6 @@ static int __init xen_pm_init(void)
>  }
>  late_initcall(xen_pm_init);
>  
> -
> -/* empty stubs */
> -void xen_arch_pre_suspend(void) { }
> -void xen_arch_post_suspend(int suspend_cancelled) { }
> -void xen_timer_resume(void) { }
> -void xen_arch_resume(void) { }
> -void xen_arch_suspend(void) { }
> -
> -
>  /* In the hypervisor.S file. */
>  EXPORT_SYMBOL_GPL(HYPERVISOR_event_channel_op);
>  EXPORT_SYMBOL_GPL(HYPERVISOR_grant_table_op);
> diff --git a/arch/arm/xen/suspend.c b/arch/arm/xen/suspend.c
> new file mode 100644
> index 0000000..b420758
> --- /dev/null
> +++ b/arch/arm/xen/suspend.c
> @@ -0,0 +1,54 @@
> +#include <linux/types.h>
> +#include <linux/tick.h>
> +
> +#include <xen/interface/xen.h>
> +
> +#include <asm/xen/hypercall.h>
> +
> +#include "xen-ops.h"
> +
> +void xen_arch_pre_suspend(void) {
> +	/* Nothing to do */
> +}
> +
> +void xen_arch_post_suspend(int suspend_cancelled)
> +{
> +	xen_register_shared_info();
> +	if (!suspend_cancelled)
> +		xen_vcpu_restore();
> +}
> +
> +static void xen_vcpu_notify_suspend(void *data)
> +{
> +	tick_suspend_local();
> +}
> +
> +static void xen_vcpu_notify_resume(void *data)
> +{
> +	int suspend_cancelled = *(int *)data;
> +
> +	if (smp_processor_id() == 0)
> +		return;
> +
> +	/* Boot processor done in post_suspend */
> +	if (!suspend_cancelled)
> +		xen_vcpu_restore();
> +
> +	/* Boot processor notified via generic timekeeping_resume() */
> +	tick_resume_local();
> +}
> +
> +void xen_arch_suspend(void)
> +{
> +	on_each_cpu(xen_vcpu_notify_suspend, NULL, 1);
> +}
> +
> +void xen_arch_resume(int suspend_cancelled)
> +{
> +	on_each_cpu(xen_vcpu_notify_resume, &suspend_cancelled, 1);

on_each_cpu_mask(others, xen_vcpu_notify_resume, &suspend_cancelled, 1)

so that you can call xen_vcpu_notify_resume only on other cpus and
remove the smp_processor_id() == 0 check in xen_vcpu_notify_resume? (The
others mask needs to be calculated but should be simple.)


> +}
> +
> +void xen_timer_resume(void)
> +{
> +	/* Nothing to do */
> +}
> diff --git a/arch/arm/xen/xen-ops.h b/arch/arm/xen/xen-ops.h
> new file mode 100644
> index 0000000..de23e91
> --- /dev/null
> +++ b/arch/arm/xen/xen-ops.h
> @@ -0,0 +1,9 @@
> +#ifndef XEN_OPS_H
> +#define XEN_OPS_H
> +
> +#include <xen/xen-ops.h>
> +
> +void xen_register_shared_info(void);
> +void xen_vcpu_restore(void);
> +
> +#endif /* XEN_OPS_H */
> diff --git a/arch/x86/xen/suspend.c b/arch/x86/xen/suspend.c
> index feddabd..ce2545a 100644
> --- a/arch/x86/xen/suspend.c
> +++ b/arch/x86/xen/suspend.c
> @@ -104,7 +104,7 @@ static void xen_vcpu_notify_suspend(void *data)
>  	tick_suspend_local();
>  }
>  
> -void xen_arch_resume(void)
> +void xen_arch_resume(int suspend_cancelled)
>  {
>  	on_each_cpu(xen_vcpu_notify_restore, NULL, 1);
>  }
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index b6e4c40..a1a64bc 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -156,7 +156,7 @@ static void do_suspend(void)
>  		si.cancelled = 1;
>  	}
>  
> -	xen_arch_resume();
> +	xen_arch_resume(si.cancelled);
>  
>  out_resume:
>  	if (!si.cancelled)
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index e4e214a..d93da50 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -12,7 +12,7 @@ void xen_arch_pre_suspend(void);
>  void xen_arch_post_suspend(int suspend_cancelled);
>  
>  void xen_timer_resume(void);
> -void xen_arch_resume(void);
> +void xen_arch_resume(int suspend_cancelled);
>  void xen_arch_suspend(void);
>  
>  void xen_resume_notifier_register(struct notifier_block *nb);
> -- 
> 2.1.4
> 

Re: [PATCH RFC v1 00/14] xen: arm: support for save restore and dead migration

[Andrew Cooper]

On 09/12/15 14:31, Ian Campbell wrote:
> For arm32 + gicv2 systems the following supports apparently successful
> save/restore as well dead migrate a domain.
>
> There are several caveats/blockers, hence RFC.
>
>  * GIC v2 support only, no GIC v3 at all
>  * ARM32 only. Doesn't even build for ARM64 (vfp state handling needs
>    adjustment)
>  * No live migration, only "dead" (no logdirty support yet)
>  * Have not fully audited the set of state which needs to be saved (i.e. I
>    might have missed some registers or something)
>
> On the flip side, thanks to migration v2 the toolstack side of this
> practically trivial.

Glad to hear this.

> This is (of course) using the current Xen hvm blob handling strata, which I
> believe there are plans afoot to rework along the lines of the libxc/libxl
> migration v2 protocol.

Yes - I have plans as part of fixing cpuid handling for x86.

>
> For now I have no problem waiting and reworking in that vein as things
> progress, clearly the blockers/caveats (logdirty support in particular) are
> going to take some time to sort out.
> This is all based (very distantly) on some original patches from Evgeny
> Fedotov and Junghyun Yoo (both of Samsung) however very little of that code
> remains here.
>
> As well as the Xen series there will be a single Linux patch too, I'll only
> CC ARM folks on that one.

Something towards the end of this series should update
docs/features/migration.pandoc to indicate the presence of ARM support.

~Andrew

Re: [PATCH RFC v1 00/14] xen: arm: support for save restore and dead migration

[Ian Campbell]

On Wed, 2015-12-09 at 15:51 +0000, Andrew Cooper wrote:
> On 09/12/15 14:31, Ian Campbell wrote:
> > For arm32 + gicv2 systems the following supports apparently successful
> > save/restore as well dead migrate a domain.
> > 
> > There are several caveats/blockers, hence RFC.
> > 
> >  * GIC v2 support only, no GIC v3 at all
> >  * ARM32 only. Doesn't even build for ARM64 (vfp state handling needs
> >    adjustment)
> >  * No live migration, only "dead" (no logdirty support yet)
> >  * Have not fully audited the set of state which needs to be saved
> > (i.e. I
> >    might have missed some registers or something)
> > 
> > On the flip side, thanks to migration v2 the toolstack side of this
> > practically trivial.
> 
> Glad to hear this.

BTW, when I said it was trivial in patch #13 I was referring to this aspect
of adding the ARM side, not the overall effort of migration v2 (which was
clearly a non-trivial effort)...

> > This is (of course) using the current Xen hvm blob handling strata,
> > which I
> > believe there are plans afoot to rework along the lines of the
> > libxc/libxl
> > migration v2 protocol.
> 
> Yes - I have plans as part of fixing cpuid handling for x86.
> 
> > 
> > For now I have no problem waiting and reworking in that vein as things
> > progress, clearly the blockers/caveats (logdirty support in particular)
> > are
> > going to take some time to sort out.
> > This is all based (very distantly) on some original patches from Evgeny
> > Fedotov and Junghyun Yoo (both of Samsung) however very little of that
> > code
> > remains here.
> > 
> > As well as the Xen series there will be a single Linux patch too, I'll
> > only
> > CC ARM folks on that one.
> 
> Something towards the end of this series should update
> docs/features/migration.pandoc to indicate the presence of ARM support.

I held off on doing so for now due to all the caveats and because I don't
think the series should be applied at all as it currently is. I suppose I
could put the caveats into the doc.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel