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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Stefano,

On 16/11/17 01:17, Stefano Stabellini wrote:
> 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.
Why do you think pending_irq does not belong to vgic? pending_irq is 
defined by vgic.h and contain the state of the interrupt for the virtual 
GIC.
However, looking at gic_clear_pending_irqs. It is only called by 
vgic_clear_pending_irqs which is then only called by do_common_cpu_on.
The commit message adding this code (see 803d6c993a "xen/arm: clear 
pending irq queues on do_psci_cpu_on") says: "Also when (re)activating a 
vcpu, clear the vgic and gic irq queues: we don't want to inject any 
irqs that couldn't be handled by the vcpu right before going offline."
Even on real hardware you may have pending interrupt at boot. This is 
taken care by the OS when initializing the GIC CPU interface. So what is 
that function trying to solve?
Cheers,

--
Julien Grall

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