[added back cc]
Paul Turner wrote:
> On 7/13/07, Avi Kivity <[EMAIL PROTECTED]> wrote:
>> Paul Turner wrote:
>> > From: Paul Turner <[EMAIL PROTECTED]>
>> >
>> > - 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/
_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel
