Re: KEXEC fix 32/64bit issues with KEXEC_CMD_kexec_get_range

[Andrew Cooper <andrew.cooper3@citrix.com>]

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