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

[Blue Swirl]

On Tue, Oct 26, 2010 at 8:04 PM, Andreas Färber <andreas.faerber at web.de> wrote:
> Am 26.10.2010 um 21:26 schrieb 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.
>
> You're right, I mixed it up with cuda where phys_addr_t is necessary for
> ->base.
>
> Only QEMU needs to distinguish between host and target though, so I don't
> think target_ makes sense here. Attached FYI.

Looks OK. For x86, there's PAE mode with 36 bits but that's probably
not supported by OpenBIOS or QEMU.