Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace

[Avi Kivity <avi@redhat.com>]

On 12/12/2011 04:38 PM, Christoffer Dall wrote:
> >
> > Why don't they match?  The assignment of lines to actual pins differs,
> > but essentially it's the same thing (otherwise we'd use a different ioctl).
> >
>
> because there is no notion of gsi and irq_source_id on ARM. 


gsi = number of irq line, just a bad name, but you do have it on ARM.

irq_source_id really shouldn't have been in kvm_set_irq(), it's an
implementation detail rather than an architectural feature; just ignore it.

> What's the
> harm of this additional tracepoint?

If we get tools that use them, they have an additional difference to
consider.  It's a weak argument but it's there.

> If I should re-use the existing one, should I simply move it outside
> of __KVM_HAVE_IOAPIC?

Protect it with __KVM_HAVE_IRQ_LINE so we don't leak unused tracepoints
for other archs.

> >> +     trace_kvm_irq_line(irq_level->irq % 2, irq_level->level, vcpu_idx);
> >> +
> >> +     if (irq_level->level) {
> >> +             vcpu->arch.virt_irq |= mask;
> >
> > racy - this is a vm ioctl, and so can (and usually is) invoked from
> > outside the vcpu thread.
> >
>
> this is taken care of in SMP host patch, but will be moved down the
> patches for next revision.

Yes please.  It's hard to review this way.  Fold all the smp stuff into
the patches which introduce the functionality.

>
> >> +             vcpu->arch.wait_for_interrupts = 0;
> >
> > Need an actual wakeup here (see x86's kvm_vcpu_kick() - should really be
> > common code; it takes care of both the 'vcpu sleeping and needs a
> > wakeup' and 'vcpu is in guest mode and needs to go to the host to
> > evaluate interrupt state').
> >
>
> the wakeup - same as above. Good point that we need to signal the
> other CPU. Will come, maybe not next revision, but the one after that.

Ok.  I think you can reuse x86 concepts and even code.  I'll accept
patches to make code arch independent ahead of the merge if it helps.


-- 
error compiling committee.c: too many arguments to function