From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [RFC PATCH v1 04/10] arm/xen: move gic save and restore registers to gic driver Date: Thu, 20 Mar 2014 15:22:43 +0000 Message-ID: <532B07C3.9050701@linaro.org> References: <1395238631-2024-1-git-send-email-vijay.kilari@gmail.com> <1395238631-2024-5-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1395238631-2024-5-git-send-email-vijay.kilari@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: vijay.kilari@gmail.com Cc: Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org Hello Vijay, Thanks for the patch. On 03/19/2014 02:17 PM, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K > > 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 > --- > 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? > /* 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. 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 - 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 - 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 ( .. ) > + 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? Regards, -- Julien Grall