Mailing List Archive

>>> 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


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

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


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

>>> 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




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