Hollis Blanchard wrote:
> On Thu, 2007-10-18 at 15:34 +0800, Zhang, Xiantao wrote:
>
>> Hi Avi,
>> According to your and community's suggestions, I changed the kvm_vcpu
>> structure to two parts. To avoid the much intrusive into current code,
>> one is common part which is defined as a macro, and the other one is
>> arch-specific part.
>> In addition, I have a suggestion to re-organize the head files, such as
>> kvm.h and x86.h. IMO, kvm.h is changed to kvm_comm.h, and only includes
>> common code for all archs.Then x86.h will be changed to kvm-x86.h, and
>> linked as kvm.h at compile time. So, other archs also defines its
>> kvm-xx.h to accommodate its arch-specific structures. What's your ideas
>> ?(This idea doesn't include in this patch.)
>>
>
> First of all let me say that I hate cpp macros. What is the problem with
> embedding an architecture-specific sub-structure, i.e.
> struct kvm_vcpu {
> ...
> struct arch_kvm_vcpu arch_vcpu;
> };
>
I think you want the opposite direction of nesting.
There already is such a thing for vt/svm. What's needed is just another
level for x86/ppc, etc. All the necessary hooks are already in place to
allocate at the very bottom of the stack.
So to summarize, today we have:
struct kvm_vcpu {
/* stuff common to vt/svm and possibly other archs*/
};
struct vcpu_svm {
struct kvm_vcpu vcpu;
/* svm specific stuff */
};
But we should move to:
struct kvm_vcpu {
/* stuff common to x86/ppc/ia64 */
};
struct vcpu_x86 {
struct kvm_vcpu vcpu;
/* stuff common to vt/svm */
}
struct vcpu_svm {
struct vcpu_x86 vcpu;
/* svm specific stuff */
};
Regards,
Anthony Liguori
> This has a nice software engineering property too: common code will have
> to explicitly dereference "arch_vcpu", which in the best case wouldn't
> even compile, but even in the worst case is at least a visual red flag.
> The way you're using macros, there is nothing obviously wrong about
> "vcpu->host_tsc" in shared code.
>
> One more comment below.
>
>
>> >From 34cebd3a3fc0afba4df511219912bc3277e2a8c7 Mon Sep 17 00:00:00 2001
>> From: Zhang Xiantao <[EMAIL PROTECTED]>
>> Date: Thu, 18 Oct 2007 12:51:02 +0800
>> Subject: [PATCH] First step to split kvm_vcpu. Currently, we just use an
>> macro to define the common fields in kvm_vcpu for all archs, and all
>> archs need to define its own kvm_vcpu struct.
>> Signed-off-by: Zhang Xiantao <[EMAIL PROTECTED]>
>> ---
>> drivers/kvm/ioapic.c | 2 +
>> drivers/kvm/irq.c | 1 +
>> drivers/kvm/kvm.h | 166
>> +++++++-------------------------------------
>> drivers/kvm/kvm_main.c | 4 +-
>> drivers/kvm/lapic.c | 2 +
>> drivers/kvm/mmu.c | 1 +
>> drivers/kvm/svm.c | 2 +-
>> drivers/kvm/vmx.c | 1 +
>> drivers/kvm/x86.h | 128 ++++++++++++++++++++++++++++++++++
>> drivers/kvm/x86_emulate.c | 1 +
>> 10 files changed, 166 insertions(+), 142 deletions(-)
>>
> ...
>
>> +#ifdef CONFIG_HAS_IOMEM
>> +#define KVM_VCPU_MMIO \
>> + int mmio_needed;\
>> + int mmio_read_completed;\
>> + int mmio_is_write;\
>> + 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;
>> - wait_queue_head_t wq;
>>
>> - int sigset_active;
>> - sigset_t sigset;
>> +#else
>> +#define KVM_VCPU_MMIO
>>
>> - struct kvm_stat stat;
>> +#endif
>>
>
> ...
>
>
>> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
>> index aab465d..9ff049c 100644
>> --- a/drivers/kvm/kvm_main.c
>> +++ b/drivers/kvm/kvm_main.c
>> @@ -2272,7 +2272,7 @@ static int kvm_vcpu_ioctl_run(struct kvm_vcpu
>> *vcpu, struct kvm_run *kvm_run)
>> if (r)
>> goto out;
>> }
>> -
>> +#if CONFIG_HAS_IOMEM
>> if (vcpu->mmio_needed) {
>> memcpy(vcpu->mmio_data, kvm_run->mmio.data, 8);
>> vcpu->mmio_read_completed = 1;
>> @@ -2287,7 +2287,7 @@ static int kvm_vcpu_ioctl_run(struct kvm_vcpu
>> *vcpu, struct kvm_run *kvm_run)
>> goto out;
>> }
>> }
>> -
>> +#endif
>>
>
> It does not make sense to share kvm_vcpu_ioctl_run(). Just look at it.
>
> FYI, "char mmio_data[8]" has alignment problems. PowerPC has
> endian-reversed load/store instructions, and to use them target data
> must be aligned.
>
> Also, memcpy() doesn't work for big-endian systems with sub-word loads.
> Imagine if I do a single-byte load: "memcpy(&gpr, mmio_data, 1)" would
> set the MSB, but the byte should land in the LSB of the register.
>
>
-------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel
