Re: [PATCH 03/14] Nested Virtualization: data structure

[Christoph Egger <Christoph.Egger@amd.com>]

On Tuesday 17 August 2010 12:28:17 Keir Fraser wrote:
> On 17/08/2010 11:01, "Christoph Egger" <Christoph.Egger@amd.com> wrote:
> >> Yes, we should be strict on the layout of this structure.
> >> SVM/VMX-specific stuff goes into a sub-structure in a union. Absolutely.
> >
> > I have moved the SVM/VMX-specific pieces into the 'void *nh_arch' field
> > above. It is initialized in the svm/vmx specific vcpu initialization.
>
> I suggest to make this a union including SVM/VMX-specific struct pointers.
> It will avoid unnecessary explicit casting, and you can use an anonymous
> union if you want. Is using pointers better than actually including the
> structures in the union, do you think?
>
> So I mean something like: union {
>     void *nh_arch;
>     struct nestedsvm *nh_svm;
>     struct nestedvmx *nh_vmx;
> };
>
> Or: union {
>     struct nestedsvm nh_svm;
>     struct nestedvmx nh_vmx;
> };

All of this is possible but a union is actually only needed if you want
to access svm or vmx specific data from common code which is
bad from the software engineering side.
The advantage of using a pointer is that gcc can point you to
such a mistake.

In svm/vmx code you don't need explicit casts with a pointer.
Have a look at the top of the nsvm_vmcb_prepare4vmrun() function
in the nh_svm patch. There you see how I access 'struct nestedsvm'
w/o a cast.

> What is the nh_arch_size field for? Well I can guess what it represents,
> but why do you need such a thing?

It's purpose is to allow to copy svm/vmx specific data to somewhere else
w/o knowing them. It is currently nowhere needed. Once it turns out
it is neither needed for VMX it can go away.

> > When you look into the svm specific patch, you will find a 'struct
> > nestedsvm' in xen/include/asm-x86/hvm/svm/vmcb.h
> >
> >> And you would only go peeking at the SVM sub-structure
> >> if hvm_svm_enabled(v)==TRUE.
> >
> > Correct. On a Intel CPU Xen should never allow the guest to
> > set the EFER.SVME bit.
> >
> >> And we'd have a similar predicate hvm_vmx_enabled(v)==TRUE, presumably.
> >> And maybe a generic hvm_nestedvirt_enabled(v) too.
> >
> > What you call hvm_nestedvirt_enabled() actually exists as
> > nestedhvm_enabled().
>
> Fine.
>
>  -- Keir



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632