Re: [OpenBIOS] [PATCH 3/6] Introduce va2pa() and pa2va() functions for PPC for subsequent use by OFMEM.
Mark, Am 29.12.2010 um 12:07 schrieb Mark Cave-Ayland:
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@siriusit.co.uk>
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
-- OpenBIOS http://openbios.org/ Mailinglist: http://lists.openbios.org/mailman/listinfo Free your System - May the Forth be with you
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. -- Mark Cave-Ayland - Senior Technical Architect PostgreSQL - PostGIS Sirius Corporation plc - control through freedom http://www.siriusit.co.uk t: +44 870 608 0063 Sirius Labs: http://www.siriusit.co.uk/labs
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
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. -- Mark Cave-Ayland - Senior Technical Architect PostgreSQL - PostGIS Sirius Corporation plc - control through freedom http://www.siriusit.co.uk t: +44 870 608 0063 Sirius Labs: http://www.siriusit.co.uk/labs
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
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. -- Mark Cave-Ayland - Senior Technical Architect PostgreSQL - PostGIS Sirius Corporation plc - control through freedom http://www.siriusit.co.uk t: +44 870 608 0063 Sirius Labs: http://www.siriusit.co.uk/labs
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
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. -- Mark Cave-Ayland - Senior Technical Architect PostgreSQL - PostGIS Sirius Corporation plc - control through freedom http://www.siriusit.co.uk t: +44 870 608 0063 Sirius Labs: http://www.siriusit.co.uk/labs
participants (2)
-
Andreas Färber -
Mark Cave-Ayland