Re: [PATCH] Split kvm_vcpu to support new archs.

[Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

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 <xiantao.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 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 <xiantao.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  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/