Re: xen spl rework

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

On 25 November 2012 14:29, Cherry G. Mathew 
<cherry.g.mathew%gmail.com@localhost> wrote:
> Taking off some of the lists on Cc: to reduce noise.....
>
> Hi Jean-Yves,
>
> Thanks for taking the time to review this.
>
> On 11 November 2012 18:21, Jean-Yves Migeon 
> <jeanyves.migeon%free.fr@localhost> wrote:
>> 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).
>>
>
> Thanks for catching this - I've committed a fix for this:
> http://mail-index.netbsd.org/source-changes/2012/11/25/msg039125.html
>
>> - line 218, the KASSERT() is not needed given the instruction just above.
>>
>>
>
> I've updated the patch with your suggestions.
> ftp://ftp.netbsd.org/pub/NetBSD/misc/cherry/tmp/port-xen/spl1.c.diff
>

Let me know if this is ok to commit. It's survived multiple build worlds

Cheers,
-- 
~Cherry