Re: xen spl rework

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Thu, Dec 06, 2012 at 03:35:02PM -0600, Cherry G. Mathew wrote:
> Hi Manuel,
> 
> Thanks for taking the time.
> 
> On 6 December 2012 04:00, Manuel Bouyer <bouyer%antioche.eu.org@localhost> 
> wrote:
> > On Sun, Nov 25, 2012 at 02:29:50PM -0600, Cherry G. Mathew wrote:
> >> I've updated the patch with your suggestions.
> >> ftp://ftp.netbsd.org/pub/NetBSD/misc/cherry/tmp/port-xen/spl1.c.diff
> >
> > Cosmetic first:
> > move extern variable and function declaration to hearders, included by
> > all .c that need it. It's way too easy for things to get out of
> > sync otherwise.
> >
> 
> Ok, thanks.
> 
> >
> > in evtchn_do_event() seems to not be used anymore (it's computed but
> > never read).
> 
> Not sure I understand, it's called from hypervisor_machdep.c

It should have been:
in evtchn_do_event(), iplmask seems to not be used anymore (it's computed but
never read).

> 
> > And, more important, what is setting bits in ci->ci_ipending now ?
> > It seems that hypervisor_set_ipending() is never called any more ?
> 
> I think you're referring to this:
> 
> Index: arch/xen/xen/evtchn.c
> ===================================================================
> RCS file: /cvsroot/src/sys/arch/xen/xen/evtchn.c,v
> retrieving revision 1.64
> diff -u -r1.64 evtchn.c
> --- arch/xen/xen/evtchn.c     25 Nov 2012 08:39:36 -0000      1.64
> +++ arch/xen/xen/evtchn.c     25 Nov 2012 20:19:15 -0000
> [...]
> 
> ***************
> *** 307,366 ****
>                 if (evtch == IRQ_DEBUG)
>                     printf("ih->ih_level %d <= ilevel %d\n",
> ih->ih_level, ilevel);
>   #endif
> -                       cli();
> -                       hypervisor_set_ipending(iplmask,
> -                           evtch >> LONG_SHIFT, evtch & LONG_MASK);
> 
> 
> It removes a redundant (second) hypervisor_set_ipending() call - jym
> pointed this up a bit earlier in this thread - probably should have
> been broken out into a separate patch. The new codepath makes sure
> that pending bits are only set on local cpus and uses ipis to set
> remote pending bits.

Right, there's another case where hypervisor_set_ipending() is called that I
missed. But it's for a different case. Here you're skipping calling
handlers because the IPL isn't low enough, but as the pending bit will not
be set (even on local CPU) nothing will call this handler when the spl
is lowered. What did I miss ?

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--