Re: ndis: patch for review
On 10/20/10, Bernhard Schmidt wrote:
> 9, 2010 at 23:53, Paul B Mahol 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_tx86_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-unsubscr...@freebsd.org"
>>
>
>
>
> --
> Bernhard
>
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
Re: ndis: patch for review
9, 2010 at 23:53, Paul B Mahol 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?
> 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-unsubscr...@freebsd.org"
>
--
Bernhard
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
ndis: patch for review
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.
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_tx86_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-unsubscr...@freebsd.org"
