On Wed, Dec 05, 2012 at 10:43:58AM +0000, Will Deacon wrote:
> On Sat, Nov 10, 2012 at 03:45:39PM +0000, Christoffer Dall wrote:
> > From: Marc Zyngier <marc.zyngier@xxxxxxx>
> >
> > If we have level interrupts already programmed to fire on a vcpu,
> > there is no reason to kick it after injecting a new interrupt,
> > as we're guaranteed that we'll exit when the level interrupt will
> > be EOId (VGIC_LR_EOI is set).
> >
> > The exit will force a reload of the VGIC, injecting the new interrupts.
> >
> > Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> > Signed-off-by: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> > arch/arm/include/asm/kvm_vgic.h | 10 ++++++++++
> > arch/arm/kvm/arm.c | 10 +++++++++-
> > arch/arm/kvm/vgic.c | 10 ++++++++--
> > 3 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h
> > index a8e7a93..7d2662c 100644
> > --- a/arch/arm/include/asm/kvm_vgic.h
> > +++ b/arch/arm/include/asm/kvm_vgic.h
> > @@ -215,6 +215,9 @@ struct vgic_cpu {
> > u32 vgic_elrsr[2]; /* Saved only */
> > u32 vgic_apr;
> > u32 vgic_lr[64]; /* A15 has only 4... */
> > +
> > + /* Number of level-triggered interrupt in progress */
> > + atomic_t irq_active_count;
> > #endif
> > };
> >
> > @@ -254,6 +257,8 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> >
> > #define irqchip_in_kernel(k) (!!((k)->arch.vgic.vctrl_base))
> > #define vgic_initialized(k) ((k)->arch.vgic.ready)
> > +#define vgic_active_irq(v) (atomic_read(&(v)->arch.vgic_cpu.irq_active_count) == 0)
>
> When is the atomic_t initialised to zero? I can only see increments.
I'd question whether an atomic type is correct for this; the only
protection that it's offering is to ensure that the atomic increment
and decrement occur atomically - there's nothing else that they're doing
in this code.
If those atomic increments and decrements are occuring beneath a common
lock, then using atomic types is just mere code obfuscation.
For example, I'd like to question the correctness of this:
+ if (vgic_active_irq(vcpu) &&
+ cmpxchg(&vcpu->mode, EXITING_GUEST_MODE, IN_GUEST_MODE) == EXITING_GUEST_MODE)
What if vgic_active_irq() reads the atomic type, immediately after it gets
decremented to zero before the cmpxchg() is executed? Would that be a
problem?
If yes, yet again this illustrates why the use of atomic types leads people
down the path of believing that their code somehow becomes magically safe
through the use of this smoke-screen. IMHO, every use of atomic_t must be
questioned and carefully analysed before it gets into the kernel - many
are buggy through assumptions that atomic_t buys you something magic.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
