Re: [PATCH] move structures to svm/vmx.c, use untyped arch_data in independent code

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.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/