Xen devel - May 2006 - [PATCH] Fix mem.c so that X Windows can restart

This patch fixes the problem where you cannot start X Windows on Dom0 after
you have created an HVM guest.  The problem is that X uses `mmap'' to
map
1 page at physical address 0 with read/write permission.  Before an HVM
guest is created this check at around line 1496 of `mm.c'':

	if ( unlikely((x & (PGT_type_mask|PGT_va_mask)) != type) )

causes the offending code from this patch to be bypassed and X''s
`mmap''
call works.  After an HVM guest is created the check at 1496 of `mm.c''
is
now true and the code from this patch is executed, causing the `mmap''
call to
erroneously fail.

I''d be curious if someone can explain what this code is trying to do
and,
if this patch is appropriate, it should be applied.



Signed-off-by: Don Dugger <donald.d.dugger@intel.com>

-- 
Don Dugger
"Censeo Toto nos in Kansa esse decisse." - D. Gale
Donald.D.Dugger@intel.com
Ph: (303)440-1368



diff -r ad33b3882867 xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c	Wed May 24 19:41:47 2006 +0100
+++ b/xen/arch/x86/mm.c	Thu May 25 10:47:51 2006 -0600
@@ -1537,9 +1537,7 @@ int get_page_type(struct page_info *page
                         return 0;
 #endif
                     /* Fixme: add code to propagate va_unknown to subtables. */
-                    if ( ((type & PGT_type_mask) >= PGT_l2_page_table)
&&
-                         !shadow_mode_refcounts(page_get_owner(page)) )
-                        return 0;
+
                     /* This table is possibly mapped at multiple locations. */
                     nx &= ~PGT_va_mask;
                     nx |= PGT_va_unknown;

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

On 25 May 2006, at 20:32, Donald D. Dugger wrote:
> This patch fixes the problem where you cannot start X Windows on Dom0 
> after
> you have created an HVM guest.  The problem is that X uses `mmap''
to
> map
> 1 page at physical address 0 with read/write permission.  Before an HVM
> guest is created this check at around line 1496 of `mm.c'':
>
> 	if ( unlikely((x & (PGT_type_mask|PGT_va_mask)) != type) )
>
> causes the offending code from this patch to be bypassed and X''s
`mmap''
> call works.  After an HVM guest is created the check at 1496 of
`mm.c''
> is
> now true and the code from this patch is executed, causing the
`mmap''
> call to
> erroneously fail.
This is totally bogus. That test (line 1496) should *never* be true for 
pages that are not type PGT_l*_page_table. I think some HVM code is 
indexing frame_table with a zero frame number and then fiddling with 
fields in that very first page_info structure. That''s very bad -- in 
fact it''s tempting to add some assertions about page 0 to be tested at 
various suitable moments to prevent this sort of bad behaviour.

  -- Keir


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

>>> On 5/25/2006 at 2:52 PM, in message
<6c2ece1dc117d8bbafad32bb376eb273@cl.cam.ac.uk>, Keir Fraser
<Keir.Fraser@cl.cam.ac.uk> wrote:
> On 25 May 2006, at 20:32, Donald D. Dugger wrote:
> 
>> This patch fixes the problem where you cannot start X Windows on
Dom0 
>> after
>> you have created an HVM guest.  The problem is that X uses
`mmap'' to
>> map
>> 1 page at physical address 0 with read/write permission.  Before an
HVM
>> guest is created this check at around line 1496 of `mm.c'':
>>
>> 	if ( unlikely((x & (PGT_type_mask|PGT_va_mask)) != type) )
>>
>> causes the offending code from this patch to be bypassed and
X''s
`mmap''
>> call works.  After an HVM guest is created the check at 1496 of
`mm.c'' 
>> is
>> now true and the code from this patch is executed, causing the
`mmap'' 
>> call to
>> erroneously fail.
> 
> This is totally bogus. That test (line 1496) should *never* be true
for 
> pages that are not type PGT_l*_page_table. I think some HVM code is 
> indexing frame_table with a zero frame number and then fiddling with
> fields in that very first page_info structure. That''s very bad --
in
> fact it''s tempting to add some assertions about page 0 to be
tested
at 
> various suitable moments to prevent this sort of bad behaviour.
Keir,
We hit this very frequently.  Any chance you could put together a patch
with the asserts in place so we can throw it into our code base to try
and catch the problem?
Clyde
> 
>   -- Keir
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com 
> http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel