[patch] unwanted sign extending

[patch] unwanted sign extending

[Scott Parish]

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


static int alloc_l3_table(struct pfn_info *page) 
{ 
  ...
    unsigned long  vaddr; 
    unsigned int   i; 
  ...
    for ( i = 0; i < L3_PAGETABLE_ENTRIES; i++ ) 
    { 
        vaddr = i << L3_PAGETABLE_SHIFT; 
  ...
    } 
...
}

"i" gets sign extended when its shifted, so vaddr has all its high
bits set. Because of that some l2 page_type's come out looking like
PGT_writable instead of PGT_l2. Eventually this leads to an attempt to
call put_page_type on the page twice, once when cleaning up recursively
from l4, and once from walking the raw frames list. The second
put_page_type hits the ASSERT that the type count isn't 0.

With the attached patch, i can completely run a simple "hello world"
domu, and its cleanup. Linux domu still probably doesn't work.

sRp

-- 
Scott Parish
Signed-off-by: srparish@us.ibm.com

[-- Attachment #2: no-sign-extension.diff --]
[-- Type: text/plain, Size: 394 bytes --]

--- old-xen-build/xen/arch/x86/mm.c	2005-06-14 15:55:34.000000000 +0000
+++ new-xen-build/xen/arch/x86/mm.c	2005-06-21 20:35:09.000000000 +0000
@@ -814,7 +814,7 @@ static int alloc_l3_table(struct pfn_inf
     unsigned long  pfn = page_to_pfn(page);
     unsigned long  vaddr;
     l3_pgentry_t  *pl3e;
-    int            i;
+    unsigned int   i;
 
     ASSERT(!shadow_mode_refcounts(d));
 


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

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

Re: [patch] unwanted sign extending

[Jan Beulich]

I'd think that for correctness this should also be done to alloc_l2_table. And I also think that this is still wrong for 64 bits: Shifting left an unsigned yields an unsigned, and since 'i' can range from 0 to 511 and the shift count is 30, the result is going to be truncated. That is, the code should be

        vaddr = (unsigned long)i << L3_PAGETABLE_SHIFT; 

(and again, for consistency it should also be done so in alloc_l2_table).

Jan

>>> "Scott Parish" <srparish@us.ibm.com> 21.06.05 22:10:30 >>>

static int alloc_l3_table(struct pfn_info *page) 
{ 
  ...
    unsigned long  vaddr; 
    unsigned int   i; 
  ...
    for ( i = 0; i < L3_PAGETABLE_ENTRIES; i++ ) 
    { 
        vaddr = i << L3_PAGETABLE_SHIFT; 
  ...
    } 
...
}

"i" gets sign extended when its shifted, so vaddr has all its high
bits set. Because of that some l2 page_type's come out looking like
PGT_writable instead of PGT_l2. Eventually this leads to an attempt to
call put_page_type on the page twice, once when cleaning up recursively
from l4, and once from walking the raw frames list. The second
put_page_type hits the ASSERT that the type count isn't 0.

With the attached patch, i can completely run a simple "hello world"
domu, and its cleanup. Linux domu still probably doesn't work.

sRp

-- 
Scott Parish
Signed-off-by: srparish@us.ibm.com

Re: [patch] unwanted sign extending

[Scott Parish]

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

On Wed, Jun 22, 2005 at 01:14:34AM -0600, Jan Beulich wrote:

> I'd think that for correctness this should also be done to
> alloc_l2_table. And I also think that this is still wrong for 64 bits:
> Shifting left an unsigned yields an unsigned, and since 'i' can range
> from 0 to 511 and the shift count is 30, the result is going to be
> truncated. That is, the code should be
>
>         vaddr = (unsigned long)i << L3_PAGETABLE_SHIFT;
>
> (and again, for consistency it should also be done so in
> alloc_l2_table).

Good point

sRp

-- 
Scott Parish
Signed-off-by: srparish@us.ibm.com

[-- Attachment #2: no-sign-extend-2.diff --]
[-- Type: text/plain, Size: 1641 bytes --]

--- old-xen-build/xen/arch/x86/mm.c	2005-06-14 15:55:34.000000000 +0000
+++ new-xen-build/xen/arch/x86/mm.c	2005-06-22 12:06:46.000000000 +0000
@@ -658,7 +658,7 @@ static int alloc_l1_table(struct pfn_inf
     struct domain *d = page_get_owner(page);
     unsigned long  pfn = page_to_pfn(page);
     l1_pgentry_t  *pl1e;
-    int            i;
+    unsigned long  i;
 
     ASSERT(!shadow_mode_refcounts(d));
 
@@ -687,7 +687,7 @@ static int create_pae_xen_mappings(l3_pg
     struct pfn_info *page;
     l2_pgentry_t    *pl2e;
     l3_pgentry_t     l3e3;
-    int              i;
+    unsigned long    i;
 
     pl3e = (l3_pgentry_t *)((unsigned long)pl3e & PAGE_MASK);
 
@@ -762,7 +762,7 @@ static int alloc_l2_table(struct pfn_inf
     unsigned long  pfn = page_to_pfn(page);
     unsigned long  vaddr;
     l2_pgentry_t  *pl2e;
-    int            i;
+    unsigned long  i;
 
     /* See the code in shadow_promote() to understand why this is here. */
     if ( (PGT_base_page_table == PGT_l2_page_table) &&
@@ -814,7 +814,7 @@ static int alloc_l3_table(struct pfn_inf
     unsigned long  pfn = page_to_pfn(page);
     unsigned long  vaddr;
     l3_pgentry_t  *pl3e;
-    int            i;
+    unsigned long  i;
 
     ASSERT(!shadow_mode_refcounts(d));
 
@@ -851,7 +851,7 @@ static int alloc_l4_table(struct pfn_inf
     struct domain *d = page_get_owner(page);
     unsigned long  pfn = page_to_pfn(page);
     l4_pgentry_t  *pl4e = page_to_virt(page);
-    int            i;
+    unsigned long  i;
 
     /* See the code in shadow_promote() to understand why this is here. */
     if ( (PGT_base_page_table == PGT_l4_page_table) &&

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

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