Re: [PATCH v2 03/15] arm/xen: move gic save and restore registers to gic driver

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

Hello Vijay,

Thank your for the patch.

On 04/04/2014 12:56 PM, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> gic saved registers are moved to gic driver.
> This required structure is allocated at runtime
> and is saved & restored.
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/domain.c        |    3 +++
>  xen/arch/arm/gic.c           |   26 +++++++++++++++++---------
>  xen/include/asm-arm/domain.h |    3 +--
>  xen/include/asm-arm/gic.h    |    8 ++++++++
>  4 files changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 3d7e685..701298a 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -467,6 +467,9 @@ int vcpu_initialise(struct vcpu *v)
>      v->arch.saved_context.sp = (register_t)v->arch.cpu_info;
>      v->arch.saved_context.pc = (register_t)continue_new_vcpu;
>  
> +    if ( (rc = vcpu_gic_init(v)) != 0 )
> +        return rc;
> +

You don't need to allocate it for idle VCPU. Can you move it after the
if ( is_idle_vcpu(v)) ?

>      /* Idle VCPUs don't need the rest of this setup */
>      if ( is_idle_vcpu(v) )
>          return rc;

Somewhere here.

> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 86c52ac..a59118f 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -106,11 +106,11 @@ void gic_save_state(struct vcpu *v)
>       * this call and it only accesses struct vcpu fields that cannot be
>       * accessed simultaneously by another pCPU.
>       */


I'm surprised that we save theses registers for idle VCPU but not
restored...

With my request, you will have to add an if ( is_idle_vcpu(v) ) return
in gic_save_state.

> -    for ( i=0; i<nr_lrs; i++)
> -        v->arch.gic_lr[i] = GICH[GICH_LR + i];
> +    for ( i = 0; i < nr_lrs; i++ )
> +        v->arch.gic_state->gic_lr[i] = GICH[GICH_LR + i];
>      v->arch.lr_mask = this_cpu(lr_mask);
> -    v->arch.gic_apr = GICH[GICH_APR];
> -    v->arch.gic_vmcr = GICH[GICH_VMCR];
> +    v->arch.gic_state->gic_apr = GICH[GICH_APR];
> +    v->arch.gic_state->gic_vmcr = GICH[GICH_VMCR];
>      /* Disable until next VCPU scheduled */
>      GICH[GICH_HCR] = 0;
>      isb();
> @@ -125,10 +125,10 @@ void gic_restore_state(struct vcpu *v)
>          return;
>  
>      this_cpu(lr_mask) = v->arch.lr_mask;
> -    for ( i=0; i<nr_lrs; i++)
> -        GICH[GICH_LR + i] = v->arch.gic_lr[i];
> -    GICH[GICH_APR] = v->arch.gic_apr;
> -    GICH[GICH_VMCR] = v->arch.gic_vmcr;
> +    for ( i = 0; i < nr_lrs; i++ )
> +        GICH[GICH_LR + i] = v->arch.gic_state->gic_lr[i];
> +    GICH[GICH_APR] = v->arch.gic_state->gic_apr;
> +    GICH[GICH_VMCR] = v->arch.gic_state->gic_vmcr;
>      GICH[GICH_HCR] = GICH_HCR_EN;
>      isb();
>  
> @@ -975,6 +975,14 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>      } while (1);
>  }
>  
> +int vcpu_gic_init(struct vcpu *v)
> +{
> +     v->arch.gic_state = xzalloc(struct gic_state_data);
> +     if(!v->arch.gic_state)

if ( !v->arch.gic_state )

Regards,

-- 
Julien Grall