Hi, Uh, it''s a bit messy, with the changes sprinkled over the sparse tree and the patches directory, which makes it a bit hard to fixup stuff. IMHO the kexec code makes way to many decisions at compile time, not runtime, especially the ones in the kexec code core. Having something depend on CONFIG_XEN doesn''t fly with the paravirt approach planned for mainline merge (same kernel binary runs both native and paravirtualized). I''m also in trouble now with guest kexec patches as they work with guest phys addrs not machine phys addrs. I think we need either wrapper functions for machine_kexec_* functions which dispatch to the correct function depending on the environment (dom0 vs domU, later also native) or just make them function pointers to archive the same effect. Same goes for the KEXEC_ARCH_HAS_PAGE_MACROS stuff. IMHO "#ifdef CONFIG_XEN" should go away from the core code (i.e. kernel/kexec.c). cheers, Gerd -- Gerd Hoffmann <kraxel@suse.de> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi Gerd, On 12/5/06, Gerd Hoffmann <kraxel@suse.de> wrote:> Hi, > > Uh, it''s a bit messy, with the changes sprinkled over the sparse tree > and the patches directory, which makes it a bit hard to fixup stuff.Well, I''m sorry to hear that you think it is messy. I don''t think that we touch that many places in the sparse tree, but I agree that the combination of patches and sparse may be a bit confusing. The alternative to patches would have been to duplicate the files by copying the into the sparse tree which I wanted to avoid because I think it makes future up porting difficult.> IMHO the kexec code makes way to many decisions at compile time, not > runtime, especially the ones in the kexec code core. Having something > depend on CONFIG_XEN doesn''t fly with the paravirt approach planned for > mainline merge (same kernel binary runs both native and paravirtualized).Sure, but isn''t the paravirt stuff just for domU first to begin with? I''m pretty sure that making the code dynamically decide between dom0, domU or native is quite simple to implement when it comes to kexec, but I wanted to wait with that until most parts of dom0 was running under paravirt.> I''m also in trouble now with guest kexec patches as they work with guest > phys addrs not machine phys addrs.Sorry if that made your life difficult, but shouldn''t it just be a matter of using the native versions of the page macros for domU? They are in include/linux/kexec.h if I''m not mistaken. In a patch, not in sparse.> I think we need either wrapper functions for machine_kexec_* functions > which dispatch to the correct function depending on the environment > (dom0 vs domU, later also native) or just make them function pointers to > archive the same effect. Same goes for the KEXEC_ARCH_HAS_PAGE_MACROS > stuff. IMHO "#ifdef CONFIG_XEN" should go away from the core code (i.e. > kernel/kexec.c).You mean for the paravirt stuff? Isn''t paravirt basically a set of callbacks that you can register? If so, what is stopping us from registering a set of paravirt callbacks for the kexec code? / magnus _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi,>> IMHO the kexec code makes way to many decisions at compile time, not >> runtime, especially the ones in the kexec code core. Having something >> depend on CONFIG_XEN doesn''t fly with the paravirt approach planned for >> mainline merge (same kernel binary runs both native and paravirtualized). > > Sure, but isn''t the paravirt stuff just for domU first to begin with?domU only as first step, later dom0 too.> I''m pretty sure that making the code dynamically decide between dom0, > domU or native is quite simple to implement when it comes to kexec, > but I wanted to wait with that until most parts of dom0 was running > under paravirt.I''d prefer to do that _now_.>> I''m also in trouble now with guest kexec patches as they work with guest >> phys addrs not machine phys addrs. > > Sorry if that made your life difficult, but shouldn''t it just be a > matter of using the native versions of the page macros for domU?No. The same xen kernel can run as both dom0 and domU, thus that must be decided at runtime.>> I think we need either wrapper functions for machine_kexec_* functions >> which dispatch to the correct function depending on the environment >> (dom0 vs domU, later also native) or just make them function pointers to >> archive the same effect. Same goes for the KEXEC_ARCH_HAS_PAGE_MACROS >> stuff. IMHO "#ifdef CONFIG_XEN" should go away from the core code (i.e. >> kernel/kexec.c). > > You mean for the paravirt stuff?And domU kexec. That works without any kexec core changes, and I suspect the #ifdef CONFIG_XEN code will break it.> Isn''t paravirt basically a set of > callbacks that you can register?Yes.> If so, what is stopping us from > registering a set of paravirt callbacks for the kexec code?Hmm, we''ll end up with *two* sets of callbacks for xen, one for dom0 and one for domU kexec. Not sure that fits the current paravirt design. Given we may move to paravirt some day it''s probably best to go with the function pointers approach for now, that makes switching over to the paravirt infrastructure (once it is mainline) easier. And I think its also less messy in the code. cheers, Gerd -- Gerd Hoffmann <kraxel@suse.de> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi again Gerd, [CC Simon] On 12/6/06, Gerd Hoffmann <kraxel@suse.de> wrote: > >> I''m also in trouble now with guest kexec patches as they work with guest> >> phys addrs not machine phys addrs. > > > > Sorry if that made your life difficult, but shouldn''t it just be a > > matter of using the native versions of the page macros for domU? > > No. The same xen kernel can run as both dom0 and domU, thus that must > be decided at runtime.Well, for us there was no need to decide that at runtime. Our scope was only dom0. For you a runtime check makes sense, especially now when our code is merged and you have a conflict. It does however sound like you are pissed because the conflict, but I don''t think you should blame that on us. Simon and I reposted the patches at least 10 times over the last half a year - so you had your time to come with feedback. That aside, what about doing as little as possible now? Use is_initial_xendomain() or something like that to switch between the different dom0 and domU implementations. And whenever domU and dom0 runs under paravirt we fix up to code to remove the #ifdef and add native mode support.> >> I think we need either wrapper functions for machine_kexec_* functions > >> which dispatch to the correct function depending on the environment > >> (dom0 vs domU, later also native) or just make them function pointers to > >> archive the same effect. Same goes for the KEXEC_ARCH_HAS_PAGE_MACROS > >> stuff. IMHO "#ifdef CONFIG_XEN" should go away from the core code (i.e. > >> kernel/kexec.c). > > > > You mean for the paravirt stuff? > > And domU kexec. That works without any kexec core changes, and I > suspect the #ifdef CONFIG_XEN code will break it.Replacing the #ifdefs with a runtime check that is fine by me. I''m think it''s nice to avoid #ifdefs if possible, but again - our scope of implementation was simply to add dom0 support. We did not care about domU support or paravirt that wasn''t included at that time.> > If so, what is stopping us from > > registering a set of paravirt callbacks for the kexec code? > > Hmm, we''ll end up with *two* sets of callbacks for xen, one for dom0 and > one for domU kexec. Not sure that fits the current paravirt design.I''m pretty sure that these things will be easy to resolve when the time is right.> Given we may move to paravirt some day it''s probably best to go with the > function pointers approach for now, that makes switching over to the > paravirt infrastructure (once it is mainline) easier. And I think its > also less messy in the code.There is only a point in having function pointers when you have more than one implementation. And now you are going from one implementation to two so adding function pointers makes sense. If we would have added function pointers in our patch it would have been pure bloat because there was no one there except us to use them. / magnus _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 5/12/06 3:53 pm, "Magnus Damm" <magnus.damm@gmail.com> wrote:>> I think we need either wrapper functions for machine_kexec_* functions >> which dispatch to the correct function depending on the environment >> (dom0 vs domU, later also native) or just make them function pointers to >> archive the same effect. Same goes for the KEXEC_ARCH_HAS_PAGE_MACROS >> stuff. IMHO "#ifdef CONFIG_XEN" should go away from the core code (i.e. >> kernel/kexec.c). > > You mean for the paravirt stuff? Isn''t paravirt basically a set of > callbacks that you can register? If so, what is stopping us from > registering a set of paravirt callbacks for the kexec code?I think partly Gerd''s point is that CONFIG_XEN in kernel/kexec.c will never get merged upstream. Guaranteed. The kexec/kdump patches are not very tidy in some respects like this. We applied them now because the functionality is useful, but I don''t think we yet have the finished polished article. Also you got away with it because the code changes were hidden in the patches/ directory, which you originally said was simply backported code from 2.6.19 (not backported-and-hacked-on!). -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Magnus Damm wrote:> For you a runtime check makes sense, especially now when our code is > merged and you have a conflict. It does however sound like you are > pissed because the conflict, but I don''t think you should blame that > on us.Yes, a bit, especially as we''ve talked a bit about dom0/domU kexec at the Xen Summit, so I assumed you are aware of the problem. The sparse/patches split of the code also makes it hard to change it.> Simon and I reposted the patches at least 10 times over the > last half a year - so you had your time to come with feedback.Yes, I should have checked before. -ENOTIME. Bad decision nevertheless, now it probably costs even more time to fix it up afterwards ....> That aside, what about doing as little as possible now? Use > is_initial_xendomain() or something like that to switch between the > different dom0 and domU implementations. And whenever domU and dom0 > runs under paravirt we fix up to code to remove the #ifdef and add > native mode support.I''d go for the function pointer approach. I think it is easier to maintain in the long run. Wrapper functions which look at is_initial_xendomain() then call either xen0_machine_kexec or xenU_machine_kexec quickly get messy with lots of #ifdef CONFIG_FOOBAR, and it would be a temporary solution only anyway. I think you compile in native code too, although it is dead code, right? So we can make machine_kexec() + friends function pointers, rename the native functions and initialize the function pointers to the native versions. I think it should even be possible to make them function pointers for i386/x86_64 archs only. Things keep working with CONFIG_XEN=n then, and with CONFIG_XEN=y the initialization function just switches the function pointers (depending on is_initial_domain()). This also eliminates the first set of #ifdefs in kernel/kexec.c ;)> Replacing the #ifdefs with a runtime check that is fine by me. I''m > think it''s nice to avoid #ifdefs if possible, but again - our scope of > implementation was simply to add dom0 support. We did not care about > domU support or paravirt that wasn''t included at that time.Having "#ifdef CONFIG_XEN" in kernel/kexec.c most likely never ever is accepted mainline (and we do seek mainline merge, don''t we?). IMHO that is enough reason to avoid it in the first place. cheers, Gerd -- Gerd Hoffmann <kraxel@suse.de> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 12/6/06, Keir Fraser <keir@xensource.com> wrote:> On 5/12/06 3:53 pm, "Magnus Damm" <magnus.damm@gmail.com> wrote: > > >> I think we need either wrapper functions for machine_kexec_* functions > >> which dispatch to the correct function depending on the environment > >> (dom0 vs domU, later also native) or just make them function pointers to > >> archive the same effect. Same goes for the KEXEC_ARCH_HAS_PAGE_MACROS > >> stuff. IMHO "#ifdef CONFIG_XEN" should go away from the core code (i.e. > >> kernel/kexec.c). > > > > You mean for the paravirt stuff? Isn''t paravirt basically a set of > > callbacks that you can register? If so, what is stopping us from > > registering a set of paravirt callbacks for the kexec code? > > I think partly Gerd''s point is that CONFIG_XEN in kernel/kexec.c will never > get merged upstream. Guaranteed.Sure, I understand that. But I see this as an iterative process, where the our code so far has been written to fit the current codebase. When dom0 runs on paravirt and we can test the code then it should be adjusted. It''s kind of hard to write for something that doesn''t yet exist. =) So regardless how you do it, you still need to adjust your code towards the new interface in the end - it''s just a matter of how much code you need to adjust. I''m all for converting the code into using runtime checks or callbacks if that is needed, and I would have done so in the first place if I''d known that it was something that you guys wanted. But I didn''t so we used the simplest possible solution instead which was CONFIG_XEN.> The kexec/kdump patches are not very tidy in some respects like this. We > applied them now because the functionality is useful, but I don''t think we > yet have the finished polished article. Also you got away with it because > the code changes were hidden in the patches/ directory, which you originally > said was simply backported code from 2.6.19 (not backported-and-hacked-on!).The git-patches are backports. The other ones are not: http://lists.xensource.com/archives/html/xen-devel/2006-10/msg01240.html / magnus _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 12/6/06, Gerd Hoffmann <kraxel@suse.de> wrote:> Magnus Damm wrote: > > For you a runtime check makes sense, especially now when our code is > > merged and you have a conflict. It does however sound like you are > > pissed because the conflict, but I don''t think you should blame that > > on us. > > Yes, a bit, especially as we''ve talked a bit about dom0/domU kexec at > the Xen Summit, so I assumed you are aware of the problem. The > sparse/patches split of the code also makes it hard to change it.We chit-chatted a bit, but I don''t remember us talking about any implementation details. I''ve heard complaints and doubts about using sparse together with patches, but when I ask for a better alternative it''s always awfully silent. We could have copied the files into sparse and applied our patches, but duplicating files seemed a step in the wrong direction. It''s funny because the reason behind using patches is to simplify up porting, but now instead of simplifying it seems to confuse people. Maybe we should have copied the files to sparse instead, would that have been better?> > Simon and I reposted the patches at least 10 times over the > > last half a year - so you had your time to come with feedback. > > Yes, I should have checked before. -ENOTIME. Bad decision > nevertheless, now it probably costs even more time to fix it up > afterwards ....I don''t mind changing pieces of the code now. It would probably have been easier to do the right thing earlier, but the number of changes needed are probably pretty low. If there is anything I can help out with just let me know!> > That aside, what about doing as little as possible now? Use > > is_initial_xendomain() or something like that to switch between the > > different dom0 and domU implementations. And whenever domU and dom0 > > runs under paravirt we fix up to code to remove the #ifdef and add > > native mode support. > > I''d go for the function pointer approach. I think it is easier to > maintain in the long run. Wrapper functions which look at > is_initial_xendomain() then call either xen0_machine_kexec or > xenU_machine_kexec quickly get messy with lots of #ifdef CONFIG_FOOBAR, > and it would be a temporary solution only anyway.Yes, the function pointer solution is a lot nicer.> I think you compile in native code too, although it is dead code, right?The only dead code function that I know of would be machine_kexec(), and that one will be needed if we want to support native mode.> So we can make machine_kexec() + friends function pointers, rename the > native functions and initialize the function pointers to the native > versions. I think it should even be possible to make them function > pointers for i386/x86_64 archs only. Things keep working with > CONFIG_XEN=n then, and with CONFIG_XEN=y the initialization function > just switches the function pointers (depending on is_initial_domain()). > This also eliminates the first set of #ifdefs in kernel/kexec.c ;)Sounds exactly what I would have done! =)> > Replacing the #ifdefs with a runtime check that is fine by me. I''m > > think it''s nice to avoid #ifdefs if possible, but again - our scope of > > implementation was simply to add dom0 support. We did not care about > > domU support or paravirt that wasn''t included at that time. > > Having "#ifdef CONFIG_XEN" in kernel/kexec.c most likely never ever is > accepted mainline (and we do seek mainline merge, don''t we?). IMHO that > is enough reason to avoid it in the first place.Yes and no. =) You seem to code with the goal of having something that will be directly acceptable for mainilne, but my goal is to write as simple code as possible which should be easy to adjust to whatever framework that exists at the time of mainline merge. Let me know what I can do to help out. Thanks, / magnus _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi,> We chit-chatted a bit, but I don''t remember us talking about any > implementation details.Discussed briefly possible code sharing, that there likely isn''t much to share because we have two very different approachs to take, and that we are probably best off just having two machine_kexec() versions for dom0/domU. No details yet how to actually implement that, but at least the need for some kind of runtime switching should have been clear.> I''ve heard complaints and doubts about using sparse together with > patches, but when I ask for a better alternative it''s always awfully > silent. We could have copied the files into sparse and applied our > patches, but duplicating files seemed a step in the wrong direction.For backports and code planned for quick mainline merge maintaining as patches is fine, makes it easier to move forward once stuff is merged and/or the xen linux tree is updated to a newer upstream kernel. For code which likely lives longer in the xen tree (especially kexec-generic.patch which has almost no chance to be accepted mainline as-is) it is a pain to deal with as patch. I''d love to see kernel/kexec.c not being touched at all, but I think that is impossible for dom0 kexec (due to range checks which must happen in machine not guest address space for example).>> So we can make machine_kexec() + friends function pointers, rename the >> native functions and initialize the function pointers to the native >> versions. I think it should even be possible to make them function >> pointers for i386/x86_64 archs only. Things keep working with >> CONFIG_XEN=n then, and with CONFIG_XEN=y the initialization function >> just switches the function pointers (depending on is_initial_domain()). >> This also eliminates the first set of #ifdefs in kernel/kexec.c ;) > > Sounds exactly what I would have done! =)Great, so lets do that.> You seem to code with the goal of having something that will be > directly acceptable for mainilne, but my goal is to write as simple > code as possible which should be easy to adjust to whatever framework > that exists at the time of mainline merge.Given that the framework will be paravirt_ops function pointers fit nicely ;) cheers, Gerd -- Gerd Hoffmann <kraxel@suse.de> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 12/6/06, Gerd Hoffmann <kraxel@suse.de> wrote:> Hi, > > > We chit-chatted a bit, but I don''t remember us talking about any > > implementation details. > > Discussed briefly possible code sharing, that there likely isn''t much to > share because we have two very different approachs to take, and that we > are probably best off just having two machine_kexec() versions for > dom0/domU. No details yet how to actually implement that, but at least > the need for some kind of runtime switching should have been clear.We needed to work together to implement runtime switching anyhow, and that''s what is happening now. But maybe I should have considered the runtime switching earlier...> > I''ve heard complaints and doubts about using sparse together with > > patches, but when I ask for a better alternative it''s always awfully > > silent. We could have copied the files into sparse and applied our > > patches, but duplicating files seemed a step in the wrong direction. > > For backports and code planned for quick mainline merge maintaining as > patches is fine, makes it easier to move forward once stuff is merged > and/or the xen linux tree is updated to a newer upstream kernel.Ack.> For code which likely lives longer in the xen tree (especially > kexec-generic.patch which has almost no chance to be accepted mainline > as-is) it is a pain to deal with as patch.Yeah, I can agree with that. Feel free to add the files to sparse and throw out the patch. The dependency on patches and other stuff may make it difficult though.> I''d love to see kernel/kexec.c not being touched at all, but I think > that is impossible for dom0 kexec (due to range checks which must happen > in machine not guest address space for example).We hoped to not touch the generic code at all too, but we had to because of machine addresses> >> So we can make machine_kexec() + friends function pointers, rename the > >> native functions and initialize the function pointers to the native > >> versions. I think it should even be possible to make them function > >> pointers for i386/x86_64 archs only. Things keep working with > >> CONFIG_XEN=n then, and with CONFIG_XEN=y the initialization function > >> just switches the function pointers (depending on is_initial_domain()). > >> This also eliminates the first set of #ifdefs in kernel/kexec.c ;) > > > > Sounds exactly what I would have done! =) > > Great, so lets do that.Excellent! Let me know how and where you want my help.> > You seem to code with the goal of having something that will be > > directly acceptable for mainilne, but my goal is to write as simple > > code as possible which should be easy to adjust to whatever framework > > that exists at the time of mainline merge. > > Given that the framework will be paravirt_ops function pointers fit > nicely ;)Function pointers sound like the right way to go! Happy hacking! Thanks, / magnus _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi,>> For code which likely lives longer in the xen tree (especially >> kexec-generic.patch which has almost no chance to be accepted mainline >> as-is) it is a pain to deal with as patch. > > Yeah, I can agree with that. Feel free to add the files to sparse and > throw out the patch. The dependency on patches and other stuff may > make it difficult though.*Aaaaargh*, it''s even messier than I thought. We have linux kernel source files which are modified by patches *AND* are in the sparse tree. And the two versions don''t match of course. Looks like that is an older issue though, so I can''t blame kexec for that one ;) These patches can''t be removed cleanly after running mkbuildtree: x86-put-note-sections-into-a-pt_note-segment-in-vmlinux.patch smp-alts.patch net-gso-2-checksum-fix.patch net-gso-0-base.patch We *must* find a more sane way to maintain the linux kernel sources, this is one more reason why mixing sparse tree and patches isn''t going to fly. As far I know at least the sparse tree is planned to be dropped, now with dom0 and xen being decoupled (3.0.3+) it should be possible without too much hassle. Any plans what to use instead? quilt patch queue? cheers, Gerd -- Gerd Hoffmann <kraxel@suse.de> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Wed, Dec 06, 2006 at 02:23:36PM +0100, Gerd Hoffmann wrote:> We *must* find a more sane way to maintain the linux kernel sources, > this is one more reason why mixing sparse tree and patches isn''t going > to fly. As far I know at least the sparse tree is planned to be > dropped, now with dom0 and xen being decoupled (3.0.3+) it should be > possible without too much hassle. Any plans what to use instead? quilt > patch queue?A full hg or git tree would be nicer... the way things are going, patches applied to such a tree wouldn''t be appropriate for upstream inclusion without cleaning up anyway, so I don''t see what the patch queue method will buy us as opposed to a full tree. Of course nearly anything would be better than the sparse + patches method. Cheers, Muli _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi,> Function pointers sound like the right way to go! Happy hacking!First step of a cleanup by moving to function pointers. Compile tested only. First three attachments replace the patches with identical names in patches/linux-2.6. The last should be applied to the sparse tree. cheers, Gerd -- Gerd Hoffmann <kraxel@suse.de> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi again Gerd, On 12/7/06, Gerd Hoffmann <kraxel@suse.de> wrote:> Hi, > > > Function pointers sound like the right way to go! Happy hacking! > > First step of a cleanup by moving to function pointers.As a first step I think they look pretty good. I have a few random comments.> Compile tested only.Ok. I''ve browsed through the patches and done some basic compilation too.> First three attachments replace the patches with identical names in > patches/linux-2.6. The last should be applied to the sparse tree.I think using a structure for all callbacks will result in cleaner code. This is sort of a nitpick because it does not really matter function wise, but it sounded earlier like you were aiming for something that would be directly acceptable by the kexec and kdump community. And I''m all for cleanliness. Personally I would go with changing the code in kernel/kexec.c to instead of calling machine_kexec() call kexec_ops.machine_kexec(). This regardless of the use of KEXEC_ARCH_USES_HOOKS. Then I would have a single global instance of the structure kexec_ops declared in kernel/kexec.c, and it would by default fill in kexec_ops.machine_kexec() to machine_kexec. That way you won''t have to rename the arch-specific functions and there is no need to declare the hooks in the arch-specific files. Maybe you won''t need KEXEC_ARCH_USES_HOOKS at all. The load and unload code may be broken today if KEXEC_ARCH_USES_HOOKS is unset - can you really check if machine_kexec_load is non-NULL if it is inline? The reason why I did put the page-macros in arch-specific header files was because they need to be different on ia64. So your unification in drivers/xen/core/machine_kexec.c may be ok for now (if our goal is x86 only), but in the future we need to figure out how to change them nicely on ia64. You probably remember that I was kind of negative to trying to solve mainline merge issues at the same time as implementing this "switch". This was because I remembered that paravirt allowed patching of inline machine code. At least that''s the impression I got from a presentation given here in Tokyo by Rusty. I think the page macros ideally should be patched in, but it''s kind of hard trying to do that without paravirt.. Finally, we should get rid of the #ifdef CONFIG_XEN left here and there. My main concern is the code in crash.c which need to be replaced with runtime checks if we are aiming for a single binary for both native and dom0. I left out domU because it doesn''t do crash, right? If you have an updated snapshot (or a replay saying I should use this version) then I''ll try out the code the first thing next week. Thanks, / magnus _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Magnus Damm wrote:> I think using a structure for all callbacks will result in cleaner > code. This is sort of a nitpick because it does not really matter > function wise, but it sounded earlier like you were aiming for > something that would be directly acceptable by the kexec and kdump > community. And I''m all for cleanliness. > > Personally I would go with changing the code in kernel/kexec.c to > instead of calling machine_kexec() call kexec_ops.machine_kexec(). > This regardless of the use of KEXEC_ARCH_USES_HOOKS. Then I would have > a single global instance of the structure kexec_ops declared in > kernel/kexec.c, and it would by default fill in > kexec_ops.machine_kexec() to machine_kexec. That way you won''t have to > rename the arch-specific functions and there is no need to declare the > hooks in the arch-specific files. Maybe you won''t need > KEXEC_ARCH_USES_HOOKS at all.Yep, good idea, that works without the hooks define (and also without touching all architectures which I want to avoid too).> The load and unload code may be broken today if KEXEC_ARCH_USES_HOOKS > is unset - can you really check if machine_kexec_load is non-NULL if > it is inline?Didn''t check what gcc made out of it. It''s a moot point now anyway with the switch to a ops struct.> The reason why I did put the page-macros in arch-specific header files > was because they need to be different on ia64. So your unification in > drivers/xen/core/machine_kexec.c may be ok for now (if our goal is x86 > only), but in the future we need to figure out how to change them > nicely on ia64.I simply wasn''t aware of the ia64 issue. Well, maybe we should simply create a arch/$arch/kernel/machine_kexec_xen0.c file with that kind of code placed into. And maybe move the arch-independant xen bits in driver/xen/core/machine_kexec.c to bits to kernel/kexec_xen0, but I think that discussion better should be defered until we are actually seeking mainline merge. Maybe we get our own subdirectory below kernel/ for that kind of stuff. There is some more code which is arch-specific on native but simply a hypercall on xen, smpboot.c for example.> You probably remember that I was kind of negative to trying to solve > mainline merge issues at the same time as implementing this "switch". > This was because I remembered that paravirt allowed patching of inline > machine code. At least that''s the impression I got from a presentation > given here in Tokyo by Rusty. I think the page macros ideally should > be patched in, but it''s kind of hard trying to do that without > paravirt..Yep, there is, to get some percent performace improvements for hot path code. Which IMHO isn''t true for the kexec bits. It isn''t performance critical, usually you load a kexec kernel only once, I don''t think it is worth the trouble.> Finally, we should get rid of the #ifdef CONFIG_XEN left here and > there. My main concern is the code in crash.c which need to be > replaced with runtime checks if we are aiming for a single binary for > both native and dom0. I left out domU because it doesn''t do crash, > right?I expect *lots* of changes in that area (apic/smp) anyway when we upgrade the xen linux tree to be based on 2.6.20-rc1 or newer. paravirt infrastructure is in Linus'' tree now. I''d wait until that is done then look again.> If you have an updated snapshot (or a replay saying I should use this > version) then I''ll try out the code the first thing next week.Updated patches attached. cheers, Gerd -- Gerd Hoffmann <kraxel@suse.de> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi Gerd, On Fri, 2006-12-08 at 11:01 +0100, Gerd Hoffmann wrote:> Updated patches attached.Unfortunately I''m just about to push a changeset which move the contents of these patches: patches/linux-2.6.16.33/kexec-generic.patch patches/linux-2.6.16.33/linux-2.6.19-rc1-kexec-xen-i386.patch patches/linux-2.6.16.33/linux-2.6.19-rc1-kexec-xen-x86_64.patch into the sparse tree where they belong. Sorry for moving the ground under you. Also due to the freeze we won''t be able to take these changes until after 3.0.4 is released. Cheers, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell wrote:> Hi Gerd, > > On Fri, 2006-12-08 at 11:01 +0100, Gerd Hoffmann wrote: >> Updated patches attached. > > Unfortunately I''m just about to push a changeset which move the contents > of these patches: > patches/linux-2.6.16.33/kexec-generic.patch > patches/linux-2.6.16.33/linux-2.6.19-rc1-kexec-xen-i386.patch > patches/linux-2.6.16.33/linux-2.6.19-rc1-kexec-xen-x86_64.patch > into the sparse tree where they belong. Sorry for moving the ground > under you.Oh, that is fine. Makes it easier for me, also the I can fold my changes into a single patch for the sparse tree then which likely is smaller and easier to review ;) Your changes are not in the public tree yet though .... cheers, Gerd -- Gerd Hoffmann <kraxel@suse.de> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 8/12/06 11:28, "Gerd Hoffmann" <kraxel@suse.de> wrote:> Oh, that is fine. Makes it easier for me, also the I can fold my > changes into a single patch for the sparse tree then which likely is > smaller and easier to review ;) > > Your changes are not in the public tree yet though ....The staging tree is stalled for some reason. I''m not sure whether there''s a systematic problem or just a few random problems in a row... We''ll look into it this afternoon so we can get stuff pushed to the public tree later today. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Fri, 2006-12-08 at 12:28 +0100, Gerd Hoffmann wrote:> Ian Campbell wrote: > > Hi Gerd, > > > > On Fri, 2006-12-08 at 11:01 +0100, Gerd Hoffmann wrote: > >> Updated patches attached. > > > > Unfortunately I''m just about to push a changeset which move the contents > > of these patches: > > patches/linux-2.6.16.33/kexec-generic.patch > > patches/linux-2.6.16.33/linux-2.6.19-rc1-kexec-xen-i386.patch > > patches/linux-2.6.16.33/linux-2.6.19-rc1-kexec-xen-x86_64.patch > > into the sparse tree where they belong. Sorry for moving the ground > > under you. > > Oh, that is fine. Makes it easier for me, also the I can fold my > changes into a single patch for the sparse tree then which likely is > smaller and easier to review ;) > > Your changes are not in the public tree yet though ....I was delayed a bit in pushing them, they are in now. Hopefully that will be unwedged and flow through this afternoon. Cheers, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Fri, 2006-12-08 at 12:28 +0100, Gerd Hoffmann wrote:> Your changes are not in the public tree yet though ....Should be there now. Cheers, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel