[OpenBIOS] [PATCH 3/3] Don't assume that pointer and cell size are identical

[Blue Swirl]

On Tue, Oct 26, 2010 at 7:14 PM, Andreas Färber <andreas.faerber at web.de> wrote:
> Am 26.10.2010 um 20:53 schrieb Blue Swirl:
>
>> On Mon, Oct 25, 2010 at 9:08 PM, Andreas Färber <andreas.faerber at web.de>
>> wrote:
>>>
>>> Am 20.10.2010 um 21:04 schrieb Blue Swirl:
>>>
>>>> On Wed, Oct 20, 2010 at 6:36 PM, Andreas Färber <andreas.faerber at web.de>
>>>> wrote:
>>>>>
>>>>> Am 20.10.2010 um 18:19 schrieb Blue Swirl:
>>>>>
>>>>>> On Sun, Oct 17, 2010 at 11:18 PM, Andreas Färber
>>>>>> <andreas.faerber at web.de>
>>>>>> wrote:
>>>>>>>
>>>>>>> On ppc64, cell size is 32 bits but pointers are 64-bit. Thus, direct
>>>>>>> casts
>>>>>>> result in warnings, treated as errors.
>>>>>>>
>>>>>>> Use [u]intptr_t cast or cell2pointer and pointer2cell macros as
>>>>>>> necessary.
>>>>>>
>>>>>>> -int uart_init(uint64_t port, unsigned long speed)
>>>>>>> +int uart_init(uintptr_t port, unsigned long speed)
>>>>>>
>>>>>> This is not correct. The physical addresses on Sparc32 are really 36
>>>>>> bits wide, so uintptr_t is not wide enough.
>>>>>
>>>>> Okay, then I'll revert this one. The patch may be mechanical to
>>>>> _review_
>>>>> but
>>>>> it's pretty invasive, so I'd rather have some parts of it committed
>>>>> before
>>>>> running into larger merge conflicts.
>>>>>
>>>>>> For that, we'd need a
>>>>>> physical address type. We could reuse for example QEMU name,
>>>>>> target_phys_addr_t.
>>>>>
>>>>> Sounds good. Should we go ahead with my patch first and do the uint64_t
>>>>> ->
>>>>> target_phys_addr_t change as a follow-up patch? Or should I strip all
>>>>> changes to drivers/ from my patch?
>>>>
>>>> The cell2pointer etc. changes look OK.
>>>
>>> Thanks, applied in r922, with one additional (uintptr_t) ->
>>> cell2pointer()
>>> fix in mac-parts.c that I forgot to mention.
>>>
>>> Attached is what I have not applied from the original patch. I'm thinking
>>> we
>>> should better avoid the native-bitwidth-equals-host-bitwidth path on
>>> ppc64
>>> than fixing the symptoms of using it. For the rest, target_phys_addr_t
>>> would
>>> seem a nice solution. Additionally, the double-dereference of "myself"
>>> and
>>> the trampoline issue still need proper fixes.
>>
>> In some cases, instead of casts, the structure fields (in adb_kbd.c:
>> dev->state) should be widened to uintptr_t or pointer.
>
> I was rather thinking of phys_addr_t.

That should be used for physical addresses. But if the address is
"virtual" (for example a pointer to a structure, like in the adb_kbd.c
case), then a pointer type would be better. There's in those cases an
assumption that the addressed data can be accessed via pointer
dereference, so CPU word length is enough.

In other places the cast just extends the address (from 32 to 64
bits), but the upper bits have been lost already earlier. Then the
structures should be changed (to target_phys_addr_t) to contain a full
physical address.