Re: xen spl rework

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

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 are disabled (hence, after cli) rather than before? It can 
silently mask the interrupt if one is raised before returning from 
hypervisor_set_ipending, no?

(I guess that the mutex for the affected evtchn will protect the call 
anyway, but I prefer asking first).

- line 218, the KASSERT() is not needed given the instruction just above.

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

IIRC this trick comes from Manuel.

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

Okay.

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

Agreed.

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