Re: [OpenBIOS] [PATCH 3/6] Introduce va2pa() and pa2va() functions for PPC for subsequent use by OFMEM.
7:20 p.m.
Mark,
Am 29.12.2010 um 12:07 schrieb Mark Cave-Ayland:
Signed-off-by: Mark Cave-Ayland
<mark.cave-ayland(a)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
8:08 p.m.
New subject: [PATCH 3/6] Introduce va2pa() and pa2va() functions for PPC for subsequent use by OFMEM.
On 30/12/10 18:20, Andreas Färber wrote:
Hi Andreas,
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.
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
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;
> + }
> +} 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.
8:25 p.m.
New subject: [PATCH 3/6] Introduce va2pa() and pa2va() functions for PPC for subsequent use by OFMEM.
Hi,
Am 30.12.2010 um 20:08 schrieb Mark Cave-Ayland:
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 18:20, 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.
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;
> + }
> +} 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.
9:20 p.m.
New subject: [PATCH 3/6] Introduce va2pa() and pa2va() functions for PPC for subsequent use by OFMEM.
On 30/12/10 19:25, 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.
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.
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
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. 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.
11:43 p.m.
New subject: [PATCH 3/6] Introduce va2pa() and pa2va() functions for PPC for subsequent use by OFMEM.
Am 30.12.2010 um 21:20 schrieb Mark Cave-Ayland:
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
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.
12:31 a.m.
New subject: [PATCH 3/6] Introduce va2pa() and pa2va() functions for PPC for subsequent use by OFMEM.
On 30/12/10 22:43, Andreas Färber wrote:
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
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? :)
12:46 a.m.
New subject: [PATCH 3/6] Introduce va2pa() and pa2va() functions for PPC for subsequent use by OFMEM.
Am 31.12.2010 um 00:31 schrieb Mark Cave-Ayland:
On 30/12/10 22:43, Andreas Färber wrote:
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
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? :)
2:09 p.m.
New subject: [PATCH 3/6] Introduce va2pa() and pa2va() functions for PPC for subsequent use by OFMEM.
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
3244
days inactive
3246
days old
7 comments
2 participants
participants (2)
-
Andreas Färber -
Mark Cave-Ayland