From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] move structures to svm/vmx.c, use untyped arch_data in independent code Date: Thu, 19 Jul 2007 11:33:22 +0300 Message-ID: <469F21D2.3010605@qumranet.com> References: <469EAC5A.2070900@codemonkey.ws> <469EBB26.5080106@codemonkey.ws> <469ECF13.6050004@codemonkey.ws> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, avi-atKUWr5tajDk1uMJSBkQmQ@public.gmane.org To: Anthony Liguori Return-path: In-Reply-To: <469ECF13.6050004-rdkfGonbjUSkNkDKm+mE6A@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org Anthony Liguori wrote: > Paul Turner wrote: > >>>>>> + struct vmcs *vmcs; >>>>>> >>> + struct kvm_vcpu vcpu; >>> >>> >> In this approach you might as well embed that at the start so that the >> arch independent code can still allocate/free, that or move the memory >> alloc/dealloc entirely to the arch specific code. Although that >> should probably be done anyway with this approach otherwise it's not >> clear exactly what is occurring from the arch independent code path's >> pov. >> > > If it's not too much churn, I'd think that allocating the vcpu structure > in the arch dependent code would make the most sense since it's that > code that knows how large the structure should be. > > >>>>>> #define IOPM_ALLOC_ORDER 2 >>>>>> #define MSRPM_ALLOC_ORDER 1 >>>>>> >>>>>> @@ -95,7 +115,7 @@ static inline u32 svm_has(u32 feat) >>>>>> >>>>>> static unsigned get_addr_size(struct kvm_vcpu *vcpu) >>>>>> { >>>>>> - struct vmcb_save_area *sa = &vcpu->svm->vmcb->save; >>>>>> + struct vmcb_save_area *sa = &svm(vcpu)->vmcb->save; >>>>>> >>> I think this needs to be: >>> >>> struct kvm_svm_data *svm = kvm_vcpu_to_svm_data(vcpu); >>> struct vmcb_save_area *sa = &svm->vmcb->save; >>> >>> The rest should just follow the same style. >>> >> Motivating reason? >> > > Clarity on the part of the reader. The problem with using a macro like > this is that it's not very clear in this function what the type of > svm(vcpu) is. There's also a bit of magic as to how svm() gets the > whatever the type is from a vcpu. Not to mention the fact that svm() is > a horrible function name :-) > > By using the typeX_to_typeY idiom, it's very clear to the reader what's > going on. It's just another embedded structure who's conversion > function is container_of() based. > Generally I'd agree, but in this case the function is oft-used and limited in scope to just one file. So maybe: struct kvm_svm_data *svm = __svm(vcpu); An advantage of embedding vcpu into svm/vmx is that the offsetoff macros used to feed inline assembly still work. -- error compiling committee.c: too many arguments to function ------------------------------------------------------------------------- This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/