From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Egger Subject: Re: [PATCH 03/14] Nested Virtualization: data structure Date: Tue, 17 Aug 2010 12:48:36 +0200 Message-ID: <201008171248.37216.Christoph.Egger@amd.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Keir Fraser Cc: "xen-devel@lists.xensource.com" , "Dong, Eddie" List-Id: xen-devel@lists.xenproject.org On Tuesday 17 August 2010 12:28:17 Keir Fraser wrote: > On 17/08/2010 11:01, "Christoph Egger" 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