Re: xen spl rework

[Jean-Yves Migeon <jeanyves.migeon%free.fr@localhost>]

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()
>
> See:
> ftp://ftp.netbsd.org/pub/NetBSD/misc/cherry/tmp/port-xen/spl.diff

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.

[1] http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2009-2793

--
Jean-Yves Migeon
jeanyves.migeon%free.fr@localhost