[OpenBIOS] [PATCH 3/6] Introduce va2pa() and pa2va() functions for PPC for subsequent use by OFMEM.

[Andreas Färber]

Hi,

Am 30.12.2010 um 20:08 schrieb Mark Cave-Ayland:

> On 30/12/10 18:20, Andreas Färber wrote:
>
>> This does not really address my comments...
>
> Ah okay - I was still hoping that no news was good news and I admit  
> I wanted something in place to unblock the SPARC32 OFMEM patchset.
>
>>> +/* Private functions for mapping between physical/virtual  
>>> addresses */
>>> +inline phys_addr_t
>>> +va2pa(unsigned long va)
>>> +{
>>> + if (va >= OF_CODE_START && va < OF_CODE_START + OF_CODE_SIZE) {
>>
>> The latter will overflow on 32-bit ppc, and (va < 0) == 0 so that  
>> branch
>> would never be taken. Is there no compiler warning?
>>
>>> + return (phys_addr_t)get_rom_base() - OF_CODE_START + va;
>>
>> Again, what's the reason for duplicating this calculation?
>>
>>> + } else {
>>> + return (phys_addr_t)va;
>>> + }
>>> +}
>
> Only because I was in two minds with regards to whether to keep  
> pa2va and va2pa as "symmetrical" as possible. I'm happy to go with  
> whatever you think is best.

Well I did point out that we're missing the opposite direction, so I  
was hoping you would add an ofmem_translate_inverse() or so, so that  
both could almost symmetrically use those functions, not just for ppc.

>> What about the following trivial implementation instead?
>>
>> phys_addr_t va2pa(unsigned long va)
>> {
>> ucell mode;
>> return ea_to_phys(va, &mode); // calls ofmem_translate() internally  
>> for
>> the 'else' case
>> }
>
> Yes - that seems to work in my tests here.
>
>>> +
>>> +inline unsigned long
>>> +pa2va(phys_addr_t pa)
>>> +{
>>> + if ((pa - get_rom_base() + OF_CODE_START >= OF_CODE_START) &&
>>> + (pa - get_rom_base() + OF_CODE_START < OF_CODE_START +  
>>> OF_CODE_SIZE))
>>> + return (unsigned long)pa - get_rom_base() + OF_CODE_START;
>>> + else
>>> + return (unsigned long)pa;
>>> +}
>>
>> Trailing whitespace?
>>
>> Missing braces for QEMU style.
>
> Ooops my bad. Does the attached patch look better?

Mostly, apart from the above issue. If you don't mind, I would apply  
this part myself tomorrow, moving your two functions down to avoid the  
static prototype and without my explanatory C++-style comment.

Andreas