Xen devel - Jan 2007 - [PATCH] fix build when CONFIG_COMPAT disabled

Properly conditionalize two places where compat-only structure fields get
accessed, and avoid compiler warnings on unused variables in two other
places.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

Index: 2007-01-08/xen/arch/x86/mm.c
==================================================================---
2007-01-08.orig/xen/arch/x86/mm.c	2007-01-09 17:00:55.000000000 +0100
+++ 2007-01-08/xen/arch/x86/mm.c	2007-01-11 15:33:22.000000000 +0100
@@ -1143,10 +1143,12 @@ static int alloc_l4_table(struct page_in
     pl4e[l4_table_offset(PERDOMAIN_VIRT_START)]         
l4e_from_page(virt_to_page(d->arch.mm_perdomain_l3),
                       __PAGE_HYPERVISOR);
+#ifdef CONFIG_COMPAT
     if ( IS_COMPAT(d) )
         pl4e[l4_table_offset(COMPAT_ARG_XLAT_VIRT_BASE)]             
l4e_from_page(virt_to_page(d->arch.mm_arg_xlat_l3),
                           __PAGE_HYPERVISOR);
+#endif
 
     return 1;
 
@@ -1376,7 +1378,7 @@ static int mod_l2_entry(l2_pgentry_t *pl
         if ( !l2e_has_changed(ol2e, nl2e, _PAGE_PRESENT))
             return UPDATE_ENTRY(l2, pl2e, ol2e, nl2e, pfn, current);
 
-        if ( unlikely(!get_page_from_l2e(nl2e, pfn, current->domain)) )
+        if ( unlikely(!get_page_from_l2e(nl2e, pfn, d)) )
             return 0;
 
         if ( unlikely(!UPDATE_ENTRY(l2, pl2e, ol2e, nl2e, pfn, current)) )
@@ -1439,7 +1441,7 @@ static int mod_l3_entry(l3_pgentry_t *pl
         if (!l3e_has_changed(ol3e, nl3e, _PAGE_PRESENT))
             return UPDATE_ENTRY(l3, pl3e, ol3e, nl3e, pfn, current);
 
-        if ( unlikely(!get_page_from_l3e(nl3e, pfn, current->domain)) )
+        if ( unlikely(!get_page_from_l3e(nl3e, pfn, d)) )
             return 0;
 
         if ( unlikely(!UPDATE_ENTRY(l3, pl3e, ol3e, nl3e, pfn, current)) )
Index: 2007-01-08/xen/include/asm-x86/config.h
==================================================================---
2007-01-08.orig/xen/include/asm-x86/config.h	2007-01-08 14:54:33.000000000 +0100
+++ 2007-01-08/xen/include/asm-x86/config.h	2007-01-11 15:27:04.000000000 +0100
@@ -208,7 +208,11 @@ extern unsigned long _end; /* standard E
 
 /* This is not a fixed value, just a lower limit. */
 #define __HYPERVISOR_COMPAT_VIRT_START 0xF5800000
+#ifdef CONFIG_COMPAT
 #define HYPERVISOR_COMPAT_VIRT_START(d) ((d)->arch.hv_compat_vstart)
+#else
+#define HYPERVISOR_COMPAT_VIRT_START(d) 0
+#endif
 #define MACH2PHYS_COMPAT_VIRT_START    HYPERVISOR_COMPAT_VIRT_START
 #define MACH2PHYS_COMPAT_VIRT_END      0xFFE00000
 #define MACH2PHYS_COMPAT_NR_ENTRIES(d) \


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

On 11/1/07 15:42, "Jan Beulich" <jbeulich@novell.com> wrote:
> Properly conditionalize two places where compat-only structure fields get
> accessed, and avoid compiler warnings on unused variables in two other
> places.
Should we bother to #ifdef where we don''t need to? It''s ugly
and the
compiler should be able to remove dead code where a condition evaluates to
zero at compile time: we already have IS_COMPAT(d) hardcoded to zero if
!CONFIG_COMPAT, which is enough to give the compiler a fair chance.

 -- Keir


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

On Thu, Jan 11, 2007 at 03:47:25PM +0000, Keir Fraser wrote:
> > Properly conditionalize two places where compat-only structure fields
get
> > accessed, and avoid compiler warnings on unused variables in two other
> > places.
> 
> Should we bother to #ifdef where we don''t need to? It''s
ugly and the
> compiler should be able to remove dead code where a condition evaluates to
> zero at compile time: we already have IS_COMPAT(d) hardcoded to zero if
> !CONFIG_COMPAT, which is enough to give the compiler a fair chance.
> 
>  -- Keir
The problem is the COMPAT_ARG_XLAT_VIRT_BASE used in that chunk.

regards
john

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

>>> Keir Fraser <keir@xensource.com> 11.01.07 16:47 >>>
>On 11/1/07 15:42, "Jan Beulich" <jbeulich@novell.com> wrote:
>
>> Properly conditionalize two places where compat-only structure fields
get
>> accessed, and avoid compiler warnings on unused variables in two other
>> places.
>
>Should we bother to #ifdef where we don''t need to? It''s
ugly and the
>compiler should be able to remove dead code where a condition evaluates to
>zero at compile time: we already have IS_COMPAT(d) hardcoded to zero if
>!CONFIG_COMPAT, which is enough to give the compiler a fair chance.
Generally no, but in the one case in the patch we have to, as the structure
member (mm_arg_xlat_l3) doesn''t exist without CONFIG_COMPAT.

Jan

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

On 11/1/07 16:31, "Jan Beulich" <jbeulich@novell.com> wrote:
>> Should we bother to #ifdef where we don''t need to?
It''s ugly and the
>> compiler should be able to remove dead code where a condition evaluates
to
>> zero at compile time: we already have IS_COMPAT(d) hardcoded to zero if
>> !CONFIG_COMPAT, which is enough to give the compiler a fair chance.
> 
> Generally no, but in the one case in the patch we have to, as the structure
> member (mm_arg_xlat_l3) doesn''t exist without CONFIG_COMPAT.
CONFIG_COMPAT isn''t a real config option though. It''s a
function of the
architecture we''re building for. So we don''t need ifdef
CONFIG_COMPAT in any
x86/64-specific code.

I''m very keen to get rid of CONFIG_* where possible. In x86 code we can
variously remove ifdefs or turn them into CONFIG_X86_64. We probably need to
keep CONFIG_COMPAT in some common code I guess.

This really follows my view that much of this new code should simply be
viewed as an always-on x86/64 extension, and that code should be propagated
down into arch/x86 and arch/x86/x86_64 as much as possible to reflect that.

 -- Keir


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

On Thu, Jan 11, 2007 at 05:57:45PM +0000, Keir Fraser wrote:
> CONFIG_COMPAT isn''t a real config option though. It''s a
function of the
> architecture we''re building for. So we don''t need ifdef
CONFIG_COMPAT in any
> x86/64-specific code.
Assuming we can get it at least building on non-Linux, it''s fine by me.
Rewriting in Python was less painful than I suspected, so there''s just
the pragma issue left.

regards
john

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

>CONFIG_COMPAT isn''t a real config option though. It''s a
function of the
>architecture we''re building for. So we don''t need ifdef
CONFIG_COMPAT in any
>x86/64-specific code.
Okay, I didn''t view it that way - I really thought that it
wouldn''t be bad to be
able to turn off optional features like HVM or COMPAT...

Jan

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