Re: [kvm-ppc-devel] [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 16:31 -0500, Anthony Liguori wrote:
>   
>> Hollis Blanchard wrote:
>>     
>>> On Thu, 2007-10-18 at 16:04 -0500, Anthony Liguori wrote:
>>>   
>>>       
>>>> Hollis Blanchard wrote:
>>>>     
>>>>         
>>>>> 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. 
>>>>     
>>>>         
>>> ...
>>>   
>>>       
>>>> 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  */
>>>> };
>>>>     
>>>>         
>>> Why?
>>>   
>>>       
>> It provides better encapsulation.  If you have a kvm_vcpu, unless you do 
>> container_of(), you can't access the arch_vcpu.  It helps make sure that 
>> architecture common code remains common.
>>     
>
> I must be misunderstanding, because this seems completely backwards to
> me. With your nesting, any time architecture code wants to access
> architecture state (which is almost all the time), you'd *need*
> container_of:
>
>         void arch_func(struct kvm_vcpu *vcpu) {
>                 struct arch_vcpu *arch = container_of(vcpu, arch_vcpu,
>                 arch);
>                 arch->gpr[3] = 0;
>         }
>
> In contrast, my nesting proposal would look like this:
>
>         void arch_func(struct kvm_vcpu *vcpu) {
>                 vcpu->arch.gpr[3] = 0;
>         }
>   

Well, you'd probably define a to_ppc() and then do something like:

void arch_func(struct kvm_vcpu *vcpu) {
       to_arch(vcpu)->gpr[3] = 0;
}

Which is exactly what's done in the vt/svm backend (see usage of 
to_svm/to_vmx).

Regards,

Anthony Liguori

>> It also leaves open the possibility of supporting multiple architectures 
>> at the same time.  I don't know why you would want to do that :-)
>>     
>
> That's true, though this could also be accomplished by keeping arch_vcpu
> as the last member of kvm_vcpu.
>
>   


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