Re: [Xen-devel] kexec trouble

["Magnus Damm" <magnus.damm@xxxxxxxxx>]

On 12/6/06, Gerd Hoffmann <kraxel@xxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel