Avoid alloc for xsave before xsave_init

Avoid alloc for xsave before xsave_init

[Wei, Gang]

While debugging some weird booting failure bugs, just found currently, xsave_alloc_save_area will be called in init_idle_domain->scheduler_init->alloc_vcpu->vcpu_initialise calls, it is earlier than xsave_init called in identity_cpu(). This may causing buffer overflow on xmem_pool. I am thinking about how to fix it.

Jimmy

RE: Avoid alloc for xsave before xsave_init

[Huang2, Wei]

Hi Gang,

Was the issue caused by the uninitialized variable xsave_cntxt_size, triggering problem for _xmalloc()? If so, one solution is to set xsave_cntxt_size=576 (the default value after reset) as a default value. When xsave_alloc_save_area() is called for idel VCPU, _xmalloc() will initialize 576 bytes. Idle domain doesn't change xcr0 from my understanding. So its xcr0 is XSTATE_FP_SSE all the time. 

Best,
-Wei

-----Original Message-----
From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Wei, Gang
Sent: Thursday, January 13, 2011 12:49 PM
To: xen-devel@lists.xensource.com
Cc: Keir Fraser; Wei, Gang
Subject: [Xen-devel] Avoid alloc for xsave before xsave_init

While debugging some weird booting failure bugs, just found currently, xsave_alloc_save_area will be called in init_idle_domain->scheduler_init->alloc_vcpu->vcpu_initialise calls, it is earlier than xsave_init called in identity_cpu(). This may causing buffer overflow on xmem_pool. I am thinking about how to fix it.

Jimmy

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: Avoid alloc for xsave before xsave_init

[Keir Fraser]

On 13/01/2011 18:48, "Wei, Gang" <gang.wei@intel.com> wrote:

> While debugging some weird booting failure bugs, just found currently,
> xsave_alloc_save_area will be called in
> init_idle_domain->scheduler_init->alloc_vcpu->vcpu_initialise calls, it is
> earlier than xsave_init called in identity_cpu(). This may causing buffer
> overflow on xmem_pool. I am thinking about how to fix it.

I doubt idle vcpus need an xsave context. Can we check for is_idle_vcpu() in
xsave_{alloc,free}_save_area()?

Is this an issue only for xen-unstable/4.1 (not 4.0)?

 -- Keir

> Jimmy

Re: Avoid alloc for xsave before xsave_init

[Keir Fraser]

On 13/01/2011 20:21, "Huang2, Wei" <Wei.Huang2@amd.com> wrote:

> Hi Gang,
> 
> Was the issue caused by the uninitialized variable xsave_cntxt_size,
> triggering problem for _xmalloc()? If so, one solution is to set
> xsave_cntxt_size=576 (the default value after reset) as a default value. When
> xsave_alloc_save_area() is called for idel VCPU, _xmalloc() will initialize
> 576 bytes. Idle domain doesn't change xcr0 from my understanding. So its xcr0
> is XSTATE_FP_SSE all the time.

Idle domain isn't using FPU,SSE,AVX or any such extended state and doesn't
need it saved. Xsave_{alloc,free}_save_area() should test-and-exit on
is_idle_vcpu(), and our context switch code should not be doing XSAVE when
switching out an idle vcpu (I hope this is the case already, as it would be
a pointless waste of time).

 -- Keir

> Best,
> -Wei
> 
> -----Original Message-----
> From: xen-devel-bounces@lists.xensource.com
> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Wei, Gang
> Sent: Thursday, January 13, 2011 12:49 PM
> To: xen-devel@lists.xensource.com
> Cc: Keir Fraser; Wei, Gang
> Subject: [Xen-devel] Avoid alloc for xsave before xsave_init
> 
> While debugging some weird booting failure bugs, just found currently,
> xsave_alloc_save_area will be called in
> init_idle_domain->scheduler_init->alloc_vcpu->vcpu_initialise calls, it is
> earlier than xsave_init called in identity_cpu(). This may causing buffer
> overflow on xmem_pool. I am thinking about how to fix it.
> 
> Jimmy
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 
> 

Re: Avoid alloc for xsave before xsave_init

[Wei Huang]

