Avi Kivity wrote:
> Zhang, Xiantao wrote:
>>> From 12457e0fb85ef32f1a1f808be294bebe8d22667c Mon Sep 17 00:00:00
>>> 2001
>> From: Zhang xiantao <[EMAIL PROTECTED]>
>> Date: Fri, 12 Oct 2007 13:29:30 +0800
>> Subject: [PATCH] Split kvm_vcpu to support new archs. Define a new
>> sub field kvm_arch_vcpu to hold arch-specific sections.
>>
>> I am not sure data fields related to mmu should put under
>> kvm_arch_vcpu or not, because IA64 side doesn't need them, and only
>> need kvm module to allocate memory for guests.
>>
>>
>
>
> The patch is a good start, but too big... I think there are three
> possible approaches to make the split:
>
> 1: kvm_vcpu is common, with arch specific fields in a member
>
> struct kvm_arch_vcpu {
> ... x86 specific fields ...
> };
>
> struct kvm_vcpu {
> ... common fields...
> struct kvm_arch_vcpu arch;
> };
>
> 2. kvm_vcpu is arch specific, with common fields in a member
>
> struct kvm_vcpu_common {
> ... common fields ...
> };
>
> struct kvm_vcpu {
> struct kvm_vcpu_common common;
> ... x86 specific fields ...
> };
>
> 3. kvm_vcpu contains both common and arch specific fields:
>
> #define KVM_VCPU_COMMON_FIELDS \
> struct kvm *kvm; \
> struct preempt_notifier *preempt_notifier; \
> ...
>
> struct kvm_vcpu {
> KVM_VCPU_COMMON_FIELDS
> ... x86 specific fields ....
> };
>
> I prefer the second approach to the first one since most fields are
> arch specific, and this allows most fields to be accessed directly
> without "arch." in front of every field. However I'd like to start
> with the third approach since it is least intrusive (the downside is
> that it is a little dirty from a C point of view).
Agree. OK, Maybe we can begin with the third one.
> I think you left out some of the arch specific fields:
>
>> struct kvm_vcpu {
>> struct kvm *kvm;
>> struct preempt_notifier preempt_notifier;
>> int vcpu_id;
>> struct mutex mutex;
>> int cpu;
>> - u64 host_tsc;
>> struct kvm_run *run;
>>
>
>> int interrupt_window_open;
>>
>
> arch specific, though maybe it's close enough to generic
OK, Maybe we can move it to arch first. If it is neeed for most archs,
we can move them back.
>> int guest_mode;
>> unsigned long requests;
>>
>
>> unsigned long irq_summary; /* bit vector: 1 per word in
>> irq_pending */
>> DECLARE_BITMAP(irq_pending, KVM_NR_INTERRUPTS);
>>
>
> irq_pending and irq_summary are probably arch specific
At least IA64 need them, but anyway, we can put them under arch.
>> - 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 */
>> #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;
>>
>
> mp_state (probably) and sipi_vector (certainly)
I suppose put them under arch first.
>> - u64 ia32_misc_enable_msr;
>>
>> struct kvm_mmu mmu;
>>
>
> mmu
About mmu, i am not sure about this. IA64 only need kvm module to
allocate and free memory for guests. Most functions related to mmu are
useless for IA64. If we put them as common, Maybe we have to define some
blank functions.
>>
>> @@ -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;
>>
>> int mmio_needed;
>> int mmio_read_completed;
>> @@ -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;
>>
>
> pio is x86 specific
Why see pio as x86 specific? as I know, most archs have port IO access .
>> wait_queue_head_t wq;
>> @@ -375,24 +359,9 @@ struct kvm_vcpu {
>>
>> struct kvm_stat stat;
>>
>> - 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 on Intel only */
>>
>
> this too
Agree.
>> + struct kvm_arch_vcpu arch; /*Arch-specific fields*/
>>
>> - int cpuid_nent;
>> - struct kvm_cpuid_entry cpuid_entries[KVM_MAX_CPUID_ENTRIES]; -
>> - /* emulate context */
>> -
>> - struct x86_emulate_ctxt emulate_ctxt;
>> };
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
