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

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@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;
>         };
>
> 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.
>
>   

The usage of the macro is only for an intermediate stage, so this patch 
shows the changes in the data structures, while the next one will be 
littered with code changes due to the changes in the way fields are 
addressed.

I was initially in favor of doing

    struct kvm_vcpu {
        struct kvm_vcpu_common common;
        ...
    };

in order to avoid the majority of fields requiring an 'arch.' prefix 
(most fields are arch dependent, very few are common), but using 
container_of() as someone suggested seems to be a better idea.


> It does not make sense to share kvm_vcpu_ioctl_run(). Just look at it.
>
>   

Agree.


-- 
error compiling committee.c: too many arguments to function


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