HYPERVISOR_VIRT_START is an unsigned long constant. In shadow code, Xen
compares this value with a int which is wrong.
multi.c:1981:5: error: comparison of constant 8796025913344 with expression of
type ''int'' is always true
[-Werror,-Wtautological-constant-out-of-range-compare]
SHADOW_FOREACH_L2E(sl2mfn, sl2e, 0, 0, v->domain, {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
multi.c:1320:18: note: expanded from macro
''SHADOW_FOREACH_L2E''
< (HYPERVISOR_VIRT_START >> SHADOW_L2_PAGETABLE_SHIFT) ) \
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
xen/arch/x86/mm/shadow/multi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index a8ef75e..f2e70d2 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -1308,7 +1308,8 @@ do {
\
/* 32-bit l2 on PAE/64: four pages, touch every second entry, and avoid Xen */
#define SHADOW_FOREACH_L2E(_sl2mfn, _sl2e, _gl2p, _done, _dom, _code) \
do { \
- int _i, _j, __done = 0; \
+ unsigned long _i, _j; \
+ int __done = 0; \
int _xen = !shadow_mode_external(_dom); \
ASSERT(mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_32_shadow);\
for ( _j = 0; _j < 4 && !__done; _j++ )
\
@@ -1335,7 +1336,7 @@ do {
\
/* PAE: if it''s an l2h, don''t touch Xen mappings */
#define SHADOW_FOREACH_L2E(_sl2mfn, _sl2e, _gl2p, _done, _dom, _code) \
do { \
- int _i; \
+ unsigned long _i; \
int _xen = !shadow_mode_external(_dom); \
shadow_l2e_t *_sp = sh_map_domain_page((_sl2mfn)); \
ASSERT(mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_pae_shadow \
--
1.7.10.4
Jan Beulich
2013-May-02 10:37 UTC
Re: [PATCH] xen/x86: Fix compilation issue with clang 3.2
>>> On 01.05.13 at 15:20, Julien Grall <julien.grall@linaro.org> wrote: > HYPERVISOR_VIRT_START is an unsigned long constant. In shadow code, Xen > compares this value with a int which is wrong. > > multi.c:1981:5: error: comparison of constant 8796025913344 with expression > of > type ''int'' is always true [-Werror,-Wtautological-constant-out-of-range-compare] > SHADOW_FOREACH_L2E(sl2mfn, sl2e, 0, 0, v->domain, { > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > multi.c:1320:18: note: expanded from macro ''SHADOW_FOREACH_L2E'' > < (HYPERVISOR_VIRT_START >> SHADOW_L2_PAGETABLE_SHIFT) ) \ > ^ > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~Which makes relatively obvious that these checks are there only for 32-bit Xen, which we don''t have anymore. So rather than fixing dead code, why don''t we simply drop it? Jan> Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > xen/arch/x86/mm/shadow/multi.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c > index a8ef75e..f2e70d2 100644 > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -1308,7 +1308,8 @@ do { > \ > /* 32-bit l2 on PAE/64: four pages, touch every second entry, and avoid Xen > */ > #define SHADOW_FOREACH_L2E(_sl2mfn, _sl2e, _gl2p, _done, _dom, _code) \ > do { \ > - int _i, _j, __done = 0; \ > + unsigned long _i, _j; \ > + int __done = 0; \ > int _xen = !shadow_mode_external(_dom); \ > ASSERT(mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_32_shadow);\ > for ( _j = 0; _j < 4 && !__done; _j++ ) \ > @@ -1335,7 +1336,7 @@ do { > \ > /* PAE: if it''s an l2h, don''t touch Xen mappings */ > #define SHADOW_FOREACH_L2E(_sl2mfn, _sl2e, _gl2p, _done, _dom, _code) > \ > do { > \ > - int _i; \ > + unsigned long _i; > \ > int _xen = !shadow_mode_external(_dom); > \ > shadow_l2e_t *_sp = sh_map_domain_page((_sl2mfn)); > \ > ASSERT(mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_pae_shadow \ > -- > 1.7.10.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Tim Deegan
2013-May-02 10:44 UTC
[PATCH] Re: xen/x86: Fix compilation issue with clang 3.2
At 14:20 +0100 on 01 May (1367418011), Julien Grall wrote:> HYPERVISOR_VIRT_START is an unsigned long constant. In shadow code, Xen > compares this value with a int which is wrong.Thanks for the report. In fact the comparison is never going to be true, even with the wider type, since we''re comparing 8796025913344 to a number that''s never larger than 2047. This is a hangover from the 32-bit build, so I''ll just remove it: commit aa2f0f18c7068127520c97b3b2ad9acf039ee7e9 Author: Tim Deegan <tim@xen.org> Date: Thu May 2 11:37:56 2013 +0100 x86/mm/shadow: remove dead code for avoiding Xen entries on 32-bit tables. All non-external-mode (==PV) guests have 4-level pagetables now that the PAE build of Xen is gone. This patch should have no effect, since the condition it removes could never be true anyway: the l2 offset of HYPERVISOR_VIRT_START on 64-bit Xen is much higher than any l2 offset we could have seen in the tables (and indeed bigger than the ''int'' type, which clang was complaining about). Actual compat PV guest xen entries are handled by the equivalent test in the 64-bit SHADOW_FOREACH_L2E() below. Reported-by: Julien Grall <julien.grall@linaro.org> Signed-off-by: Tim Deegan <tim@xen.org> diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c index a8ef75e..e216063 100644 --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -1305,26 +1305,23 @@ do { \ #if GUEST_PAGING_LEVELS == 2 -/* 32-bit l2 on PAE/64: four pages, touch every second entry, and avoid Xen */ +/* 32-bit l2 on PAE/64: four pages, touch every second entry */ #define SHADOW_FOREACH_L2E(_sl2mfn, _sl2e, _gl2p, _done, _dom, _code) \ do { \ int _i, _j, __done = 0; \ - int _xen = !shadow_mode_external(_dom); \ - ASSERT(mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_32_shadow);\ + ASSERT(shadow_mode_external(_dom)); \ + ASSERT(mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_32_shadow); \ for ( _j = 0; _j < 4 && !__done; _j++ ) \ { \ shadow_l2e_t *_sp = sh_map_domain_page(_sl2mfn); \ for ( _i = 0; _i < SHADOW_L2_PAGETABLE_ENTRIES; _i += 2 ) \ - if ( (!(_xen)) \ - || ((_j * SHADOW_L2_PAGETABLE_ENTRIES) + _i) \ - < (HYPERVISOR_VIRT_START >> SHADOW_L2_PAGETABLE_SHIFT) ) \ - { \ - (_sl2e) = _sp + _i; \ - if ( shadow_l2e_get_flags(*(_sl2e)) & _PAGE_PRESENT ) \ - {_code} \ - if ( (__done = (_done)) ) break; \ - increment_ptr_to_guest_entry(_gl2p); \ - } \ + { \ + (_sl2e) = _sp + _i; \ + if ( shadow_l2e_get_flags(*(_sl2e)) & _PAGE_PRESENT ) \ + {_code} \ + if ( (__done = (_done)) ) break; \ + increment_ptr_to_guest_entry(_gl2p); \ + } \ sh_unmap_domain_page(_sp); \ if ( _j < 3 ) _sl2mfn = sh_next_page(_sl2mfn); \ } \ @@ -1332,26 +1329,22 @@ do { \ #elif GUEST_PAGING_LEVELS == 3 -/* PAE: if it''s an l2h, don''t touch Xen mappings */ +/* PAE: touch all entries */ #define SHADOW_FOREACH_L2E(_sl2mfn, _sl2e, _gl2p, _done, _dom, _code) \ do { \ int _i; \ - int _xen = !shadow_mode_external(_dom); \ shadow_l2e_t *_sp = sh_map_domain_page((_sl2mfn)); \ - ASSERT(mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_pae_shadow \ - || mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2h_pae_shadow);\ + ASSERT(shadow_mode_external(_dom)); \ + ASSERT(mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_pae_shadow \ + || mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2h_pae_shadow); \ for ( _i = 0; _i < SHADOW_L2_PAGETABLE_ENTRIES; _i++ ) \ - if ( (!(_xen)) \ - || mfn_to_page(_sl2mfn)->u.sh.type != SH_type_l2h_pae_shadow\ - || ((_i + (3 * SHADOW_L2_PAGETABLE_ENTRIES)) \ - < (HYPERVISOR_VIRT_START >> SHADOW_L2_PAGETABLE_SHIFT)) ) \ - { \ - (_sl2e) = _sp + _i; \ - if ( shadow_l2e_get_flags(*(_sl2e)) & _PAGE_PRESENT ) \ - {_code} \ - if ( _done ) break; \ - increment_ptr_to_guest_entry(_gl2p); \ - } \ + { \ + (_sl2e) = _sp + _i; \ + if ( shadow_l2e_get_flags(*(_sl2e)) & _PAGE_PRESENT ) \ + {_code} \ + if ( _done ) break; \ + increment_ptr_to_guest_entry(_gl2p); \ + } \ sh_unmap_domain_page(_sp); \ } while (0)