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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 10 Nov 2017, 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@xxxxxxx>
> > 
> > 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.

Could you at least retain gic_clear_pending_irqs as a separate function?

pending_irq is clearly separate from anything vgic and doesn't belong
there. Nonetheless, I can live with moving gic_clear_pending_irqs to
vgic.c to make future development easier, but at least let's keep
gic_clear_pending_irqs as is.


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