| Message ID | 20170609174141.5068-11-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 09/06/17 18:41, Andre Przywara wrote: > For LPIs we later want to dynamically allocate struct pending_irqs. > So beside needing to initialize the struct from there we also need > to clean it up and re-initialize it later on. > Export vgic_init_pending_irq() and extend it to be reusable. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/vgic.c | 4 +++- > xen/include/asm-arm/vgic.h | 1 + > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 2e4820f..7e8dba6 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -60,8 +60,10 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq) > return vgic_get_rank(v, rank); > } > > -static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq) > +void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq) > { > + memset(p, 0, sizeof(*p)); So for initialization, we will clear the memory twice which looks rather pointless (see the current caller). We probably to drop the memset or replace xzalloc by xalloc in the caller. I would be ok to see this change in a follow-up patch. Assuming you will sent a patch: Reviewed-by: Julien Grall <julien.grall@arm.com> Cheers,
Hi, On 12/06/17 16:36, Julien Grall wrote: > Hi Andre, > > On 09/06/17 18:41, Andre Przywara wrote: >> For LPIs we later want to dynamically allocate struct pending_irqs. >> So beside needing to initialize the struct from there we also need >> to clean it up and re-initialize it later on. >> Export vgic_init_pending_irq() and extend it to be reusable. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/vgic.c | 4 +++- >> xen/include/asm-arm/vgic.h | 1 + >> 2 files changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >> index 2e4820f..7e8dba6 100644 >> --- a/xen/arch/arm/vgic.c >> +++ b/xen/arch/arm/vgic.c >> @@ -60,8 +60,10 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, >> unsigned int irq) >> return vgic_get_rank(v, rank); >> } >> >> -static void vgic_init_pending_irq(struct pending_irq *p, unsigned int >> virq) >> +void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq) >> { >> + memset(p, 0, sizeof(*p)); > > So for initialization, we will clear the memory twice which looks rather > pointless (see the current caller). > > We probably to drop the memset or replace xzalloc by xalloc in the > caller. I would be ok to see this change in a follow-up patch. Assuming > you will sent a patch: So I checked the callers and now moved the memset from here to its_discard_event(), just before the call to vgic_init_pending_irq(). That should be safe, because: 1) For the existing code (initialising SGIs/PPIs and SPIs) we always zero pending_irq anyway, either by xzalloc or by an explicit memset. 2) The call in its_discard_event() has now an explicit memset before the call. 3) Allocating struct pending_irqs for LPI upon mapping a device already uses xzalloc, so they are initially zeroed. Before we re-use a struct, we call its_discard_event(), which zeroes it as described in 2) So I merged the change (remove memset here, put it in its_discard_event()) into the new series. Please tell me if that is too dangerous and I can back it out again. Cheers, Andre.
Hi Andre, On 06/14/2017 04:54 PM, Andre Przywara wrote: > Hi, > > On 12/06/17 16:36, Julien Grall wrote: >> Hi Andre, >> >> On 09/06/17 18:41, Andre Przywara wrote: >>> For LPIs we later want to dynamically allocate struct pending_irqs. >>> So beside needing to initialize the struct from there we also need >>> to clean it up and re-initialize it later on. >>> Export vgic_init_pending_irq() and extend it to be reusable. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> xen/arch/arm/vgic.c | 4 +++- >>> xen/include/asm-arm/vgic.h | 1 + >>> 2 files changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >>> index 2e4820f..7e8dba6 100644 >>> --- a/xen/arch/arm/vgic.c >>> +++ b/xen/arch/arm/vgic.c >>> @@ -60,8 +60,10 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, >>> unsigned int irq) >>> return vgic_get_rank(v, rank); >>> } >>> >>> -static void vgic_init_pending_irq(struct pending_irq *p, unsigned int >>> virq) >>> +void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq) >>> { >>> + memset(p, 0, sizeof(*p)); >> >> So for initialization, we will clear the memory twice which looks rather >> pointless (see the current caller). >> >> We probably to drop the memset or replace xzalloc by xalloc in the >> caller. I would be ok to see this change in a follow-up patch. Assuming >> you will sent a patch: > > So I checked the callers and now moved the memset from here to > its_discard_event(), just before the call to vgic_init_pending_irq(). > That should be safe, because: > 1) For the existing code (initialising SGIs/PPIs and SPIs) we always > zero pending_irq anyway, either by xzalloc or by an explicit memset. > 2) The call in its_discard_event() has now an explicit memset before the > call. > 3) Allocating struct pending_irqs for LPI upon mapping a device already > uses xzalloc, so they are initially zeroed. Before we re-use a struct, > we call its_discard_event(), which zeroes it as described in 2) The place I am the most concerned is in the MAPTI. Because you would call vgic_init_pending_irq assuming this would have already been zeroed. It is not straight-forward when looking at the code who did that. I would prefer to keep the memset in vgic_init_pending_irq and avoid it in the caller. This is more future proof. > So I merged the change (remove memset here, put it in > its_discard_event()) into the new series. > Please tell me if that is too dangerous and I can back it out again. Let's look for a follow-up patch and not in this series. I don't want to delay the series just for that. Cheers,
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 2e4820f..7e8dba6 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -60,8 +60,10 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq) return vgic_get_rank(v, rank); } -static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq) +void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq) { + memset(p, 0, sizeof(*p)); + INIT_LIST_HEAD(&p->inflight); INIT_LIST_HEAD(&p->lr_queue); p->irq = virq; diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index c9075a9..a59be6d 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -300,6 +300,7 @@ extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq); extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq); extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq); extern void vgic_clear_pending_irqs(struct vcpu *v); +extern void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq); extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq); extern struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq); extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n, int s);
For LPIs we later want to dynamically allocate struct pending_irqs. So beside needing to initialize the struct from there we also need to clean it up and re-initialize it later on. Export vgic_init_pending_irq() and extend it to be reusable. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/vgic.c | 4 +++- xen/include/asm-arm/vgic.h | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)