ndis: patch for review

[Paul B Mahol]

On 10/20/10, Bernhard Schmidt <bschmidt at freebsd.org> wrote:
> 9, 2010 at 23:53, Paul B Mahol <onemda at gmail.com> wrote:
>> Hi,
>>
>> First: we should pin curthread on CPU before we check on which CPU is
>> curthread.
>>
>> Second: instead of sti & cli use critical sections when saving %fs
>> register.
>>
>> Third: I do not know what happens if we get preempted while windows
>> code were running,
>> so leave critical section only _after_ executing windows code.
>
>
> Do you have a test case for that? If so, can you post it?

Unfortunately I do not have test case for third problem.

But not using sched_pin or/and critical sections correctly cause
NTOS: timer %p fired even though it was canceled
in my environment - causing watchdog on ndisX.

And attempt to unload miniport driver after that will cause panic
because something got corrupted.

Theoretically when executing windows code there is small chance for
our thread to be preempted and than it will actually cause %fs corruption
on CPU we were running, and reading comments it appears FreeBSD use %fs
for PCPU.

>
>> diff --git a/sys/compat/ndis/kern_windrv.c b/sys/compat/ndis/kern_windrv.c
>> index f231863..ba29fd2 100644
>> --- a/sys/compat/ndis/kern_windrv.c
>> +++ b/sys/compat/ndis/kern_windrv.c
>> @@ -613,8 +613,6 @@ struct gdt {
>>  extern uint16_t        x86_getfs(void);
>>  extern void x86_setfs(uint16_t);
>>  extern void *x86_gettid(void);
>> -extern void x86_critical_enter(void);
>> -extern void x86_critical_exit(void);
>>  extern void x86_getldt(struct gdt *, uint16_t *);
>>  extern void x86_setldt(struct gdt *, uint16_t);
>>
>> @@ -668,8 +666,10 @@ extern void x86_setldt(struct gdt *, uint16_t);
>>  void
>>  ctxsw_utow(void)
>>  {
>> -       struct tid              *t;
>> +       struct tid *t;
>>
>> +       sched_pin();
>> +       critical_enter();
>>        t = &my_tids[curthread->td_oncpu];
>>
>>        /*
>> @@ -685,12 +685,9 @@ ctxsw_utow(void)
>>        if (t->tid_self != t)
>>                x86_newldt(NULL);
>>
>> -       x86_critical_enter();
>>        t->tid_oldfs = x86_getfs();
>>        t->tid_cpu = curthread->td_oncpu;
>> -       sched_pin();
>>        x86_setfs(SEL_TO_FS(t->tid_selector));
>> -       x86_critical_exit();
>>
>>        /* Now entering Windows land, population: you. */
>>  }
>> @@ -706,11 +703,10 @@ ctxsw_wtou(void)
>>  {
>>        struct tid              *t;
>>
>> -       x86_critical_enter();
>>        t = x86_gettid();
>>        x86_setfs(t->tid_oldfs);
>> +       critical_exit();
>>        sched_unpin();
>> -       x86_critical_exit();
>>
>>        /* Welcome back to UNIX land, we missed you. */
>>
>> diff --git a/sys/compat/ndis/winx32_wrap.S b/sys/compat/ndis/winx32_wrap.S
>> index c051504..fa4aa87 100644
>> --- a/sys/compat/ndis/winx32_wrap.S
>> +++ b/sys/compat/ndis/winx32_wrap.S
>> @@ -375,11 +375,3 @@ ENTRY(x86_setfs)
>>  ENTRY(x86_gettid)
>>        mov     %fs:12,%eax
>>        ret
>> -
>> -ENTRY(x86_critical_enter)
>> -       cli
>> -       ret
>> -
>> -ENTRY(x86_critical_exit)
>> -       sti
>> -       ret
>> _______________________________________________
>> freebsd-net at freebsd.org mailing list
>> http://lists.freebsd.org/mailman/listinfo/freebsd-net
>> To unsubscribe, send any mail to "freebsd-net-unsubscribe at freebsd.org"
>>
>
>
>
> --
> Bernhard
>