Keir Fraser wrote: > On 25 Jul 2006, at 07:44, Gerd Hoffmann wrote: > > >>> Actually, I think that it should be something like: >>> #ifdef CONFIG_PARAVIRT >>> #define __FIXADDR_TOP (paravirt_ops.fixaddr_top) >>> #endif >>> >>> Then paravirt_ops.init will set this value in the paravirt_ops struct >>> (just as it will set kernel_rpl). >>> >> What is wrong with simply calling set_fixaddr_top() in >> paravirt_ops.init? I don't see the point of moving that into the >> paravirt_ops struct ... >> > > It makes sense to avoid this if there are other users of variable > fixaddr_top. I generally prefer idea of the idea of poking values from > paravirt init code rather than cluttering paravirt_ops with data > fields. > If we want a purely functional interface to encapsulate the data, it makes sense to have a get_linear_top() instead of a set_fixaddr_top(), as the set code is totally redundant for each paravirt_op backend, and also it is not really a paravirt operation to set the internal Linux memory top - this is really a hook back into Linux, and it is cleaner to have the hooks going one way (from Linux to PV-ops) where possible. It also depends on how Linux orders the memory management set up when this becomes relevant, and thus I much prefer the following sequence: 1) Init code fills in pv_ops struct 2) Linux does a bunch of stuff - now about to set up pagetables on BSP 3) set_fixaddr_top(pv_ops->get_linear_top()) (once only, in arch/i386/mm/init.c) Otherwise, one can imagine a sequence where someone thinks they need to add the "missing" set_fixaddr_top to the native code path, and the top we set at init time now gets overridden in the pv case. Surely, not a real concern as the bug will be fixed soon enough, but it does show the conceptual problem with having this in multiple disparate init paths. Zach
