From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH RFC 3/3] Break dependency between vcpu index in vcpus array and vcpu's apic id. Date: Thu, 21 May 2009 15:21:45 -0300 Message-ID: <20090521182145.GA11828@amt.cnet> References: <1242910606-29286-1-git-send-email-gleb@redhat.com> <1242910606-29286-4-git-send-email-gleb@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: avi@redhat.com, kvm@vger.kernel.org To: Gleb Natapov Return-path: Received: from mx2.redhat.com ([66.187.237.31]:45300 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753662AbZEUSWR (ORCPT ); Thu, 21 May 2009 14:22:17 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n4LIMJSr010682 for ; Thu, 21 May 2009 14:22:19 -0400 Content-Disposition: inline In-Reply-To: <1242910606-29286-4-git-send-email-gleb@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, May 21, 2009 at 03:56:46PM +0300, Gleb Natapov wrote: > Move decision on how to store vcpus into arch code. X86 finds first free > slot in vcpus array and uses id that userspace pass as vcpu's apic id. > Other arches keep using it as array index for now. > > Signed-off-by: Gleb Natapov > --- > arch/ia64/kvm/kvm-ia64.c | 25 ++++++++++++++++++--- > arch/powerpc/kvm/booke.c | 20 ++++++++++++++++- > arch/powerpc/kvm/powerpc.c | 7 +++++- > arch/s390/kvm/kvm-s390.c | 24 ++++++++++++++++++-- > arch/x86/include/asm/kvm_host.h | 1 + > arch/x86/kvm/lapic.c | 4 +- > arch/x86/kvm/x86.c | 45 ++++++++++++++++++++++++++++++++++++++- > include/linux/kvm_host.h | 7 +++++- > virt/kvm/kvm_main.c | 27 +--------------------- > 9 files changed, 122 insertions(+), 38 deletions(-) > > diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c > index 8a5911c..575765c 100644 > --- a/arch/ia64/kvm/kvm-ia64.c > +++ b/arch/ia64/kvm/kvm-ia64.c > @@ -1200,7 +1200,7 @@ out: > > #define PALE_RESET_ENTRY 0x80000000ffffffb0UL > > -int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) > +int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu, u32 id) > { > struct kvm_vcpu *v; > int r; > @@ -1212,6 +1212,8 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) > union context *p_ctx = &vcpu->arch.guest; > struct kvm_vcpu *vmm_vcpu = to_guest(vcpu->kvm, vcpu); > > + vcpu->vcpu_id = id; > + > /*Init vcpu context for first run.*/ > if (IS_ERR(vmm_vcpu)) > return PTR_ERR(vmm_vcpu); > @@ -1321,8 +1323,7 @@ fail: > return r; > } > > -struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, > - unsigned int id) > +struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id) > { > struct kvm_vcpu *vcpu; > unsigned long vm_base = kvm->arch.vm_base; > @@ -1332,6 +1333,9 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, > BUG_ON(sizeof(struct kvm_vcpu) > VCPU_STRUCT_SIZE/2); > > r = -EINVAL; > + if (!valid_vcpu_idx(id)) > + goto fail; > + > if (id >= KVM_MAX_VCPUS) { > printk(KERN_ERR"kvm: Can't configure vcpus > %ld", > KVM_MAX_VCPUS); > @@ -1365,6 +1369,19 @@ fail: > > int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu) > { > + struct kvm *kvm = vcpu->kvm; > + > + mutex_lock(&kvm->lock); > + if (kvm->vcpus[vcpu->vcpu_id]) { > + mutex_unlock(&kvm->lock); > + kvm_arch_vcpu_destroy(vcpu); > + return -EEXIST; > + } > + /* first one created is BSP */ > + if (!kvm->bsp_vcpu) > + kvm->bsp_vcpu = vcpu; > + kvm->vcpus[vcpu->vcpu_id] = vcpu; > + mutex_unlock(&kvm->lock); Why don't make a convention that vcpu_id 0 is the BSP, by default, instead of the first vcpu created? This way if userspace creates vcpu 3 first, there are no problems. That brings the question, why is the apic_id passed as the argument to vcpu_create? It seems that the argument to vcpu_create should be the kVM's internal vcpu_id, and the apic_id should come from somewhere else, apic_mmio_write maybe? AFAICS there is no linkage between apic_id and BSP (MP SPEC says BSP is determined by hardware and BIOS), but in KVM's BIOS case the BSP is conventioned to be vcpu with vcpu_id == 0, no? Another thing, it would be better if the linking of the vcpu in the array could be in arch independent code as it is today?