Re: [RFC v3 2/6] xen/arm: Add save/restore support for ARM GIC V2

[Julien Grall <julien.grall@linaro.org>]

Hi Wei,

(Adding Vijay in CC).

Vijay is working on GICv3 support in Xen. His patch series and this 
patch will clash sooner.

I think you should work together to avoid big reworking later.

On 08/05/14 22:18, Wei Huang wrote:
> This patch implements a save/restore support for
> ARM guest GIC. Two types of GIC V2 states are saved seperately:

separately

> 1) VGICD_* contains the GIC distributor state from
> guest VM's view; 2) GICH_* is the GIC virtual control

I would add a newline before "2)", we don't care about long commit 
message :).

> state from hypervisor's persepctive.

perspective

>
> Signed-off-by: Evgeny Fedotov <e.fedotov@samsung.com>
> Signed-off-by: Wei Huang <w1.huang@samsung.com>
> ---
>   xen/arch/arm/vgic.c                    |  171 ++++++++++++++++++++++++++++++++
>   xen/include/public/arch-arm/hvm/save.h |   34 ++++++-
>   2 files changed, 204 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 4cf6470..505e944 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -24,6 +24,7 @@
>   #include <xen/softirq.h>
>   #include <xen/irq.h>
>   #include <xen/sched.h>
> +#include <xen/hvm/save.h>
>
>   #include <asm/current.h>
>
> @@ -73,6 +74,110 @@ static struct vgic_irq_rank *vgic_irq_rank(struct vcpu *v, int b, int n)
>           return NULL;
>   }
>
> +/* Save guest VM's distributor info into a context to support domains
> + * save/restore. Such info represents guest VM's view of its GIC
> + * distributor (GICD_*).
> + */
> +static int hvm_vgicd_save(struct domain *d, hvm_domain_context_t *h)
> +{
> +    struct hvm_arm_vgicd_v2 ctxt;
> +    struct vcpu *v;
> +    struct vgic_irq_rank *rank;
> +    int rc = 0;
> +
> +    /* Save the state for each VCPU */
> +    for_each_vcpu( d, v )
> +    {
> +        rank = &v->arch.vgic.private_irqs;
> +
> +        /* IENABLE, IACTIVE, IPEND,  PENDSGI */
> +        ctxt.ienable = rank->ienable;
> +        ctxt.iactive = rank->iactive;
> +        ctxt.ipend = rank->ipend;
> +        ctxt.pendsgi = rank->pendsgi;
> +
> +        /* ICFG */
> +        ctxt.icfg[0] = rank->icfg[0];
> +        ctxt.icfg[1] = rank->icfg[1];

I would use the same pattern as IPRIOTITY and ITARGETS.

[..]

> +/* Load guest VM's distributor info from a context to support domain
> + * save/restore. The info is loaded into vgic_irq_rank.
> + */
> +static int hvm_vgicd_load(struct domain *d, hvm_domain_context_t *h)
> +{

[..]

> +    /* ICFG */
> +    rank->icfg[0] = ctxt.icfg[0];
> +    rank->icfg[1] = ctxt.icfg[1];

Same remark here.

[..]

> +/* Save GIC virtual control state into a context to support save/restore.
> + * The info reprsents most of GICH_* registers. */

represents

Regards,

-- 
Julien Grall