Re: [Patch][RFC]Split kvm_vcpu to support new archs.

Zhang, Xiantao
2007-10-12T09:11:47Z

>> int vcpu_id;
>> struct mutex mutex;
>> int cpu;
>> - u64 host_tsc;
>> struct kvm_run *run;
>> int interrupt_window_open;
> I am not sure if this is the right thing for all archs. We have
> various forms of interrupts (I/O, external etc) which can all be
> masked seperately. I think interrupt_window_open should go to arch.

Thank you, I will resend it :)

>> int guest_mode;
>> unsigned long requests;
>> unsigned long irq_summary; /* bit vector: 1 per word in
>> irq_pending */
> We don't have irq. This works completely different for us, thus this
> needs to go to arch.
>
>> DECLARE_BITMAP(irq_pending, KVM_NR_INTERRUPTS); Same here.
>
>> #define VCPU_MP_STATE_RUNNABLE 0
>> #define VCPU_MP_STATE_UNINITIALIZED 1
>> #define VCPU_MP_STATE_INIT_RECEIVED 2
>> @@ -339,7 +329,6 @@ struct kvm_vcpu {
>> #define VCPU_MP_STATE_HALTED 4
>> int mp_state;
>> int sipi_vector;
> This one is arch dependent and should go to arch.
>
>> - u64 ia32_misc_enable_msr;
>>
>> struct kvm_mmu mmu;
>>
>> @@ -354,10 +343,6 @@ struct kvm_vcpu {
>>
>> struct kvm_guest_debug guest_debug;
>>
>> - struct i387_fxsave_struct host_fx_image;
>> - struct i387_fxsave_struct guest_fx_image;
>> - int fpu_active;
>> - int guest_fpu_loaded;
> I think guest_fpu_loaded should be generic. Don't you want to use the
> lazy fpu restore with preempt notification too?
>
>>
>> int mmio_needed;
>> int mmio_read_completed;
> This is arch dependent, we don't have CONFIG_MMIO.
>
>> @@ -365,7 +350,6 @@ struct kvm_vcpu {
>> int mmio_size;
>> unsigned char mmio_data[8];
>> gpa_t mmio_phys_addr;
>> - gva_t mmio_fault_cr2;
>> struct kvm_pio_request pio;
>> void *pio_data;
> All above are arch dependent.
>
>> diff --git a/drivers/kvm/kvm_arch.h b/drivers/kvm/kvm_arch.h
>> new file mode 100644
>> index 0000000..fe73d3d
>> --- /dev/null
>> +++ b/drivers/kvm/kvm_arch.h
>> @@ -0,0 +1,65 @@
>> +#ifndef __KVM_ARCH_H
>> +#define __KVM_ARCH_H
> This should go to x86.h, no new header please.
>
>> +struct kvm_arch_vcpu{
>> +
>> + u64 host_tsc;
>> +
>> + unsigned long regs[NR_VCPU_REGS]; /* for rsp:
>> vcpu_load_rsp_rip() */
>> + unsigned long rip; /* needs vcpu_load_rsp_rip() */ +
>> + unsigned long cr0;
>> + unsigned long cr2;
>> + unsigned long cr3;
>> + unsigned long cr4;
>> + unsigned long cr8;
>> + u64 pdptrs[4]; /* pae */
>> + u64 shadow_efer;
>> + u64 apic_base;
>> + struct kvm_lapic *apic; /* kernel irqchip context */ +
>> + u64 ia32_misc_enable_msr;
>> +
>> +
>> + struct i387_fxsave_struct host_fx_image;
>> + struct i387_fxsave_struct guest_fx_image;
>> + int fpu_active;
>> + int guest_fpu_loaded;
>> +
>> + gva_t mmio_fault_cr2;
>> +
>> + 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 cpuid_nent;
>> + struct kvm_cpuid_entry cpuid_entries[KVM_MAX_CPUID_ENTRIES]; +
>> + /* emulate context */
>> +
>> + struct x86_emulate_ctxt emulate_ctxt;
>> +};
>> +
>> +#endif
> Very nice. The only thing that should'nt be here is fpu_active as far
> as I can tell.

Since some archs don't need to care fpu, so I put it under arch. If most
archs need it, maybe we can move it to top level. Just a tradeoff.:)

> I like this split overall, per architecture vcpu data structures are
> an important step and clearly the right way to go.

> with kind regards,
> Carsten