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

[Anthony Liguori <anthony-rdkfGonbjUSkNkDKm+mE6A@public.gmane.org>]

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 <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.
>
>   


-------------------------------------------------------------------------
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/