On Tue, 2012-02-07 at 17:38 -0200, Marcelo Tosatti wrote: > On Tue, Feb 07, 2012 at 05:32:07PM +1100, Michael Ellerman wrote: > > A test case which does the following: > > > > ioctl(vmfd, KVM_CREATE_VCPU, 0); > > ioctl(vmfd, KVM_CREATE_IRQCHIP); > > ioctl(cpufd, KVM_RUN); > > > > Can oops in kvm_apic_accept_pic_intr() because vcpu->arch.apic == NULL. > > > > Because irqchip_in_kernel() is false when we create the vcpu we leave > > vcpu->arch.apic uninitialised (in kvm_arch_vcpu_init()). Then when we run, > > irqchip_in_kernel() is true, but we didn't do the correct initialisation. > > > > The root of the problem seems to be that there is an assumption that > > KVM_CREATE_IRQCHIP will be called before any VCPUs are created. The > > documentation says "sets up future vcpus to have a local APIC". > > > > So the simplest fix seems to be to enforce that ordering in the code. > > Ugh. With your patch below there is still the window for a race: > > kvm_arch_vcpu_init can create a vcpu without vcpu->arch.apic, > block on mutex_lock(kvm->lock). Meanwhile a separate thread is on > KVM_CREATE_IRQCHIP holding kvm->lock, finds online_vcpus == 0 and > proceeds. Then kvm_arch_vcpu_init finishes. Yeah bugger. I missed that most of the vcpu create is done without the mutex held. > Moving mutex_lock(kvm->lock) to the beginning of > kvm_vm_ioctl_create_vcpu should fix it? Yeah I think so. The slight problem is that arch code in the formerly unlocked vcpu create path might take the mutex, powerpc does, but AFAICS that is the only arch that does. So as long as we remove that it should work. I'm travelling today and tomorrow but I'll get you a new patch as soon as I get a chance. cheers
Attachment:
signature.asc
Description: This is a digitally signed message part
