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

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

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.

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.xen.org
https://lists.xen.org/xen-devel