Re: [PATCH 03/12] ARM: VGIC: remove gic_clear_pending_irqs()

[Andre Przywara <andre.przywara@linaro.org>]

Hi,

On 10/11/17 16:42, Andre Przywara wrote:
> Hi,
> 
> On 26/10/17 01:14, Stefano Stabellini wrote:
>> On Thu, 19 Oct 2017, Andre Przywara wrote:
>>> gic_clear_pending_irqs() was not only misnamed, but also misplaced, as
>>> a function solely dealing with the GIC emulation should not live in gic.c.
>>> Move the functionality of this function into its only caller in vgic.c
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>
>> The reason why gic_clear_pending_irqs is in gic.c is that lr_mask and
>> lr_pending are considered part of the gic driver (gic.c). On the other
>> end, inflight is part of the vgic.
>>
>> As an example, the idea is that the code outside of gic.c (for example
>> vgic.c) shouldn't have to know, or have to care, whether a given IRQ is
>> in the lr_pending queue or actually in a LR register.
> 
> I can understand that the lr_pending queue *should* be logical
> continuation of the LR registers, something like spill-over LRs.
> Though I wasn't aware of this before ;-)
> So I can see that from a *logical* point of view it looks like it
> belongs to the hardware part of the GIC (more specifically gic-vgic.c),
> which deals with the actual LRs. But I guess this is somewhat of a grey
> area.
> 
> BUT:
> This is a design choice of the VGIC, and one which the KVM VGIC design
> for instance does *not* share. Also my earlier Xen VGIC rework patches
> got rid of this as well (because dealing with two lists is too complicated).
> Also, the name is misleading: gic_clear_pending_irqs() does not hint at
> all that this is dealing with the GIC emulation, I think it should read
> vgic_vcpu_clear_pending_irqs().
> And as it accesses VGIC specific data structures only, I don't think it
> belongs to gic.c, really.
> So I could live with moving it into the new gic-vgic.c, let me see if
> that works.
> 
> The need for this patch didn't come out of the blue, I actually need it
> to be able to reuse gic.c with *any* other VGIC implementation. And this
> applies to both a VGIC rework and the KVM VGIC port.
> These lr_queue and lr_pending queues are really an implementation detail
> of the existing *VGIC*, and, more importantly: they refer to the struct
> pending_irq, which is definitely a VGIC detail.
> 
> The rabbit to follow in this series is to strictly split the usage of
> struct pending_irq from the hardware GIC driver. The KVM VGIC does not
> have a "struct pending_irq", so we can't have anything mentioning that
> in code that should survive a KVM VGIC port.
> So short of replacing gic.c at all, moving everything mentioning
> pending_irq out of gic.c is the only option.

Having said that I just moved it to gic-vgic.c, where it's also you of
the way. With this code eventually being removed anyway sometimes in the
future, it's not really worth arguing about it.

Cheers,
Andre.

>> lr_mask and lr_pending are only accessed from gic.c. The only exception
>> is the initialization (INIT_LIST_HEAD(&v->arch.vgic.lr_pending)).
>>
>>
>>> ---
>>>  xen/arch/arm/gic.c        | 11 -----------
>>>  xen/arch/arm/vgic.c       |  4 +++-
>>>  xen/include/asm-arm/gic.h |  1 -
>>>  3 files changed, 3 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index ed363f6c37..75b2e0e0ca 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -675,17 +675,6 @@ out:
>>>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>>>  }
>>>  
>>> -void gic_clear_pending_irqs(struct vcpu *v)
>>> -{
>>> -    struct pending_irq *p, *t;
>>> -
>>> -    ASSERT(spin_is_locked(&v->arch.vgic.lock));
>>> -
>>> -    v->arch.lr_mask = 0;
>>> -    list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
>>> -        gic_remove_from_lr_pending(v, p);
>>> -}
>>> -
>>>  int gic_events_need_delivery(void)
>>>  {
>>>      struct vcpu *v = current;
>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>>> index d8acbbeaaa..451a306a98 100644
>>> --- a/xen/arch/arm/vgic.c
>>> +++ b/xen/arch/arm/vgic.c
>>> @@ -504,7 +504,9 @@ void vgic_clear_pending_irqs(struct vcpu *v)
>>>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>>>      list_for_each_entry_safe ( p, t, &v->arch.vgic.inflight_irqs, inflight )
>>>          list_del_init(&p->inflight);
>>> -    gic_clear_pending_irqs(v);
>>> +    list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
>>> +        gic_remove_from_lr_pending(v, p);
>>> +    v->arch.lr_mask = 0;
>>>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>>>  }
>>>  
>>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>>> index d3d7bda50d..2f248301ce 100644
>>> --- a/xen/include/asm-arm/gic.h
>>> +++ b/xen/include/asm-arm/gic.h
>>> @@ -236,7 +236,6 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>>>                                struct irq_desc *desc);
>>>  
>>>  extern void gic_inject(void);
>>> -extern void gic_clear_pending_irqs(struct vcpu *v);
>>>  extern int gic_events_need_delivery(void);
>>>  
>>>  extern void init_maintenance_interrupt(void);
>>> -- 
>>> 2.14.1
>>>

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