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

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

[Mark Cave-Ayland]

On 30/12/10 18:20, Andreas Färber wrote:

Hi Andreas,
> 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.
> 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?

ATB,

Mark.

[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

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

[Mark Cave-Ayland]

On 30/12/10 19:25, Andreas Färber wrote:
> > 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.

Possibly, although I took a glance at the hash table code and couldn't 
immediately get my head around how to come up with the reverse mapping. 
The good news is that these macros are designed for internal use only, 
so they only need to provide valid mappings for the ROM area which 
simplifies things a lot.
> > 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.

Okay - please do. You're probably the nearest person we have to a PPC 
maintainer at the moment so I'm happy to defer to you here.

ATB,

Mark.

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

[Andreas Färber]

Am 30.12.2010 um 21:20 schrieb Mark Cave-Ayland:
> > 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.
>
> Possibly, although I took a glance at the hash table code and  
> couldn't immediately get my head around how to come up with the  
> reverse mapping.

There's a list of translations in ofmem_t that we'd have to iterate  
through, no? :)
The hash tables are platform-specific and possibly incomplete.

Andreas

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

[Mark Cave-Ayland]

On 30/12/10 22:43, Andreas Färber wrote:
> > Possibly, although I took a glance at the hash table code and couldn't
> > immediately get my head around how to come up with the reverse mapping.
>
> There's a list of translations in ofmem_t that we'd have to iterate
> through, no? :)

IIRC the translations list doesn't contain the mappings for the ROM 
itself as the pages are mapped directly during startup. But since 
va2pa() and pa2va() are only used by ofmem_posix_memalign() (i.e. 
internal ofmem_malloc() allocations only), they are guaranteed to lie 
within the address space for the ROM image which makes things easier.

ATB,

Mark.

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

[Andreas Färber]

Am 31.12.2010 um 00:31 schrieb Mark Cave-Ayland:
> On 30/12/10 22:43, Andreas Färber wrote:
> > > Possibly, although I took a glance at the hash table code and  
> > > couldn't
> > > immediately get my head around how to come up with the reverse  
> > > mapping.
> >
> > There's a list of translations in ofmem_t that we'd have to iterate
> > through, no? :)
>
> IIRC the translations list doesn't contain the mappings for the ROM  
> itself as the pages are mapped directly during startup. But since  
> va2pa() and pa2va() are only used by ofmem_posix_memalign() (i.e.  
> internal ofmem_malloc() allocations only), they are guaranteed to  
> lie within the address space for the ROM image which makes things  
> easier.

Only for sparc! For ppc I located it further below, beneath the  
(aligned) hash map iirc.

Obviously we all try to set up sane translations initially, so I was  
more worried about runtime. But on second thoughts, if someone remaps  
our malloc zone with whatever alignment then we're doomed anyway.

I must admit that I still don't understand which case exactly broke  
the alignment on sparc in the first place, so I'm unable to provide  
pinpointed solutions. I'm regarding these as general functions - thus  
a va2pa() that doesn't respect the va translations is broken IMO.

Andreas

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

[Mark Cave-Ayland]

On 30/12/10 23:46, Andreas Färber wrote:
> Only for sparc! For ppc I located it further below, beneath the
> (aligned) hash map iirc.

Ah okay.
> Obviously we all try to set up sane translations initially, so I was
> more worried about runtime. But on second thoughts, if someone remaps
> our malloc zone with whatever alignment then we're doomed anyway.
>
> I must admit that I still don't understand which case exactly broke the
> alignment on sparc in the first place, so I'm unable to provide
> pinpointed solutions. I'm regarding these as general functions - thus a
> va2pa() that doesn't respect the va translations is broken IMO.

The issue is that the old routines just aligned the virtual address, 
whereas the new ones also ensure the physical address is aligned. This 
is required for the memory used for the MMU page tables, for example.

ATB,

Mark.