Re: [PATCH v5 11/13] ARM: KVM: Support SMP hosts

[Avi Kivity <avi@redhat.com>]

On 12/12/2011 07:37 PM, Christoffer Dall wrote:
> >
> > This looks overly complicated with two levels of locks.  x86 gets by
> > with no locks, and a much more complicated interrupt architecture.
> >
> > My recommendation is:
> >  wait_for_interrupts is managed solely by the vcpu thread
> >  KVM_IRQ_LINE does a set_bit(, virt_irq) for the appropriate irq type,
> > then IPI/wakeups the vcpu to make it examine both wait_for_interrupts
> > and virt_irq.
> >
> >
> this sounds pretty good to me.
>
> something like this:
>
> if (irq_level->level) {
> 	set_bit(&vcpu->arch.irq_lines, bit_nr);
> 	smp_mb();
> 	wake_up_interruptible(&vcpu->wq);

or, smp_send_reschedule().  See kvm_vcpu_kick().

An optimization: do a cmpxchg() and don't wakeup if the operation raised
IRQ file FIQ was set (assuming that FIQ has a higher priority than IRQ).

> } else
> 	clear_bit(&vcpu->arch.irq_lines, bit_nr);
>
>
> and the vcpu thread would clear the wait_for_interrupts flag if it
> ever sees the mask field be non-zero?

Yes.  This is what x86 does, except it's a lot more complicated.

> >> @@ -522,47 +573,42 @@ static int init_hyp_mode(void)
> >>               return -ENOMEM;
> >>
> >>       /*
> >> -      * Allocate stack page for Hypervisor-mode
> >> +      * Allocate stack pages for Hypervisor-mode
> >>        */
> >> -     kvm_arm_hyp_stack_page = (void *)__get_free_page(GFP_KERNEL);
> >> -     if (!kvm_arm_hyp_stack_page) {
> >> -             err = -ENOMEM;
> >> -             goto out_free_pgd;
> >> -     }
> >> +     for_each_possible_cpu(cpu) {
> >> +             void *stack_page;
> >>
> >> -     hyp_stack_ptr = (unsigned long)kvm_arm_hyp_stack_page + PAGE_SIZE;
> >> +             stack_page = (void *)__get_free_page(GFP_KERNEL);
> >
> > Best to allocate this (and other per-cpu state) on the cpu's node.
> >
>
> why, for performance reasons? 

Yes, I'm assuming that all multi-socket A15s will be numa?

> The code get slightly more complicated,
> since we have to pass the return value through the argument so we have
> to pass an opaque pointer to the struct or something like that.

Don't see why, just use alloc_pages_node().

> >>
> >> +     for_each_online_cpu(cpu) {
> >> +             smp_call_function_single(cpu, cpu_init_hyp_mode,
> >> +                                      (void *)(long)init_phys_addr, 1);
> >> +     }
> >
> > Need similar code for cpu hotplug.  See kvm_cpu_hotplug() and
> > kvm_arch_hardware_enable() which do all this for you.
> >
> so just to be sure, this will only be called for cpus that are
> hotplugged right? we still call the cpu_init_hyp_mode for each cpu
> that's online at this point.

The infrastructure will call kvm_arch_hardware_enable() for all
currently online cpus and any future hotplugged cpu.  Just follow
kvm_init() and fill in the arch callbacks.  You do have a call to
kvm_init() somewhere, yes?

> Do we need some locking to make sure the two don't overlap (like
> should I grab the kvm_lock here)?

Let kvm_init() do the driving and relax.

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