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

Hollis Blanchard
2007-10-18T21:43:18Z

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;
}

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

--
Hollis Blanchard
IBM Linux Technology Center