Re: xen spl rework

["Cherry G. Mathew" <cherry.g.mathew%gmail.com@localhost>]

On 20 December 2011 10:15, Jean-Yves Migeon <jeanyves.migeon%free.fr@localhost> 
wrote:
> 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:
>


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


> /* 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).
>

On a second glance, I realised that Xspllower is called only via
xen_inter.c:spllower()
This means that the recurse/resume assembler path is redundant except
for saving frame state on the stack at the point at which spllower()
was called, to fake taking an interrupt to the pending interrupt
handlers being queued to run during the lower. I've been experimenting
with using lwp->l_md->md_tf - don't have anything good yet.

> 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 just wanted to have a better semantic match between the code and the
entry paths. Currently, the xen entry paths are only via the
registered callback (machine/vector.S) and that's where the
recurse/resume stack foo should be done, imho. Also, wanted to reduce
assembler to the absolute minimum possilble.

>
>> 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.
>

The iret path isn't modified, so this is not as scary as it seems (see
my comments about spllower() above). There are no codepath returns on
user/kernel boundaries in any of the code modified above.

Cheers,
-- 
~Cherry