Rusty Russell wrote:> Chris: [RFC PATCH 12/33] Change __FIXADDR_TOP to leave room for the hypervisor. > - Replace with dynamic (Geerd) patch, put in paravirt_ops structure. >I'm looking at this, and I'm not sure that there needs to be a void (*set_fixmap_top)(unsigned long top) entry in paravirt ops. It seems to me that the hypervisor's init code can call the normal set_fixmap_top() function (which Geerd's patch introduces) with whatever appropriate value it needs when it also installs the rest of the paravirt ops pointers and/or patches. After all, this is something the hypervisor needs to set in the kernel, rather than something the kernel needs to do to the hypervisor. Does that sound reasonable, or does there really need to be a paravirt entry for this? Also, since this is a pretty generic patch with no earlier dependencies, we can push it near the front of the patch queue. J
Jeremy Fitzhardinge wrote:> Rusty Russell wrote: >> Chris: [RFC PATCH 12/33] Change __FIXADDR_TOP to leave room for the >> hypervisor. >> - Replace with dynamic (Geerd) patch, put in paravirt_ops structure. >> > I'm looking at this, and I'm not sure that there needs to be a void > (*set_fixmap_top)(unsigned long top) entry in paravirt ops. It seems > to me that the hypervisor's init code can call the normal > set_fixmap_top() function (which Geerd's patch introduces) with > whatever appropriate value it needs when it also installs the rest of > the paravirt ops pointers and/or patches. After all, this is > something the hypervisor needs to set in the kernel, rather than > something the kernel needs to do to the hypervisor. > > Does that sound reasonable, or does there really need to be a paravirt > entry for this?Well there are a couple of options - 1) A set_fixmap_top paravirt op - which doesn't work so well, since all paravirt subops need to re-use the same callouts to Linux, so there are twice (3x, 4x...) as many points to change if the Linux API changes. 2) A get_fixmap_top paravirt op - which allows the common code to just always set the fixmap in the normal startup routine, and also works for native by just returning 0xffffe000 (or should it be 0x00000000)? 3) Pass the information in at boot, or via a special argument / environment page. I like #2, since it seems most logical, and doesn't preclude doing #3 later if we decide to create such a arg / envp page. Zach
On Sun, 2006-07-23 at 17:54 -0400, Jeremy Fitzhardinge wrote:> Rusty Russell wrote: > > Chris: [RFC PATCH 12/33] Change __FIXADDR_TOP to leave room for the hypervisor. > > - Replace with dynamic (Geerd) patch, put in paravirt_ops structure. > > > I'm looking at this, and I'm not sure that there needs to be a void > (*set_fixmap_top)(unsigned long top) entry in paravirt ops. It seems to > me that the hypervisor's init code can call the normal set_fixmap_top() > function (which Geerd's patch introduces) with whatever appropriate > value it needs when it also installs the rest of the paravirt ops > pointers and/or patches. After all, this is something the hypervisor > needs to set in the kernel, rather than something the kernel needs to do > to the hypervisor. > > Does that sound reasonable, or does there really need to be a paravirt > entry for this? > > Also, since this is a pretty generic patch with no earlier dependencies, > we can push it near the front of the patch queue.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). Just back on the ground, Rusty. -- Help! Save Australia from the worst of the DMCA: http://linux.org.au/law