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

Hollis Blanchard
2007-10-19T17:16:40Z

On Thu, 2007-10-18 at 17:04 -0500, Anthony Liguori wrote:
> Hollis Blanchard wrote:
> >
> > 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).

Ah, so you're saying that once we have a "to_x86()" macro, the nesting
becomes irrelevant.

Furthermore, "to_x86()" won't even be defined when compiling shared
code, which will ensure that nobody tries to slip something in.

Finally, it means we don't have to keep the "arch" member at the end of
kvm_vcpu (though I don't think that's a big deal really).

I think I can live with that. Carsten?

[BTW, a quick glance at svm.c turns up weirdness like this:

static int rdmsr_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
{
u32 ecx = svm->vcpu.regs[VCPU_REGS_RCX];
u64 data;

if (svm_get_msr(&svm->vcpu, ecx, &data))
svm_inject_gp(&svm->vcpu, 0);
...
}

static void svm_inject_gp(struct kvm_vcpu *vcpu, unsigned error_code)
{
struct vcpu_svm *svm = to_svm(vcpu);
...
}

There's some unnecessary kvm_vcpu/svm_vcpu conversion going on there.]

--
Hollis Blanchard
IBM Linux Technology Center