Re: [PATCH RFC 3/3] Break dependency between vcpu index in vcpus array and vcpu's apic id.

[Marcelo Tosatti <mtosatti@redhat.com>]

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 <gleb@redhat.com>
> ---
>  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?