Re: [kvm-ppc-devel] [PATCH] Split kvm_vcpu to support new archs.

Anthony Liguori
2007-10-18T22:04:46Z

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.