On Wed, Dec 05, 2012 at 12:17:57PM +0000, Marc Zyngier wrote:
> On 05/12/12 10:58, Russell King - ARM Linux wrote:
> > 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.
>
> No, they occur on code paths that do not have a common lock (one of them
> being an interrupt handler). This may change though, after one comment
> Will made earlier (the thing about delayed interrupts).
>
> If these two code sections become mutually exclusive, then indeed there
> will be no point in having an atomic type anymore.
>
> > 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?
>
> I do not think so. If the value gets decremented, it means we took a
> maintenance interrupt, which means we exited the guest at some point.
> Two possibilities:
>
> - We're not in guest mode anymore (vcpu->mode = OUTSIDE_GUEST_MODE), and
> cmpxchg will fail, hence signaling the guest to reload its state. This
> is not needed (the guest will reload its state anyway), but doesn't
> cause any harm.
What is the relative ordering of the atomic decrement and setting
vcpu->mode to be OUTSIDE_GUEST_MODE ? Is there a window where we have
decremented this atomic type but vcpu->mode is still set to IN_GUEST_MODE.
> - We're back into the guest (vcpu->mode = IN_GUEST_MODE), and cmpxchg
> will fail as well, triggering a reload which is needed this time.
Well, the whole code looks really weird to me, especially that:
+ if (kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE) {
+ if (vgic_active_irq(vcpu) &&
+ cmpxchg(&vcpu->mode, EXITING_GUEST_MODE, IN_GUEST_MODE) == EXITING_GUEST_MODE)
+ return 0;
+
+ return 1;
+ }
I've no idea what kvm_vcpu_exiting_guest_mode() is (it doesn't exist in
any tree I have access to)...
In any case, look at the version I converted to spinlocks and see whether
you think the code looks reasonable in that form. If it doesn't then it
isn't reasonable in atomic types either.
--
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
