Re: [Xen-devel] [RFC PATCH v1 04/10] arm/xen: move gic save and restore registers to gic driver

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Thu, Mar 20, 2014 at 8:52 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hello Vijay,
>
> Thanks for the patch.
>
> On 03/19/2014 02:17 PM, vijay.kilari@xxxxxxxxx wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxxxx>
>> ---
>>  xen/arch/arm/domain.c        |    3 +++
>>  xen/arch/arm/gic.c           |   36 +++++++++++++++++++++++++++++-------
>>  xen/include/asm-arm/domain.h |    3 +--
>>  xen/include/asm-arm/gic.h    |    1 +
>>  4 files changed, 34 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 82a1e79..292716a 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -466,6 +466,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;
>> +
>>      /* Idle VCPUs don't need the rest of this setup */
>>      if ( is_idle_vcpu(v) )
>>          return rc;
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 4be0897..973fcf9 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -41,6 +41,13 @@
>>  #define GICH ((volatile uint32_t *) FIXMAP_ADDR(FIXMAP_GICH))
>>  static void gic_restore_pending_irqs(struct vcpu *v);
>>
>> +struct gic_state_data {
>> +    uint32_t gic_hcr;
>> +    uint32_t gic_vmcr;
>> +    uint32_t gic_apr;
>> +    uint32_t gic_lr[64];
>> +};
>> +
>
> Can you move this structure in a gic_v2 header?
>
OK

>>  /* Global state */
>>  static struct {
>>      paddr_t dbase;       /* Address of distributor registers */
>> @@ -98,6 +105,9 @@ irq_desc_t *__irq_to_desc(int irq)
>>  void gic_save_state(struct vcpu *v)
>>  {
>>      int i;
>> +    struct gic_state_data *d;
>> +    d = (struct gic_state_data *)v->arch.gic_state;
>> +
>
> You don't need the cast. Can you also rename d in state? Using d is very
> confusing as it's a common alias used for domain.
>
OK
> struct gic_state_data *d = v->arch.gic_state;
>
>>      ASSERT(!local_irq_is_enabled());
>>
>>      /* No need for spinlocks here because interrupts are disabled around
>> @@ -105,10 +115,10 @@ void gic_save_state(struct vcpu *v)
>>       * accessed simultaneously by another pCPU.
>>       */
>>      for ( i=0; i<nr_lrs; i++)
>> -        v->arch.gic_lr[i] = GICH[GICH_LR + i];
>> +        d->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];
>> +    d->gic_apr = GICH[GICH_APR];
>> +    d->gic_vmcr = GICH[GICH_VMCR];
>>      /* Disable until next VCPU scheduled */
>>      GICH[GICH_HCR] = 0;
>>      isb();
>> @@ -117,15 +127,17 @@ void gic_save_state(struct vcpu *v)
>>  void gic_restore_state(struct vcpu *v)
>>  {
>>      int i;
>> +    struct gic_state_data *d;
>> +    d = (struct gic_state_data *)v->arch.gic_state;
>
> Same comments here.
>
>>
>>      if ( is_idle_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;
>> +        GICH[GICH_LR + i] = d->gic_lr[i];
>> +    GICH[GICH_APR] = d->gic_apr;
>> +    GICH[GICH_VMCR] = d->gic_vmcr;
>>      GICH[GICH_HCR] = GICH_HCR_EN;
>>      isb();
>>
>> @@ -877,6 +889,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)
>
> Coding style
> if ( .. )
>
OK
>> +        return -ENOMEM;
>> +     return 0;
>> +}
>> +
>
> Where is the function to free v->arch.gic_state when the domain is
> destroyed?
>
>>  int gicv_setup(struct domain *d)
>>  {
>>      int ret;
>> @@ -1001,6 +1021,8 @@ void gic_dump_info(struct vcpu *v)
>>  {
>>      int i;
>>      struct pending_irq *p;
>> +    struct gic_state_data *d;
>> +    d = (struct gic_state_data *)v->arch.gic_state;
>
> Don't need the cast and can you rename the d variable?
>
OK
> Regards,
>
> --
> Julien Grall

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