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

[Julien Grall <julien.grall@arm.com>]

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,

-- 
Julien Grall

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