tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: xen spl rework

On Tue, 20 Dec 2011 00:57:13 +0530, Cherry G. Mathew wrote:
I've reworked the spl code in XEN to use a single entry point into C
code, and to optimise the pending event processing via spllower()


Some comments:
- remove hypervisor_enable_ipl from headers, it does not seem to be used anymore. - if you are in a cleanup process, have a consistent clear/set interrupt flag function for x86 and xen. Use x86_enable/disable_intr() stubs, and do not spread cli()/sti() directly. It's easier to grep/grok for x86_ variants.
- regarding XXX comments:

/* XXX: Don't need to save everything. Just the callee saved
 * regs and stuff that hardclock() needs

You are correct, however keep in mind that you may call C functions that do not make any assumption regarding registers' usage (like xspllower), so it's still better to be conservative there and save all general purpose regs.
- please document xspllower() vs Xspllower() use (comments are enough).

One question: what's the reason behind removing the IPL recurse/remove code? Interesting stuff nonetheless, I think that this will fix a very rare issue I had with an old machine where I could trigger double faults on Xen thanks to this.

I'd love for as much test reportage as possible with this patch.

dom0 is reported to hang on launching domUs. I'd like this verified on
PAE kernels if possible.

If I can spare some cycles on my host, I'll do it this week. Else that will just be a VM test until January, I am afraid.

Please include relevant dmesg/console output.

tech-kern@ is Cc:ed for more eyes to see if I've missed anything obvious.
port-i386/amd64@ is Cc:ed because I've removed some of the common
assembler bits from the XEN path and added some new. Review would be
useful. I'm hoping this doesn't affect anything for non-XEN.

Untangling the #ifdef XEN hell is a good step, but please be aware that you are making modifications in highly sensitive code path (performance and security) [1]. So a peer review won't just be enough, you will also have to test it extensively on your side too, and maybe ask core/port maintainer approval.


Jean-Yves Migeon

Home | Main Index | Thread Index | Old Index