Andrew Cooper
2011-Dec-14 14:02 UTC
KEXEC fix 32/64bit issues with KEXEC_CMD_kexec_get_range
Attached is my first attempt at this fix. It has been compile tested
for x86_{64,32} and so far, dev tested in so much as it does not break
backwards compatibility for 32bit dom0 on 64bit Xen. It is untested as
far as ia64 goes, but I believe it should be correct from code inspection.
--
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jan Beulich
2011-Dec-14 15:08 UTC
Re: KEXEC fix 32/64bit issues with KEXEC_CMD_kexec_get_range
>>> On 14.12.11 at 15:02, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >To fix 32bit Xen which uses 32bit intergers for addresses and sizes, >change the internals to use xen_kexec64_range_t which will use 64bit >integers instead. This also invovles changing several casts to >explicitly use uint64_ts rather than unsigned longs.I don''t think fixing 32-bit Xen is really necessary: Neither does anyone care much, nor should any address be beyond 4Gb in that case. Not playing with this will likely simplify the patch quite a bit.>--- a/xen/arch/ia64/xen/machine_kexec.c >+++ b/xen/arch/ia64/xen/machine_kexec.c >@@ -102,10 +102,10 @@ void machine_reboot_kexec(xen_kexec_imag > machine_kexec(image); > } > >-int machine_kexec_get_xen(xen_kexec_range_t *range) >+int machine_kexec_get_xen(xen_kexec64_range_t *range) > { > range->start = ia64_tpa(_text); >- range->size = (unsigned long)_end - (unsigned long)_text; >+ range->size = (uint64_t)_end - (uint64_t)_text;This is bogus and pointless (same thing a few lines down the patch).> return 0; > } > >--- a/xen/arch/x86/x86_32/machine_kexec.c >+++ b/xen/arch/x86/x86_32/machine_kexec.c >@@ -11,11 +11,11 @@ > #include <asm/page.h> > #include <public/kexec.h> > >-int machine_kexec_get_xen(xen_kexec_range_t *range) >+int machine_kexec_get_xen(xen_kexec64_range_t *range) > { > range->start = virt_to_maddr(_start); >- range->size = (unsigned long)xenheap_phys_end - >- (unsigned long)range->start; >+ range->size = (uint64_t)xenheap_phys_end -And here it''s even wrong, and I doubt it compiles without warning across the supported range of compilers.>+ (uint64_t)range->start;Casting range->start here and elsewhere shouldn''t be necessary at all (the pre-existing cast was bogus too).> return 0; > } >Jan
Andrew Cooper
2011-Dec-14 15:25 UTC
Re: KEXEC fix 32/64bit issues with KEXEC_CMD_kexec_get_range
On 14/12/11 15:08, Jan Beulich wrote:>>>> On 14.12.11 at 15:02, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> To fix 32bit Xen which uses 32bit intergers for addresses and sizes, >> change the internals to use xen_kexec64_range_t which will use 64bit >> integers instead. This also invovles changing several casts to >> explicitly use uint64_ts rather than unsigned longs. > I don''t think fixing 32-bit Xen is really necessary: Neither does anyone > care much, nor should any address be beyond 4Gb in that case. Not > playing with this will likely simplify the patch quite a bit.This point was discussed on the IRC channel and it was decided to be worth doing, even though people are likely not to care else I would happily collapse the patch somewhat. Why should nothing be beyond 4GB in the 32bit case? Anything with PAE support ought to be able to use 64GB or less.>> --- a/xen/arch/ia64/xen/machine_kexec.c >> +++ b/xen/arch/ia64/xen/machine_kexec.c >> @@ -102,10 +102,10 @@ void machine_reboot_kexec(xen_kexec_imag >> machine_kexec(image); >> } >> >> -int machine_kexec_get_xen(xen_kexec_range_t *range) >> +int machine_kexec_get_xen(xen_kexec64_range_t *range) >> { >> range->start = ia64_tpa(_text); >> - range->size = (unsigned long)_end - (unsigned long)_text; >> + range->size = (uint64_t)_end - (uint64_t)_text; > This is bogus and pointless (same thing a few lines down the patch).I can understand pointless as sizeof(unsigned long) == sizeof(uint64_t) on 64bit builds, but why is it bogus? I changed it for consistency with xen_kexec64_range_t.>> return 0; >> } >> >> --- a/xen/arch/x86/x86_32/machine_kexec.c >> +++ b/xen/arch/x86/x86_32/machine_kexec.c >> @@ -11,11 +11,11 @@ >> #include <asm/page.h> >> #include <public/kexec.h> >> >> -int machine_kexec_get_xen(xen_kexec_range_t *range) >> +int machine_kexec_get_xen(xen_kexec64_range_t *range) >> { >> range->start = virt_to_maddr(_start); >> - range->size = (unsigned long)xenheap_phys_end - >> - (unsigned long)range->start; >> + range->size = (uint64_t)xenheap_phys_end - > And here it''s even wrong, and I doubt it compiles without warning > across the supported range of compilers.Why might there be warnings in this case? At the worst, all it is doing is explicitly promoting a 32bit integer to a 64bit.>> + (uint64_t)range->start; > Casting range->start here and elsewhere shouldn''t be necessary at > all (the pre-existing cast was bogus too).Agreed, but same comment regarding consistency, with a mix of not thinking about the implication on my behalf.>> return 0; >> } >> > Jan >-- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com
Jan Beulich
2011-Dec-14 15:32 UTC
Re: KEXEC fix 32/64bit issues with KEXEC_CMD_kexec_get_range
>>> On 14.12.11 at 16:25, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 14/12/11 15:08, Jan Beulich wrote: >>>>> On 14.12.11 at 15:02, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> To fix 32bit Xen which uses 32bit intergers for addresses and sizes, >>> change the internals to use xen_kexec64_range_t which will use 64bit >>> integers instead. This also invovles changing several casts to >>> explicitly use uint64_ts rather than unsigned longs. >> I don''t think fixing 32-bit Xen is really necessary: Neither does anyone >> care much, nor should any address be beyond 4Gb in that case. Not >> playing with this will likely simplify the patch quite a bit. > > This point was discussed on the IRC channel and it was decided to be > worth doing, even though people are likely not to care else I would > happily collapse the patch somewhat. Why should nothing be beyond 4GB > in the 32bit case? Anything with PAE support ought to be able to use > 64GB or less. > >>> --- a/xen/arch/ia64/xen/machine_kexec.c >>> +++ b/xen/arch/ia64/xen/machine_kexec.c >>> @@ -102,10 +102,10 @@ void machine_reboot_kexec(xen_kexec_imag >>> machine_kexec(image); >>> } >>> >>> -int machine_kexec_get_xen(xen_kexec_range_t *range) >>> +int machine_kexec_get_xen(xen_kexec64_range_t *range) >>> { >>> range->start = ia64_tpa(_text); >>> - range->size = (unsigned long)_end - (unsigned long)_text; >>> + range->size = (uint64_t)_end - (uint64_t)_text; >> This is bogus and pointless (same thing a few lines down the patch). > > I can understand pointless as sizeof(unsigned long) == sizeof(uint64_t) > on 64bit builds, but why is it bogus? I changed it for consistency with > xen_kexec64_range_t.Because of the casting of pointers to other than unsigned long.>>> return 0; >>> } >>> >>> --- a/xen/arch/x86/x86_32/machine_kexec.c >>> +++ b/xen/arch/x86/x86_32/machine_kexec.c >>> @@ -11,11 +11,11 @@ >>> #include <asm/page.h> >>> #include <public/kexec.h> >>> >>> -int machine_kexec_get_xen(xen_kexec_range_t *range) >>> +int machine_kexec_get_xen(xen_kexec64_range_t *range) >>> { >>> range->start = virt_to_maddr(_start); >>> - range->size = (unsigned long)xenheap_phys_end - >>> - (unsigned long)range->start; >>> + range->size = (uint64_t)xenheap_phys_end - >> And here it''s even wrong, and I doubt it compiles without warning >> across the supported range of compilers. > > Why might there be warnings in this case? At the worst, all it is doing > is explicitly promoting a 32bit integer to a 64bit.Oh, sorry, I somehow thought xenheap_phys_end would be a linker generated address symbol (mixed it with the various section end ones). But then - please just remove the casts (and perhaps as a general cleanup outside of this patch). Jan>>> + (uint64_t)range->start; >> Casting range->start here and elsewhere shouldn''t be necessary at >> all (the pre-existing cast was bogus too). > > Agreed, but same comment regarding consistency, with a mix of not > thinking about the implication on my behalf. > >>> return 0; >>> } >>> >> Jan >> > > -- > Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer > T: +44 (0)1223 225 900, http://www.citrix.com