On 11 November 2012 18:21, Jean-Yves Migeon <jeanyves.mig...@free.fr> wrote: > Le 10/11/12 19:15, Cherry G. Mathew a écrit : >>> >>> 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: >> >> >> I've reworked this in stages, here's the first bit (C only) that >> addresses some of your suggestions: >> ftp://ftp.netbsd.org/pub/NetBSD/misc/cherry/tmp/port-xen/spl.c.diff > > > I took a look at it this afternoon and code looks fine to me. But this goes > a bit further than s/sti/x86_enable_intr. > > FWIW: > - typo line 14, you have an extra space to the function prototype > > - line 206, shouldn't it be better to set the pending IPL when interrupts
... > - line 218, the KASSERT() is not needed given the instruction just above. > > Slightly confused by your line numbering - is that with reference to a certain file or the file offsets in the patch, or the line number in the patch itself ? -- ~Cherry
