Date: Wed, 20 Oct 2010 21:21:03 +0200 From: Paul B Mahol <onemda@gmail.com> To: Bernhard Schmidt <bschmidt@freebsd.org> Cc: net@freebsd.org Subject: Re: ndis: patch for review Message-ID: <AANLkTik=AHeu32TFekdr=Q8VGz21OL682OAb2%2B9xA4A5@mail.gmail.com> In-Reply-To: <AANLkTikknK1BcUDRWWSsBvOjaRz9ZVwSMAukisNWzyHV@mail.gmail.com> References: <AANLkTin6Pt-SbUPJ=gdZw=4Fym8MayoQOYkupGrikxMY@mail.gmail.com> <AANLkTikknK1BcUDRWWSsBvOjaRz9ZVwSMAukisNWzyHV@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 10/20/10, Bernhard Schmidt <bschmidt@freebsd.org> wrote:
> 9, 2010 at 23:53, Paul B Mahol <onemda@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@freebsd.org mailing list
>> http://lists.freebsd.org/mailman/listinfo/freebsd-net
>> To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"
>>
>
>
>
> --
> Bernhard
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTik=AHeu32TFekdr=Q8VGz21OL682OAb2%2B9xA4A5>