Port-xen archive

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

Re: xen spl rework



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


Home | Main Index | Thread Index | Old Index