[Xen-ia64-devel] RE: Rebased xen-ia64 tree with VT-i support

["Yang, Fred" <fred.yang@xxxxxxxxx>]

Forward to xen-ia64-developer mailing list
Tian, Kevin wrote:
> Hi, Dan,
>       Several comments and free to argue. :)
> 
> One thing to clarify first is: all of our code is enclosed by
> CONFIG_VTI by far, including both real VTI specific code and also
> some common enhancements. To adding such macro everywhere is to
> follow our agreement when f2f several months ago, for better track
> what we changed effectively before finally merged.    
> 
> So ideally we can merge the code keeping CONFIG_VTI and then remove
> those not specific to VTI gradually based on further agreement like
> what you said for ia64_psr.  
> 
>> 
>> 1) Makefile: Does CONFIG_VTI default to on?  Probably
>>   shouldn't since nobody outside of Intel can use it.
> 
> Agree.
> 
>>   b) Rename and move the VTI-specific versions of
>>      functions (to vmx_domain.c?)
> 
> If you look into the domain create function surrounded by CONFIG_VTI,
> it already considers both VTI and Non-VTI domain creation. Actually
> domain creation should be common with tiny diverge, based on
> dynamical detect about VTI feature. Both arch_do_createdomain and
> new_thread are such cases. Being this reason, I still prefer to leave
> them in domain.c now. :)     
> 
>> 3) mm_init.c: (minor) move VTI-specific code to
>>   separate function/file and call it at same point?
> 
> That's not VTI-specific code, which actually is alternative approach
> for HV address space, meaning whether co-exist with domain or in
> separate space. But your comment sounds good even from coding style
> point of view.   
> 
>> 5) xensetup.c: the comment re 64G memory is inconsistent
>>   with the panic message
> 
> Typo, thanks for careful review. :)
> 
>> 
>> Patch files:
>> 
>> 1) move vmx_ia64_handle_irq out of irq_ia64.c and
>>   into xenirq.c
> 
> This is just to sync with ia64_handle_irq, which still exists in
> irq_ia64.c. If you move xen specific version like xen_handle_irq,
> then its easy for us to follow that direction, since they same type
> should be in same place   
> 
>> 3) head.S: Group all the ia64_rid changes into one
>>   #ifdef XEN (no need to specify CONFIG_VTI here
>>   as it might apply to non-VTI too?)  Also, missing
>>   #ifdef surrounding the ia64_rid line change.
>>   (This should also be submitted to Tony... asm
>>   code shouldn't depend on certain values of
>>   defined constants/expressions, especially in
>>   non-performance critical code like this!)
> 
> Agree, but the point still is whether we merge first and then remove
> some CONFIG_VTI based on requirement, or remove them before merge... 
> 
>> 6) page.h: RR7_SWITCH_SHIFT can go into a new header
>>   file (or maybe regionregs.h) as it is only used
>>   in one (non-patched) file.
> 
> OK
> 
>> 4) ia64regs.h: same comment as 2)
>> 5) kregs.h: same comment as 2)
>> 7) processor.h: same comment as 2)... also since
>>   struct ia64_psr is changed but used only in
>>   vti-specific code, why not define a new structure
>>   outside of processor.h and use it where needed?
>>   (Also, submit the change to ia64_psr
>>   to Tony since the architectural definition will
>>   soon be changing.)
>> 8) system.h: same as 2)
> 
> I'm concern whether it's worthy of time to create so many new files
> just because we have some new generic definitions. Ideally, I believe
> finally XEN/IPF will go to flatten after removing about 99% unused
> Linux stuff. At that time, saying kregs.h, it becomes xen's kregs.h
> and free to add new stuff without "#ifdef XEN" any more. Why bother
> to waste time to add xen_kregs.h now and then remove and merge them
> again later? Either one patch file and one new file, you need to add
> one. Especially such definition seldom changes and should go where it
> should. It is different as new xentime.c which differs on logic. :)  
> 
> Thanks,
> Kevin


_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel