Re: [PATCH] dynamically create vcpus + vmx/svm structures

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

[added back cc]

Paul Turner wrote:
> On 7/13/07, Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:
>> Paul Turner wrote:
>> > From: Paul Turner <pjt-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>> >
>> > - vcpus now allocated on demand
>> > - vmx/svm fields separated into arch specific structures on vcpus
>> > - vmx/svm fields now only allocated on corresponding architectures
>> >
>> > - Paul
>> >
>> > diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
>> > index 0f7a4d9..c631192 100644
>> > --- a/drivers/kvm/kvm.h
>> > +++ b/drivers/kvm/kvm.h
>> > @@ -16,6 +16,7 @@ #include <linux/mm.h>
>> >  #include <asm/signal.h>
>> >
>> >  #include "vmx.h"
>> > +#include "kvm_svm.h"
>>
>> This can probably be avoided, see below.
>>
>> >  #include <linux/kvm.h>
>> >  #include <linux/kvm_para.h>
>> >
>> > @@ -326,16 +327,64 @@ struct kvm_io_device *kvm_io_bus_find_de
>> >  void kvm_io_bus_register_dev(struct kvm_io_bus *bus,
>> >                   struct kvm_io_device *dev);
>> >
>> > +struct kvm_vmx_data {
>> > +    int msr_offset_efer;
>> > +
>> > +        #ifdef CONFIG_X86_64
>> > +    int msr_offset_kernel_gs_base;
>> > +        #endif
>> > +
>> > +        struct vmx_host_state {
>> > +        int loaded;
>> > +        u16 fs_sel, gs_sel, ldt_sel;
>> > +        int fs_gs_ldt_reload_needed;
>> > +    } host_state;
>> > +
>> > +    struct vmx_msr_entry *guest_msrs;
>> > +    struct vmx_msr_entry *host_msrs;
>> > +
>> > +    struct {
>> > +        int active;
>> > +        u8 save_iopl;
>> > +        struct kvm_save_segment {
>> > +            u16 selector;
>> > +            unsigned long base;
>> > +            u32 limit;
>> > +            u32 ar;
>> > +        } tr, es, ds, fs, gs;
>> > +    } rmode;
>> > +    int halt_request; /* real mode */
>> > + +    struct vmcs *vmcs;
>> > +};
>> > +
>>
>> If this is moved to vmx.c, we can avoid including vmx.h and have no arch
>> dependent code here (given that we don't even need the size).
>>
>
> I originally did this however gcc refuses to compile with the
> incomplete types, although after further investigation it turns out
> it's a bug in gcc with an incomplete implementation of zero sized
> arrays under a union, so I can fix this now.  See notes below..
>

Looks like you forgot the notes below :)

Anyway the only fix I can see is to have a long[0] member at the end,
and have vmx.c define a function vmx(vcpu) which returns the vmx
specific data.  Accesses would look like

  vmx(vcpu)->cr0 = 42;

which is odd, but I've seen worse.  But if you have a better solution,
let's hear it.

>> > +struct kvm_svm_data {
>> > +    struct vmcb *vmcb;
>> > +    unsigned long vmcb_pa;
>> > +    struct svm_cpu_data *svm_data;
>> > +    uint64_t asid_generation;
>> > +
>> > +    unsigned long db_regs[NUM_DB_REGS];
>> > +
>> > +    u64 next_rip;
>> > +
>> > +    u64 host_user_msrs[NR_HOST_SAVE_USER_MSRS];
>> > +    u64 host_gs_base;
>> > +    unsigned long host_cr2;
>> > +    unsigned long host_db_regs[NUM_DB_REGS];
>> > +    unsigned long host_dr6;
>> > +    unsigned long host_dr7;
>> > +};
>>
>> This can remain in kvm_svm.h.
>
> I was going to move both structures out in a small follow up patch, I
> didn't in this one because of the compile issue above + not wanting to
> make this patch any larger than it already is.. :)  I can merge it
> into this one if you prefer..

Don't understand.  If it remains in kvm_svm.h, the patch gets smaller,
not larger.

>
>>
>>
>> > +
>> > +
>> >  struct kvm_vcpu {
>> >      struct kvm *kvm;
>> > +    struct mutex *mutex; /* refers to corresponding vcpu_mutex on
>> kvm */
>>
>> Please keep this as a real structure, not a pointer.  Existence testing
>> of the vcpu is now simply if (kvm->vcpus[slot]).
>
> Some of the existing code makes the assumption that locking the cpu
> locks the slot as well; also if we don't have an associated lock/mutex
> then we'd have to take a global lock on slot updates/checks.  Finally
> you'd still have a race in between checking it's a valid vcpu and
> trying to acquire it's mutex..

The only place the race matters is in vcpu creation.  There, we can do
something like

    vcpu = kvm_arch_ops->vcpu_create(...);
    spin_lock(kvm);
    if (kvm->vcpus[slot]) {
        r = -EEXIST;
        vcpu_free(vcpu);
    } else
        kvm->vcpus[slot] = vcpu;
    spin_unlock(kvm);

In the other places, if the user has a thread creating a vcpu and
another thread performing an operation on it, it's perfectly legitimate
to return -ENOENT.

>> > -    page = alloc_page(GFP_KERNEL | __GFP_ZERO);
>> > +    vcpu->mutex = &kvm->vcpu_mutex[n];
>> > +    vcpu->cpu = -1;
>> > +    vcpu->kvm = kvm;
>> > +    vcpu->mmu.root_hpa = INVALID_PAGE;
>> > +
>> > +    vcpu->vcpu_id = n;
>> > +    kvm->vcpus[n] = vcpu;
>> > +
>> > +    run_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
>> >      r = -ENOMEM;
>> > -    if (!page)
>> > -        goto out_unlock;
>> > -    vcpu->run = page_address(page);
>> > +    if (!run_page)
>> > +        goto out_deallocate;
>> > +    vcpu->run = page_address(run_page);
>> >
>>
>> This cleanup is good, but makes the patch larger.  Please defer it.
>>
>
> this needs to be done on vcpu creation (it was part of vm init
> before), if you're concerned about patch size I can break up the
> structure separation and dynamic allocation fairly easily since they
> are different commits in my repository (I just didn't originally want
> to rebase them both :)

Yes, you're right -- it is necessary.  It can live in the main patch.

>
> please advise on splits/etc as above and ill resubmit
>

One patch is okay.  We should aim for kvm_main.c not knowing (including)
anything about vmx or svm.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/