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

["Magenheimer, Dan (HP Labs Fort Collins)" <dan.magenheimer@xxxxxx>]

Hi Kevin --

Since Fred forwarded this to the main list, I will reply there.  If
there's someone on the original distribution list who isn't on
xen-ia64-devel, feel free to forward.

>       Several comments and free to argue. :)

Who?  Me 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.

Understood... I wasn't suggesting changing the CONFIG_VTI, just
suggesting some code maintainability improvements.

> >   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. :)

OK.
 
> >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

Maybe I misunderstand, but xen_handle_irq was recently
moved to xenirq.c.  I was suggesting that vmx_ia64_handle_irq
move there also.
 
> >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. :)

For header patches that require only a few lines of change
and are expected to rarely or never change, I agree it is
a waste of time to create new files.  For header patches with
dozens or hundreds of lines changed or lines that are expected
to change somewhat frequently, the "sub-include" may be
better.  This will make it more clear what new code
is added to support Xen... and also reduce complaints from Arun
about the extra step required to checkin changes to patched
files ;-)

Let's settle on splitting out the code from processor.h
and system.h into separate xen_processor.h and xen_system.h
(and perhaps gcc_intrin.h unless you're sure this is stable).

If your estimate of 99% is anywhere close to accurate,
flattening would be best.  I think it is probably closer
to 10-20% and flattening sacrifices long-term maintainablity
and functionality leverage for short-term developer
convenience.

Dan

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