Re: xen spl rework

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

On 6 December 2012 17:09, Manuel Bouyer <bouyer%antioche.eu.org@localhost> 
wrote:
> 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 ?
>

Yes you're right, I'll have another go at this.

Thanks!

-- 
~Cherry