The test showed on my machine showed that xsave areas of idle vcpus are 
inconsistent. With four CPUs, the code did the following:

1. vcpu 0 of idle domain
* xsave_alloc_save_area() is called.
* xsave_cntxt_size is 0; so vcpu->arch.xsave_area is 0 bytes.
* vcpu->arch.xcr0 and vcpu->arch.xcr0_accum is 0x3.
2. Then, xsave_init() is called. xsave_cntxt_size is now initialized 
correctly.
3. After that, vcpu 1, 2, 3 of idle domain have
* xsave_alloc_save_area() is called.
* xsave_cntxt_size is correct; so vcpu->arch.xsave_area points to an 
allocated area.
* vcpu->arch.xcr0 and vcpu->arch.xcr0_accum is 0x3.

In other words, vcpu0 has a different xsave_area from other vcpus. I 
think the following patch should fix the issues above:


diff -r 20b0f709153e xen/arch/x86/i387.c
--- a/xen/arch/x86/i387.c       Wed Jan 12 14:14:13 2011 +0000
+++ b/xen/arch/x86/i387.c       Thu Jan 13 18:08:30 2011 -0600
@@ -33,7 +33,7 @@
      if ( cr0 & X86_CR0_TS )
          clts();

-    if ( cpu_has_xsave )
+    if ( cpu_has_xsave && !is_idle_vcpu(v) )
      {
          /* XCR0 normally represents what guest OS set. In case of Xen 
itself,
           * we set all accumulated feature mask before doing save/restore.
@@ -214,7 +214,7 @@
  {
      void *save_area;

-    if ( !cpu_has_xsave )
+    if ( !cpu_has_xsave || is_idle_vcpu(v) )
          return 0;

      /* XSAVE/XRSTOR requires the save area be 64-byte-boundary aligned. */



On 01/13/2011 03:15 PM, Keir Fraser wrote:
> On 13/01/2011 20:21, "Huang2, Wei"<Wei.Huang2@amd.com>  wrote:
>
>> Hi Gang,
>>
>> Was the issue caused by the uninitialized variable xsave_cntxt_size,
>> triggering problem for _xmalloc()? If so, one solution is to set
>> xsave_cntxt_size=576 (the default value after reset) as a default value. When
>> xsave_alloc_save_area() is called for idel VCPU, _xmalloc() will initialize
>> 576 bytes. Idle domain doesn't change xcr0 from my understanding. So its xcr0
>> is XSTATE_FP_SSE all the time.
> Idle domain isn't using FPU,SSE,AVX or any such extended state and doesn't
> need it saved. Xsave_{alloc,free}_save_area() should test-and-exit on
> is_idle_vcpu(), and our context switch code should not be doing XSAVE when
> switching out an idle vcpu (I hope this is the case already, as it would be
> a pointless waste of time).
>
>   -- Keir
>
>> Best,
>> -Wei
>>
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xensource.com
>> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Wei, Gang
>> Sent: Thursday, January 13, 2011 12:49 PM
>> To: xen-devel@lists.xensource.com
>> Cc: Keir Fraser; Wei, Gang
>> Subject: [Xen-devel] Avoid alloc for xsave before xsave_init
>>
>> While debugging some weird booting failure bugs, just found currently,
>> xsave_alloc_save_area will be called in
>> init_idle_domain->scheduler_init->alloc_vcpu->vcpu_initialise calls, it is
>> earlier than xsave_init called in identity_cpu(). This may causing buffer
>> overflow on xmem_pool. I am thinking about how to fix it.
>>
>> Jimmy
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>>
>>
>
>

RE: Avoid alloc for xsave before xsave_init

[Wei, Gang]

Keir Fraser wrote on 2011-01-14:
> On 13/01/2011 18:48, "Wei, Gang" <gang.wei@intel.com> wrote:
> 
>> While debugging some weird booting failure bugs, just found
>> currently, xsave_alloc_save_area will be called in
>> init_idle_domain->scheduler_init->alloc_vcpu->vcpu_initialise calls,
>> it is earlier than xsave_init called in identity_cpu(). This may
>> causing buffer overflow on xmem_pool. I am thinking about how to fix it.
> 
> I doubt idle vcpus need an xsave context. Can we check for
> is_idle_vcpu() in xsave_{alloc,free}_save_area()?
> 
> Is this an issue only for xen-unstable/4.1 (not 4.0)?

This issue was induced by c/s 22345 two months ago, which moved the xsave alloc code out from hvm_vcpu_initialise() to support pv guest but forget to exclude idle vcpus. It looks like not back pulled to 4.0. So only 4.1 suffers from it.

Jimmy

RE: Avoid alloc for xsave before xsave_init

[Wei, Gang]

Keir Fraser wrote on 2011-01-14:
> On 13/01/2011 20:21, "Huang2, Wei" <Wei.Huang2@amd.com> wrote:
>> Was the issue caused by the uninitialized variable xsave_cntxt_size,
>> triggering problem for _xmalloc()? If so, one solution is to set
>> xsave_cntxt_size=576 (the default value after reset) as a default
>> value. When
>> xsave_alloc_save_area() is called for idel VCPU, _xmalloc() will
>> initialize
>> 576 bytes. Idle domain doesn't change xcr0 from my understanding. So
>> its xcr0 is XSTATE_FP_SSE all the time.
> 
> Idle domain isn't using FPU,SSE,AVX or any such extended state and
> doesn't need it saved. Xsave_{alloc,free}_save_area() should
> test-and-exit on is_idle_vcpu(), and our context switch code should
> not be doing XSAVE when switching out an idle vcpu (I hope this is the
> case already, as it would be a pointless waste of time).

I agree that do test-and-exit on is_idle_vcpu() in Xsave_{alloc,free}_save_area. Further, We'd better add assert(xsave_cntxt_size>=576) after the test-and-exit clause to ensure no buffer overflow will happen in the future.

I reviewed the context switch code and assure context switch code not be doing XSAVE when switching out an idle vcpu.

Jimmy
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xensource.com
>> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Wei,
>> Gang
>> Sent: Thursday, January 13, 2011 12:49 PM
>> To: xen-devel@lists.xensource.com
>> Cc: Keir Fraser; Wei, Gang
>> Subject: [Xen-devel] Avoid alloc for xsave before xsave_init
>> 
>> While debugging some weird booting failure bugs, just found
>> currently, xsave_alloc_save_area will be called in
>> init_idle_domain->scheduler_init->alloc_vcpu->vcpu_initialise calls,
>> it is earlier than xsave_init called in identity_cpu(). This may
>> causing buffer overflow on xmem_pool. I am thinking about how to fix it.

RE: Avoid alloc for xsave before xsave_init

[Wei, Gang]

[-- Attachment #1: Type: text/plain, Size: 3796 bytes --]

>>>> While debugging some weird booting failure bugs, just found
>>>> currently, xsave_alloc_save_area will be called in
>>>> init_idle_domain->scheduler_init->alloc_vcpu->vcpu_initialise
>>>> calls, it is earlier than xsave_init called in identity_cpu(). This
>>>> may causing buffer overflow on xmem_pool. I am thinking about how to fix it.
>>>
>>> Was the issue caused by the uninitialized variable
>>> xsave_cntxt_size, triggering problem for _xmalloc()? If so, one
>>> solution is to set
>>> xsave_cntxt_size=576 (the default value after reset) as a default
>>> value. When
>>> xsave_alloc_save_area() is called for idel VCPU, _xmalloc() will
>>> initialize
>>> 576 bytes. Idle domain doesn't change xcr0 from my understanding.
>>> So its xcr0 is XSTATE_FP_SSE all the time.
>> 
>> Idle domain isn't using FPU,SSE,AVX or any such extended state and
>> doesn't need it saved. Xsave_{alloc,free}_save_area() should
>> test-and-exit on is_idle_vcpu(), and our context switch code should
>> not be doing XSAVE when switching out an idle vcpu (I hope this is
>> the case already, as it would be a pointless waste of time).
> 
> I agree that do test-and-exit on is_idle_vcpu() in Xsave_{alloc,free}_save_area.
> Further, We'd better add assert(xsave_cntxt_size>=576) after the
> test-and-exit clause to ensure no buffer overflow will happen in the future.
> 
> I reviewed the context switch code and assure context switch code not
> be doing XSAVE when switching out an idle vcpu.

Here is the patch.

diff -r d839631b6048 xen/arch/x86/i387.c
--- a/xen/arch/x86/i387.c	Thu Jan 13 00:18:35 2011 +0000
+++ b/xen/arch/x86/i387.c	Sat Jan 15 20:15:24 2011 +0800
@@ -35,6 +35,9 @@ void save_init_fpu(struct vcpu *v)
 
     if ( cpu_has_xsave )
     {
+        if ( is_idle_vcpu(v) )
+            goto out;
+
         /* XCR0 normally represents what guest OS set. In case of Xen itself,
          * we set all accumulated feature mask before doing save/restore.
          */
@@ -87,6 +90,7 @@ void save_init_fpu(struct vcpu *v)
         asm volatile ( "fnsave %0 ; fwait" : "=m" (*fpu_ctxt) );
     }
 
+out:
     v->fpu_dirtied = 0;
     write_cr0(cr0|X86_CR0_TS);
 }
@@ -138,6 +142,7 @@ void restore_fpu(struct vcpu *v)
 }
 
 #define XSTATE_CPUID 0xd
