From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: KEXEC fix 32/64bit issues with KEXEC_CMD_kexec_get_range Date: Wed, 14 Dec 2011 15:25:13 +0000 Message-ID: <4EE8BFD9.3080208@citrix.com> References: <4EE8AC5A.4050205@citrix.com> <4EE8CA030200007800067C6A@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4EE8CA030200007800067C6A@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jan Beulich Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 14/12/11 15:08, Jan Beulich wrote: >>>> On 14.12.11 at 15:02, Andrew Cooper 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 >> #include >> >> -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