On Thu, May 21, 2009 at 03:21:45PM -0300, Marcelo Tosatti wrote:
> 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@xxxxxxxxxx>
> > ---
> > 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.
>
Because userspace no longer control vcpu_id allocation. I also want to
get rid of "first vcpu is bsp" restriction, but this fill require new
ioctl I guess.
> 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?
I think exactly opposite :) Why userspace should care what index vcpu
occupies in internal kernel data structure? This information is never
ever used by userspace again. And there is no reason to have vcpus array
at all. It can be linked list.
Apic_id on the other hand comes from HW (sampled from pins A11# and A12#
and pins BR0# through BR3# see Intel SDM 9.4.6) and has to be configured
at vcpu creation time before vcpu is made accessible (been put into vcpus
array) to other parts of the code (such as ioapic). And mmio_writes is not
the way to configure apic_id either. It is hard to send INIT/SIPI to a AP
cpu before its apic_id is configured and only cpu itself can do mmio_write
into its apic :) And this is what Intel SDM has to say about it:
Some processors permit software to modify the APIC ID. However,
the ability of software to modify the APIC ID is processor model
specific. Because of this, operating system software should avoid
writing to the local APIC ID register.
And indeed x2apic defines apic_id as non-writable.
>
> AFAICS there is no linkage between apic_id and BSP (MP SPEC says BSP
There is no linkage between apic_id and BSP. Way do you think I suggest
different?
> is determined by hardware and BIOS), but in KVM's BIOS case the BSP is
> conventioned to be vcpu with vcpu_id == 0, no?
Yes (except not vcpu_id == 0, but apic_id == 0), we will have to fix this
bug too. Bios should get acpi_ids of active cpus from qemu, not assume
anything.
> 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?
Possible, I will have to define kvm_arch_vcpu_find(id) function to
search for duplicates in generic code, but it will not be prettier.
I did it this way initially, but decided to hide everything in arch
code.
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
