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;
};
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.
--
Hollis Blanchard
IBM Linux Technology Center
-------------------------------------------------------------------------
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
