From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v5 18/22] xen/arm: ITS: Add domain specific ITS initialization Date: Mon, 17 Aug 2015 11:57:11 -0700 Message-ID: <55D22E87.7000909@citrix.com> References: <1437995524-19772-1-git-send-email-vijay.kilari@gmail.com> <1437995524-19772-19-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1437995524-19772-19-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, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, stefano.stabellini@citrix.com, tim@xen.org, xen-devel@lists.xen.org Cc: Prasun.Kapoor@caviumnetworks.com, manish.jaggi@caviumnetworks.com, Vijaya Kumar K List-Id: xen-devel@lists.xenproject.org Hi Vijay, This patch doesn't only do initialization but also destruction of vGIC info. Although, a part of the domain initialization is done in part in #8 and #8. It's very confusing to see the initialization split in 3 different patch. I would prefer to see the initialization/destruction in a single patch. It would help to catch whether you forgot to see you forgot some bits. On 27/07/2015 04:12, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K > > Add Domain and vcpu specific ITS initialization > > Signed-off-by: Vijaya Kumar K > --- > xen/arch/arm/vgic-v3-its.c | 10 ++++++++++ > xen/arch/arm/vgic-v3.c | 10 ++++++++++ > xen/arch/arm/vgic.c | 3 +++ > xen/include/asm-arm/gic-its.h | 2 ++ > xen/include/asm-arm/vgic.h | 3 +++ > 5 files changed, 28 insertions(+) > > diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c > index 5323192..e182cee 100644 > --- a/xen/arch/arm/vgic-v3-its.c > +++ b/xen/arch/arm/vgic-v3-its.c > @@ -44,6 +44,7 @@ > #define GITS_PIDR4_VAL (0x04) > #define GITS_BASER_INIT_VAL ((1UL << GITS_BASER_TYPE_SHIFT) | \ > (0x7UL << GITS_BASER_ENTRY_SIZE_SHIFT)) > +//#define DEBUG_ITS Why here? > #ifdef DEBUG_ITS > # define DPRINTK(fmt, args...) dprintk(XENLOG_DEBUG, fmt, ##args) > #else > @@ -1122,6 +1123,15 @@ int vits_domain_init(struct domain *d) > return 0; > } > > +void vits_domain_free(struct domain *d) > +{ > + free_xenheap_pages(d->arch.vgic.vits->prop_page, > + get_order_from_bytes(d->arch.vgic.vits->prop_size)); > + xfree(d->arch.vgic.pending_lpis); > + xfree(d->arch.vgic.vits->collections); > + xfree(d->arch.vgic.vits); For instance, you add the initialization of these fields in patch #8 and #10 but free everything here. I have to go in the others patch to get if you forgot something or not. > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index 9e6e3ff..a09ba36 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -1299,12 +1299,22 @@ static int vgic_v3_domain_init(struct domain *d) > > d->arch.vgic.ctlr = VGICD_CTLR_DEFAULT; > > + if ( is_hardware_domain(d) && gic_lpi_supported() ) I think gic_lpi_supported is badly name. vITS should only be supported when ITS is present, not when LPIs is present. And I know that you unset LPIs when ITS is not present. But this confuse the reader to do this. > + vits_domain_init(d); > + > return 0; > } > > +void vgic_v3_domain_free(struct domain *d) > +{ > + if ( is_hardware_domain(d) && gic_lpi_supported() ) > + vits_domain_free(d); > +} > + > static const struct vgic_ops v3_ops = { > .vcpu_init = vgic_v3_vcpu_init, > .domain_init = vgic_v3_domain_init, > + .domain_free = vgic_v3_domain_free, > .get_irq_priority = vgic_v3_get_irq_priority, > .get_target_vcpu = vgic_v3_get_target_vcpu, > .emulate_sysreg = vgic_v3_emulate_sysreg, > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 57c0f52..e2bfdb6 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -166,6 +166,9 @@ void domain_vgic_free(struct domain *d) > xfree(d->arch.vgic.shared_irqs); > xfree(d->arch.vgic.pending_irqs); > xfree(d->arch.vgic.allocated_irqs); > + > + if ( !d->arch.vgic.handler->domain_free ) The test is wrong. If domain_free is NULL you will end up to dereference the pointer and crash. > + d->arch.vgic.handler->domain_free(d); > } > > int vcpu_vgic_init(struct vcpu *v) > diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h > index 870c9a8..da689a4 100644 > --- a/xen/include/asm-arm/gic-its.h > +++ b/xen/include/asm-arm/gic-its.h > @@ -367,6 +367,8 @@ int its_cpu_init(void); > void its_set_lpi_properties(struct irq_desc *desc, > const cpumask_t *cpu_mask, > unsigned int priority); > +int vits_domain_init(struct domain *d); > +void vits_domain_free(struct domain *d); > int vits_get_vitt_entry(struct domain *d, uint32_t devid, > uint32_t event, struct vitt *entry); > int vits_get_vdevice_entry(struct domain *d, uint32_t devid, > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h > index b11faa0..853df04 100644 > --- a/xen/include/asm-arm/vgic.h > +++ b/xen/include/asm-arm/vgic.h > @@ -114,6 +114,8 @@ struct vgic_ops { > int (*vcpu_init)(struct vcpu *v); > /* Domain specific initialization of vGIC */ > int (*domain_init)(struct domain *d); > + /* Free domain specific resources */ > + void (*domain_free)(struct domain *d); > /* Get priority for a given irq stored in vgic structure */ > int (*get_irq_priority)(struct vcpu *v, unsigned int irq); > /* Get the target vcpu for a given virq. The rank lock is already taken > @@ -191,6 +193,7 @@ enum gic_sgi_mode; > #define vgic_num_irqs(d) ((d)->arch.vgic.nr_spis + 32) > > extern int domain_vgic_init(struct domain *d, unsigned int nr_spis); > +extern void vgic_its_init(void); Where does it come from? > extern void domain_vgic_free(struct domain *d); > extern int vcpu_vgic_init(struct vcpu *v); > extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq); > Regards, -- Julien Grall