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

[Andreas Färber]

Mark,

Am 29.12.2010 um 12:07 schrieb Mark Cave-Ayland:
> Signed-off-by: Mark Cave-Ayland

This does not really address my comments...
> openbios-devel/arch/ppc/qemu/ofmem.c |   21 +++++++++++++++++++++
> 1 files changed, 21 insertions(+), 0 deletions(-)
>
> diff --git a/openbios-devel/arch/ppc/qemu/ofmem.c b/openbios-devel/ 
> arch/ppc/qemu/ofmem.c
> index c21a112..44b5ef4 100644
> --- a/openbios-devel/arch/ppc/qemu/ofmem.c
> +++ b/openbios-devel/arch/ppc/qemu/ofmem.c
> @@ -180,6 +180,27 @@ void ofmem_arch_create_translation_entry(ucell  
> *transentry, translation_t *t)
> /*	OF private allocations						*/
> / 
> ************************************************************************/
>
> +/* 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;
> }
>
> +}

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
}
> +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.

BR,
Andreas
> void *
> malloc(int size)
> {
> -- 
> 1.7.2.3