Re: [Xen-devel] [PATCH v11 10/34] ARM: GIC: export and extend vgic_init_pending_irq()

[Andre Przywara <andre.przywara@xxxxxxx>]

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@xxxxxxx>
>> ---
>>  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.

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