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

[Andreas Färber]

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.

Andreas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Introduce-physical-address-type.patch
Type: application/octet-stream
Size: 2882 bytes
Desc: not available
URL: <http://lists.openbios.org/pipermail/openbios/attachments/20101026/236cc24f/attachment.obj>
-------------- next part --------------