Re: [Xen-devel] KEXEC fix 32/64bit issues with KEXEC_CMD_kexec_get_range

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.12.11 at 16:25, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> On 14/12/11 15:08, Jan Beulich wrote:
>>>>> On 14.12.11 at 15:02, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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 




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