+#define XSAVE_AREA_MIN_SIZE (512 + 64)
 
 /*
  * Maximum size (in byte) of the XSAVE/XRSTOR save area required by all
@@ -177,7 +182,7 @@ void xsave_init(void)
     }
 
     /* FP/SSE, XSAVE.HEADER, YMM */
-    min_size =  512 + 64 + ((eax & XSTATE_YMM) ? XSTATE_YMM_SIZE : 0);
+    min_size =  XSAVE_AREA_MIN_SIZE + ((eax & XSTATE_YMM) ? XSTATE_YMM_SIZE : 0);
     BUG_ON(ecx < min_size);
 
     /*
@@ -214,8 +219,10 @@ int xsave_alloc_save_area(struct vcpu *v
 {
     void *save_area;
 
-    if ( !cpu_has_xsave )
+    if ( !cpu_has_xsave || is_idle_vcpu(v) )
         return 0;
+
+    BUG_ON(xsave_cntxt_size < XSAVE_AREA_MIN_SIZE);
 
     /* XSAVE/XRSTOR requires the save area be 64-byte-boundary aligned. */
     save_area = _xmalloc(xsave_cntxt_size, 64);
@@ -235,6 +242,9 @@ int xsave_alloc_save_area(struct vcpu *v
 
 void xsave_free_save_area(struct vcpu *v)
 {
+    if ( is_idle_vcpu(v) )
+        return;
+
     xfree(v->arch.xsave_area);
     v->arch.xsave_area = NULL;
 }
diff -r d839631b6048 xen/include/asm-x86/i387.h
--- a/xen/include/asm-x86/i387.h	Thu Jan 13 00:18:35 2011 +0000
+++ b/xen/include/asm-x86/i387.h	Sat Jan 15 20:08:14 2011 +0800
@@ -134,6 +134,9 @@ static inline void setup_fpu(struct vcpu
         v->fpu_dirtied = 1;
         if ( cpu_has_xsave )
         {
+            if ( is_idle_vcpu(v) )
+                return;
+
             if ( !v->fpu_initialised )
                 v->fpu_initialised = 1;

Jimmy

[-- Attachment #2: xsave_init_fix.patch --]
[-- Type: application/octet-stream, Size: 2959 bytes --]

X86: Avoid buffer overflow caused by calling xsave_alloc_save_area before xsave_init

Currently, xsave_alloc_save_area will be called in init_idle_domain->scheduler_init->alloc_vcpu->vcpu_initialise calls with xsave_cntxt_size=0, it is earlier than xsave_init called in identity_cpu(). This may causing buffer overflow on xmem_pool.

Idle domain isn't using FPU,SSE,AVX or any such extended state and doesn't need it saved. Xsave_{alloc,free}_save_area() should test-and-exit on is_idle_vcpu(), and our context switch code should not be doing XSAVE when switching out an idle vcpu.

Meanwhile add !is_idle_vcpu() check to fns calling xsave/xrstor, and a BUG_ON in xsave_alloc_save_area() to avoid any possible buffer overflow.

Signed-off-by: Wei Gang <gang.wei@intel.com>

diff -r d839631b6048 xen/arch/x86/i387.c
--- a/xen/arch/x86/i387.c	Thu Jan 13 00:18:35 2011 +0000
+++ b/xen/arch/x86/i387.c	Sat Jan 15 20:15:24 2011 +0800
@@ -35,6 +35,9 @@ void save_init_fpu(struct vcpu *v)
 
     if ( cpu_has_xsave )
     {
+        if ( is_idle_vcpu(v) )
+            goto out;
+
         /* XCR0 normally represents what guest OS set. In case of Xen itself,
          * we set all accumulated feature mask before doing save/restore.
          */
@@ -87,6 +90,7 @@ void save_init_fpu(struct vcpu *v)
         asm volatile ( "fnsave %0 ; fwait" : "=m" (*fpu_ctxt) );
     }
 
+out:
     v->fpu_dirtied = 0;
     write_cr0(cr0|X86_CR0_TS);
 }
@@ -138,6 +142,7 @@ void restore_fpu(struct vcpu *v)
 }
 
 #define XSTATE_CPUID 0xd
+#define XSAVE_AREA_MIN_SIZE (512 + 64)
 
 /*
  * Maximum size (in byte) of the XSAVE/XRSTOR save area required by all
@@ -177,7 +182,7 @@ void xsave_init(void)
     }
 
     /* FP/SSE, XSAVE.HEADER, YMM */
-    min_size =  512 + 64 + ((eax & XSTATE_YMM) ? XSTATE_YMM_SIZE : 0);
+    min_size =  XSAVE_AREA_MIN_SIZE + ((eax & XSTATE_YMM) ? XSTATE_YMM_SIZE : 0);
     BUG_ON(ecx < min_size);
 
     /*
@@ -214,8 +219,10 @@ int xsave_alloc_save_area(struct vcpu *v
 {
     void *save_area;
 
-    if ( !cpu_has_xsave )
+    if ( !cpu_has_xsave || is_idle_vcpu(v) )
         return 0;
+
+    BUG_ON(xsave_cntxt_size < XSAVE_AREA_MIN_SIZE);
 
     /* XSAVE/XRSTOR requires the save area be 64-byte-boundary aligned. */
     save_area = _xmalloc(xsave_cntxt_size, 64);
@@ -235,6 +242,9 @@ int xsave_alloc_save_area(struct vcpu *v
 
 void xsave_free_save_area(struct vcpu *v)
 {
+    if ( is_idle_vcpu(v) )
+        return;
+
     xfree(v->arch.xsave_area);
     v->arch.xsave_area = NULL;
 }
diff -r d839631b6048 xen/include/asm-x86/i387.h
--- a/xen/include/asm-x86/i387.h	Thu Jan 13 00:18:35 2011 +0000
+++ b/xen/include/asm-x86/i387.h	Sat Jan 15 20:08:14 2011 +0800
@@ -134,6 +134,9 @@ static inline void setup_fpu(struct vcpu
         v->fpu_dirtied = 1;
         if ( cpu_has_xsave )
         {
+            if ( is_idle_vcpu(v) )
+                return;
+
             if ( !v->fpu_initialised )
                 v->fpu_initialised = 1;
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: Avoid alloc for xsave before xsave_init

[Keir Fraser]

On 14/01/2011 07:11, "Wei, Gang" <gang.wei@intel.com> wrote:

>> I agree that do test-and-exit on is_idle_vcpu() in
>> Xsave_{alloc,free}_save_area.
>> Further, We'd better add assert(xsave_cntxt_size>=576) after the
>> test-and-exit clause to ensure no buffer overflow will happen in the future.
>> 
>> I reviewed the context switch code and assure context switch code not
>> be doing XSAVE when switching out an idle vcpu.
> 
> Here is the patch.

I applied your patch, plus some cleanup, as of c/s 22751.

 -- Keir

> diff -r d839631b6048 xen/arch/x86/i387.c
> --- a/xen/arch/x86/i387.c Thu Jan 13 00:18:35 2011 +0000
> +++ b/xen/arch/x86/i387.c Sat Jan 15 20:15:24 2011 +0800
> @@ -35,6 +35,9 @@ void save_init_fpu(struct vcpu *v)
>  
>      if ( cpu_has_xsave )
>      {
> +        if ( is_idle_vcpu(v) )
> +            goto out;
> +
>          /* XCR0 normally represents what guest OS set. In case of Xen itself,
>           * we set all accumulated feature mask before doing save/restore.
>           */
> @@ -87,6 +90,7 @@ void save_init_fpu(struct vcpu *v)
>          asm volatile ( "fnsave %0 ; fwait" : "=m" (*fpu_ctxt) );
>      }
>  
> +out:
>      v->fpu_dirtied = 0;
>      write_cr0(cr0|X86_CR0_TS);
>  }
> @@ -138,6 +142,7 @@ void restore_fpu(struct vcpu *v)
>  }
>  
>  #define XSTATE_CPUID 0xd
> +#define XSAVE_AREA_MIN_SIZE (512 + 64)
>  
>  /*
>   * Maximum size (in byte) of the XSAVE/XRSTOR save area required by all
> @@ -177,7 +182,7 @@ void xsave_init(void)
>      }
>  
>      /* FP/SSE, XSAVE.HEADER, YMM */
> -    min_size =  512 + 64 + ((eax & XSTATE_YMM) ? XSTATE_YMM_SIZE : 0);
> +    min_size =  XSAVE_AREA_MIN_SIZE + ((eax & XSTATE_YMM) ? XSTATE_YMM_SIZE :
> 0);
>      BUG_ON(ecx < min_size);
>  
>      /*
> @@ -214,8 +219,10 @@ int xsave_alloc_save_area(struct vcpu *v
>  {
>      void *save_area;
>  
> -    if ( !cpu_has_xsave )
> +    if ( !cpu_has_xsave || is_idle_vcpu(v) )
>          return 0;
> +
> +    BUG_ON(xsave_cntxt_size < XSAVE_AREA_MIN_SIZE);
>  
>      /* XSAVE/XRSTOR requires the save area be 64-byte-boundary aligned. */
>      save_area = _xmalloc(xsave_cntxt_size, 64);
> @@ -235,6 +242,9 @@ int xsave_alloc_save_area(struct vcpu *v
>  
>  void xsave_free_save_area(struct vcpu *v)
>  {
> +    if ( is_idle_vcpu(v) )
> +        return;
> +
>      xfree(v->arch.xsave_area);
>      v->arch.xsave_area = NULL;
>  }
> diff -r d839631b6048 xen/include/asm-x86/i387.h
> --- a/xen/include/asm-x86/i387.h Thu Jan 13 00:18:35 2011 +0000
> +++ b/xen/include/asm-x86/i387.h Sat Jan 15 20:08:14 2011 +0800
> @@ -134,6 +134,9 @@ static inline void setup_fpu(struct vcpu
>          v->fpu_dirtied = 1;
>          if ( cpu_has_xsave )
>          {
> +            if ( is_idle_vcpu(v) )
> +                return;
> +
>              if ( !v->fpu_initialised )
>                  v->fpu_initialised = 1;
> 
> Jimmy

RE: Avoid alloc for xsave before xsave_init

[Wei, Gang]

Keir Fraser wrote on 2011-01-14:
> On 14/01/2011 07:11, "Wei, Gang" <gang.wei@intel.com> wrote:
> 
>>> I agree that do test-and-exit on is_idle_vcpu() in
>>> Xsave_{alloc,free}_save_area.
>>> Further, We'd better add assert(xsave_cntxt_size>=576) after the
>>> test-and-exit clause to ensure no buffer overflow will happen in the future.
>>> 
>>> I reviewed the context switch code and assure context switch code
>>> not be doing XSAVE when switching out an idle vcpu.
>> 
>> Here is the patch.
> 
> I applied your patch, plus some cleanup, as of c/s 22751.

Your cleanup looks good.

Jimmy