[PATCH] RFC: Change ofmem_common.c to set memory translation properties by reference

[Mark Cave-Ayland]

Hi folks,

The attached patch changes OFMEM so that instead of allocating new space 
within the Forth dictionary every time the /memory and /virtual-memory 
available/translations nodes are updated, we simply change the property 
to point directly to a static buffer. This has the effect of saving 
substantial amounts of memory during OpenSolaris 10 boot (in fact the 
final dictionary size after boot is now < 256K once again).

Blue/Andreas: please could you take a look at this patch and make sure 
it doesn't break anything in your SPARC64/PPC tests?

On the plus side, with this patch applied Milax gets to the end of its 
natural boot without crashing giving the following output:


OpenBIOS for Sparc64
Configuration device id QEMU version 1 machine id 0
kernel cmdline
CPUs: 1 x SUNW,UltraSPARC-IIi
UUID: 00000000-0000-0000-0000-000000000000
Welcome to OpenBIOS v1.0 built on Oct 14 2010 20:18
   Type 'help' for detailed information
Trying cdrom:f...
Not a bootable ELF image
Not a bootable a.out image

Loading FCode image...
Loaded 7084 bytes
entry point is 0x4000
Ignoring failed claim for va 1000000 memsz bf34e!
Ignoring failed claim for va 1402000 memsz 303b3!
Ignoring failed claim for va 1800000 memsz 60a30!

Jumping to entry point 00000000010071d8 for type 0000000000000001...
switching to new context: entry point 0x10071d8 stack 0x00000000ffe06b49
warning:interpret: exception -13 caught
SunOS Release 5.11 Version MilaX_0.3.2 64-bit
Copyright 1983-2008 Sun Microsystems, Inc.  All rights reserved.
Use is subject to license terms.
spacex@:interpret: exception -13 caught
kdbg-words:interpret: exception -13 caught
cb-r/w:interpret: exception -13 caught
(Can't load tod module) EXIT
-1 >


Does anyone know what the tod modules does? Is it Time Of Day (i.e. we 
are missing some kind of hardware clock emulation?)


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: [phone]

Sirius Labs: http://www.siriusit.co.uk/labs

[Artyom Tarasenko]

On Thu, Oct 14, 2010 at 10:32 PM, Mark Cave-Ayland
<mark.cave-ayland(a)siriusit.co.uk&gt; wrote:
 Hi folks,

 The attached patch changes OFMEM so that instead of allocating new space
 within the Forth dictionary every time the /memory and /virtual-memory
 available/translations nodes are updated, we simply change the property to
 point directly to a static buffer. This has the effect of saving substantial
 amounts of memory during OpenSolaris 10 boot (in fact the final dictionary
 size after boot is now < 256K once again).

 Blue/Andreas: please could you take a look at this patch and make sure it
 doesn't break anything in your SPARC64/PPC tests?

 On the plus side, with this patch applied Milax gets to the end of its
 natural boot without crashing giving the following output:


 OpenBIOS for Sparc64
 Configuration device id QEMU version 1 machine id 0
 kernel cmdline
 CPUs: 1 x SUNW,UltraSPARC-IIi
 UUID: 00000000-0000-0000-0000-000000000000
 Welcome to OpenBIOS v1.0 built on Oct 14 2010 20:18
  Type 'help' for detailed information
 Trying cdrom:f...
 Not a bootable ELF image
 Not a bootable a.out image

 Loading FCode image...
 Loaded 7084 bytes
 entry point is 0x4000
 Ignoring failed claim for va 1000000 memsz bf34e!
 Ignoring failed claim for va 1402000 memsz 303b3!
 Ignoring failed claim for va 1800000 memsz 60a30!

 Jumping to entry point 00000000010071d8 for type 0000000000000001...
 switching to new context: entry point 0x10071d8 stack 0x00000000ffe06b49
 warning:interpret: exception -13 caught
 SunOS Release 5.11 Version MilaX_0.3.2 64-bit
 Copyright 1983-2008 Sun Microsystems, Inc.  All rights reserved.
 Use is subject to license terms.
 spacex@:interpret: exception -13 caught
 kdbg-words:interpret: exception -13 caught
 cb-r/w:interpret: exception -13 caught
 (Can't load tod module) EXIT
 -1 >
Cool! Congrats!

 Does anyone know what the tod modules does? Is it Time Of Day (i.e.
we are
 missing some kind of hardware clock emulation?)
I think so. Actually m48t59 is emulated in sparc32, but not connected
in case of sparc64.

-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/

[Artyom Tarasenko]

On Thu, Oct 14, 2010 at 10:46 PM, Artyom Tarasenko <atar4qemu(a)gmail.com&gt; wrote:
 On Thu, Oct 14, 2010 at 10:32 PM, Mark Cave-Ayland
 <mark.cave-ayland(a)siriusit.co.uk&gt; wrote:
> Hi folks,
>
> The attached patch changes OFMEM so that instead of allocating new space
> within the Forth dictionary every time the /memory and /virtual-memory
> available/translations nodes are updated, we simply change the property to
> point directly to a static buffer. This has the effect of saving substantial
> amounts of memory during OpenSolaris 10 boot (in fact the final dictionary
> size after boot is now < 256K once again).
>
> Blue/Andreas: please could you take a look at this patch and make sure it
> doesn't break anything in your SPARC64/PPC tests?
>
> On the plus side, with this patch applied Milax gets to the end of its
> natural boot without crashing giving the following output:
>
>
> OpenBIOS for Sparc64
> Configuration device id QEMU version 1 machine id 0
> kernel cmdline
> CPUs: 1 x SUNW,UltraSPARC-IIi
> UUID: 00000000-0000-0000-0000-000000000000
> Welcome to OpenBIOS v1.0 built on Oct 14 2010 20:18
>  Type 'help' for detailed information
> Trying cdrom:f...
> Not a bootable ELF image
> Not a bootable a.out image
>
> Loading FCode image...
> Loaded 7084 bytes
> entry point is 0x4000
> Ignoring failed claim for va 1000000 memsz bf34e!
> Ignoring failed claim for va 1402000 memsz 303b3!
> Ignoring failed claim for va 1800000 memsz 60a30!
>
> Jumping to entry point 00000000010071d8 for type 0000000000000001...
> switching to new context: entry point 0x10071d8 stack 0x00000000ffe06b49
> warning:interpret: exception -13 caught
> SunOS Release 5.11 Version MilaX_0.3.2 64-bit
> Copyright 1983-2008 Sun Microsystems, Inc.  All rights reserved.
> Use is subject to license terms.
> spacex@:interpret: exception -13 caught
> kdbg-words:interpret: exception -13 caught
> cb-r/w:interpret: exception -13 caught
> (Can't load tod module) EXIT
> -1 >

 Cool! Congrats!

> Does anyone know what the tod modules does? Is it Time Of Day (i.e. we are
> missing some kind of hardware clock emulation?)

 I think so. Actually m48t59 is emulated in sparc32, but not connected
 in case of sparc64.
The referencing comment in the opensolaris source:
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/uts/sun4u/o...

* Appropriate tod module will be dynamically selected while booting
* based on finding a device tree node with a "device_type" property value
* of "tod". If such a node describing tod is not found, for backward
* compatibility, a node with a "name" property value of "eeprom" and
* "model" property value of "mk48t59" will be used. Failing to find a
* node matching either of the above criteria will result in no tod module
* being selected; this will cause the boot process to halt

On the real U30 machine:

ok cd /pci@1f,4000/ebus@1/eeprom@14,0
ok .properties
address                  fffa4000
reg                      00000014 00000000 00002000
model                    mk48t59
name                     eeprom
ok

-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/

[Mark Cave-Ayland]

Artyom Tarasenko wrote:

> I think so. Actually m48t59 is emulated in sparc32, but not
connected
> in case of sparc64.
 
 The referencing comment in the opensolaris source:

http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/uts/sun4u/o...
 
 * Appropriate tod module will be dynamically selected while booting
 * based on finding a device tree node with a "device_type" property value
 * of "tod". If such a node describing tod is not found, for backward
 * compatibility, a node with a "name" property value of "eeprom" and
 * "model" property value of "mk48t59" will be used. Failing to find
a
 * node matching either of the above criteria will result in no tod module
 * being selected; this will cause the boot process to halt
 
 On the real U30 machine:
 
 ok cd /pci@1f,4000/ebus@1/eeprom@14,0
 ok .properties
 address                  fffa4000
 reg                      00000014 00000000 00002000
 model                    mk48t59
 name                     eeprom
 ok
Oh, that's interesting. If I look in drivers/obio.c then I can see an 
eeprom device named "mk48t08" rather than "mk48t59". Is this a typo,
or 
does it need a new explicit device defined in pci.c's ebus_config_cb() 
somewhere?

If this is the case, it appears that qemu-system-sparc64 is set to use 
mc146818rtc rather m48t59 and so there is going to have to be a qemu 
build patch required here too.


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: [phone]

Sirius Labs: http://www.siriusit.co.uk/labs

[Blue Swirl]

On Fri, Oct 15, 2010 at 1:43 PM, Mark Cave-Ayland
<mark.cave-ayland(a)siriusit.co.uk&gt; wrote:
 Artyom Tarasenko wrote:

>> I think so. Actually m48t59 is emulated in sparc32, but not connected
>> in case of sparc64.
>
> The referencing comment in the opensolaris source:
>
>
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/uts/sun4u/o...
>
> * Appropriate tod module will be dynamically selected while booting
> * based on finding a device tree node with a "device_type" property value
> * of "tod". If such a node describing tod is not found, for backward
> * compatibility, a node with a "name" property value of "eeprom"
and
> * "model" property value of "mk48t59" will be used. Failing to
find a
> * node matching either of the above criteria will result in no tod module
> * being selected; this will cause the boot process to halt
>
> On the real U30 machine:
>
> ok cd /pci@1f,4000/ebus@1/eeprom@14,0
> ok .properties
> address                  fffa4000
> reg                      00000014 00000000 00002000
> model                    mk48t59
> name                     eeprom
> ok

 Oh, that's interesting. If I look in drivers/obio.c then I can see an eeprom
 device named "mk48t08" rather than "mk48t59". Is this a typo, or does
it
 need a new explicit device defined in pci.c's ebus_config_cb() somewhere?
mk48t59 is a newer version of mk48t08, there is alarm etc.

 If this is the case, it appears that qemu-system-sparc64 is set to
use
 mc146818rtc rather m48t59 and so there is going to have to be a qemu build
 patch required here too.
Actually, while mc146818rtc is linked in, it's not used (there are
some machines that have similar device, so that's why it still hangs
around). m48t59 is on ebus.

Maybe it's time to pick a specific real machine type instead of 'Sun4u
platform'. Something that is not too far from what we have now, with
PC-like hardware. Is Netra-T1 OK?

[Artyom Tarasenko]

On Fri, Oct 15, 2010 at 7:57 PM, Blue Swirl <blauwirbel(a)gmail.com&gt; wrote:
 Maybe it's time to pick a specific real machine type instead of
'Sun4u
 platform'. Something that is not too far from what we have now, with
 PC-like hardware. Is Netra-T1 OK?
Just curious, why not U5, U10 or U30?

-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/

[Blue Swirl]

On Sat, Oct 16, 2010 at 7:09 PM, Artyom Tarasenko <atar4qemu(a)gmail.com&gt; wrote:
 On Fri, Oct 15, 2010 at 7:57 PM, Blue Swirl
<blauwirbel(a)gmail.com&gt; wrote:
> Maybe it's time to pick a specific real machine type instead of 'Sun4u
> platform'. Something that is not too far from what we have now, with
> PC-like hardware. Is Netra-T1 OK?

 Just curious, why not U5, U10 or U30?
U5 could be indeed better, since there's a display (ATYFB). I don't
have a device tree for Ultra 10. U30 has a Psycho bridge instead of
Sabre and FFB display. Otherwise all four look pretty similar.

[Mark Cave-Ayland]

Blue Swirl wrote:

> If this is the case, it appears that qemu-system-sparc64 is set
to use
> mc146818rtc rather m48t59 and so there is going to have to be a qemu build
> patch required here too.
 
 Actually, while mc146818rtc is linked in, it's not used (there are
 some machines that have similar device, so that's why it still hangs
 around). m48t59 is on ebus.
 
 Maybe it's time to pick a specific real machine type instead of 'Sun4u
 platform'. Something that is not too far from what we have now, with
 PC-like hardware. Is Netra-T1 OK?
To be honest, I'm not really bothered as long as Solaris 9 and above 
will work, and I can migrate an existing drive image from physical to 
virtual (though this wouldn't necessarily have to be with the same drivers).


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: [phone]

Sirius Labs: http://www.siriusit.co.uk/labs

[Artyom Tarasenko]

On Fri, Oct 15, 2010 at 3:43 PM, Mark Cave-Ayland
<mark.cave-ayland(a)siriusit.co.uk&gt; wrote:
 Artyom Tarasenko wrote:

>> I think so. Actually m48t59 is emulated in sparc32, but not connected
>> in case of sparc64.
>
> The referencing comment in the opensolaris source:
>
>
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/uts/sun4u/o...
>
> * Appropriate tod module will be dynamically selected while booting
> * based on finding a device tree node with a "device_type" property value
> * of "tod". If such a node describing tod is not found, for backward
> * compatibility, a node with a "name" property value of "eeprom"
and
> * "model" property value of "mk48t59" will be used. Failing to
find a
> * node matching either of the above criteria will result in no tod module
> * being selected; this will cause the boot process to halt
>
> On the real U30 machine:
>
> ok cd /pci@1f,4000/ebus@1/eeprom@14,0
> ok .properties
> address                  fffa4000
> reg                      00000014 00000000 00002000
> model                    mk48t59
> name                     eeprom
> ok

 Oh, that's interesting. If I look in drivers/obio.c then I can see an eeprom
 device named "mk48t08" rather than "mk48t59". Is this a typo, or does
it
 need a new explicit device defined in pci.c's ebus_config_cb() somewhere?
Have you meanwhile tried to change the name to "mk48t59" ? I think
changing the chip in qemu is not neccessary (and even if it is the
name in OpenBIOS has to be changed anyway).


-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/

[Mark Cave-Ayland]

On 04/05/11 08:57, Artyom Tarasenko wrote:

>>> I think so. Actually m48t59 is emulated in sparc32, but
not connected
>>> in case of sparc64.
>>
>> The referencing comment in the opensolaris source:
>>
>>
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/uts/sun4u/o...
>>
>> * Appropriate tod module will be dynamically selected while booting
>> * based on finding a device tree node with a "device_type" property
value
>> * of "tod". If such a node describing tod is not found, for backward
>> * compatibility, a node with a "name" property value of
"eeprom" and
>> * "model" property value of "mk48t59" will be used. Failing
to find a
>> * node matching either of the above criteria will result in no tod module
>> * being selected; this will cause the boot process to halt
>>
>> On the real U30 machine:
>>
>> ok cd /pci@1f,4000/ebus@1/eeprom@14,0
>> ok .properties
>> address                  fffa4000
>> reg                      00000014 00000000 00002000
>> model                    mk48t59
>> name                     eeprom
>> ok
>
> Oh, that's interesting. If I look in drivers/obio.c then I can see an eeprom
> device named "mk48t08" rather than "mk48t59". Is this a typo, or
does it
> need a new explicit device defined in pci.c's ebus_config_cb() somewhere?

 Have you meanwhile tried to change the name to "mk48t59" ? I think
 changing the chip in qemu is not neccessary (and even if it is the
 name in OpenBIOS has to be changed anyway).
If Blue's subsequent comment about it already being wired up to the ebus 
is correct, then yes, probably renaming might work if the device 
registers are mapped correctly. Alas this was the last time I touched 
SPARC64 before switching to SPARC32, and so I never really looked into this.


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: [phone]

Sirius Labs: http://www.siriusit.co.uk/labs

[Artyom Tarasenko]

On Wed, May 4, 2011 at 11:19 AM, Mark Cave-Ayland
<mark.cave-ayland(a)siriusit.co.uk&gt; wrote:
 On 04/05/11 08:57, Artyom Tarasenko wrote:

>>>> I think so. Actually m48t59 is emulated in sparc32, but not connected
>>>> in case of sparc64.
>>>
>>> The referencing comment in the opensolaris source:
>>>
>>>
>>>
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/uts/sun4u/o...
>>>
>>> * Appropriate tod module will be dynamically selected while booting
>>> * based on finding a device tree node with a "device_type"
property
>>> value
>>> * of "tod". If such a node describing tod is not found, for
backward
>>> * compatibility, a node with a "name" property value of
"eeprom" and
>>> * "model" property value of "mk48t59" will be used.
Failing to find a
>>> * node matching either of the above criteria will result in no tod
>>> module
>>> * being selected; this will cause the boot process to halt
>>>
>>> On the real U30 machine:
>>>
>>> ok cd /pci@1f,4000/ebus@1/eeprom@14,0
>>> ok .properties
>>> address                  fffa4000
>>> reg                      00000014 00000000 00002000
>>> model                    mk48t59
>>> name                     eeprom
>>> ok
>>
>> Oh, that's interesting. If I look in drivers/obio.c then I can see an
>> eeprom
>> device named "mk48t08" rather than "mk48t59". Is this a typo,
or does it
>> need a new explicit device defined in pci.c's ebus_config_cb() somewhere?
>
> Have you meanwhile tried to change the name to "mk48t59" ? I think
> changing the chip in qemu is not neccessary (and even if it is the
> name in OpenBIOS has to be changed anyway).

 If Blue's subsequent comment about it already being wired up to the ebus is
 correct, then yes, probably renaming might work if the device registers are
 mapped correctly.
Yes, Blue is right (and I've managed to correct my wrong statement one
day before he did),
the device is connected via ebus.

 Alas this was the last time I touched SPARC64 before
 switching to SPARC32, and so I never really looked into this.


 ATB,

 Mark.

 --
 Mark Cave-Ayland - Senior Technical Architect
 PostgreSQL - PostGIS
 Sirius Corporation plc - control through freedom
 http://www.siriusit.co.uk
 t: [phone]

 Sirius Labs: http://www.siriusit.co.uk/labs


-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/

[Artyom Tarasenko]

On Thu, Oct 14, 2010 at 10:46 PM, Artyom Tarasenko <atar4qemu(a)gmail.com&gt; wrote:
 Actually m48t59 is emulated in sparc32, but not connected
 in case of sparc64.
Wrong. It is connected to isa (ebus), nevermind.

-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/

[PATCH] RFC: Change ofmem_common.c to set memory translation properties by reference

[Andreas Färber]

Mark,

Am 14.10.2010 um 22:32 schrieb Mark Cave-Ayland:

 The attached patch changes OFMEM so that instead of allocating new  
 space within the Forth dictionary every time the /memory and / 
 virtual-memory available/translations nodes are updated, we simply  
 change the property to point directly to a static buffer. This has  
 the effect of saving substantial amounts of memory during  
 OpenSolaris 10 boot (in fact the final dictionary size after boot is  
 now < 256K once again).

 Blue/Andreas: please could you take a look at this patch and make  
 sure it doesn't break anything in your SPARC64/PPC tests?
It looks as if none of us actually tested or ack'ed it yet, all  
answers were about sparc progress and devices only...
Me for one couldn't save your inline patch and my mailer is known to  
cause whitespace damage, and git-am wasn't able to handle the mbox  
file since your SVN diff is missing the a/ directory level expected by  
Git.

If you're asking for comments from us, please be more patient. I'm  
still playing with Blue's slightly older libgcc patch, and some  
trivial ones of mine (e.g., CONFIG_RTAS or wrong return) are awaiting  
review/committing longer than yours. Please at least let us know in  
advance how long you're gonna wait, so that we have a chance to send  
in incomplete review comments even if still untested.

I like the general optimization idea, but I found one typo s/fix/fit/  
and I was wondering whether the property setting function really needs  
to live inside ofmem_common.c or whether we might want to make  
set_property_nocopy() out of it for general use.
I also had some style questions related to the inconsistent use of  
spacing inside braces. Some ppc code even has a wild mix of tabs and 8  
spaces, which is unfortunate when many other projects including QEMU  
use an indentation of 4. We're lacking a CODING_STYLE document imo, to  
look up which way it's supposed to be.

BR,
Andreas

P.S. I have local patches to prepare ofmem_release() inside  
ofmem_common.c.

[Blue Swirl]

On Sat, Oct 16, 2010 at 3:18 PM, Andreas Färber <andreas.faerber(a)web.de&gt; wrote:
 Mark,

 Am 14.10.2010 um 22:32 schrieb Mark Cave-Ayland:

> The attached patch changes OFMEM so that instead of allocating new space
> within the Forth dictionary every time the /memory and /virtual-memory
> available/translations nodes are updated, we simply change the property to
> point directly to a static buffer. This has the effect of saving substantial
> amounts of memory during OpenSolaris 10 boot (in fact the final dictionary
> size after boot is now < 256K once again).
>
> Blue/Andreas: please could you take a look at this patch and make sure it
> doesn't break anything in your SPARC64/PPC tests?

 It looks as if none of us actually tested or ack'ed it yet, all answers were
 about sparc progress and devices only...
 Me for one couldn't save your inline patch and my mailer is known to cause
 whitespace damage, and git-am wasn't able to handle the mbox file since your
 SVN diff is missing the a/ directory level expected by Git.

 If you're asking for comments from us, please be more patient. I'm still
 playing with Blue's slightly older libgcc patch, and some trivial ones of
 mine (e.g., CONFIG_RTAS or wrong return) are awaiting review/committing
 longer than yours. Please at least let us know in advance how long you're
 gonna wait, so that we have a chance to send in incomplete review comments
 even if still untested.
I'll give them a spin.

 I like the general optimization idea, but I found one typo s/fix/fit/
and I
 was wondering whether the property setting function really needs to live
 inside ofmem_common.c or whether we might want to make set_property_nocopy()
 out of it for general use.
 I also had some style questions related to the inconsistent use of spacing
 inside braces. Some ppc code even has a wild mix of tabs and 8 spaces, which
 is unfortunate when many other projects including QEMU use an indentation of
 4. We're lacking a CODING_STYLE document imo, to look up which way it's
 supposed to be.
I'd vote for QEMU style because of common developer base and even
small amount of shared code.

[Mark Cave-Ayland]

Andreas Färber wrote:

> Blue/Andreas: please could you take a look at this patch and make
sure 
> it doesn't break anything in your SPARC64/PPC tests?
 
 It looks as if none of us actually tested or ack'ed it yet, all answers 
 were about sparc progress and devices only...
 Me for one couldn't save your inline patch and my mailer is known to 
 cause whitespace damage, and git-am wasn't able to handle the mbox file 
 since your SVN diff is missing the a/ directory level expected by Git.
 
 If you're asking for comments from us, please be more patient. I'm still 
 playing with Blue's slightly older libgcc patch, and some trivial ones 
 of mine (e.g., CONFIG_RTAS or wrong return) are awaiting 
 review/committing longer than yours. Please at least let us know in 
 advance how long you're gonna wait, so that we have a chance to send in 
 incomplete review comments even if still untested.
I feel your pain when it comes to having multiple patchsets on the go at 
once. For a while, I ended up with about 3 simultaneous sets working 
various things that had bugs, which when I fixed had other other bugs...

In terms of the patch comments, maybe I could have waited longer. 
Generally the OpenBIOS code is still being actively developed, and so 
the few people actively working on the code generally push on with what 
they are doing, and as long as people don't introduce regressions then 
no-one complains too loudly.

A few times when I've been unsure about a patch, I've posted it to the 
list for comment and people have been fairly quick in the past to 
criticise any bad points, so mostly I accept lack of response that 
people are generally happy with it. Perhaps now with more people working 
on the code we should reconsider this? I also did a complete set of 
testing on my PPC test images and didn't see any regressions there, so I 
felt it was reasonably safe...

 I like the general optimization idea, but I found one typo s/fix/fit/

 and I was wondering whether the property setting function really needs 
 to live inside ofmem_common.c or whether we might want to make 
 set_property_nocopy() out of it for general use.
Yes possibly, although I think strings/integers need to be encoded (and 
it is the encoding process that makes the copy of the input) whereas 
natively the properties work using byte arrays and so we can just set 
the property to point directly to them.

 I also had some style questions related to the inconsistent use of 
 spacing inside braces. Some ppc code even has a wild mix of tabs and 8 
 spaces, which is unfortunate when many other projects including QEMU use 
 an indentation of 4. We're lacking a CODING_STYLE document imo, to look 
 up which way it's supposed to be.
Yes, maybe we should start thinking more about CODING_STYLE...

 BR,
 Andreas
 
 P.S. I have local patches to prepare ofmem_release() inside ofmem_common.c.
Interesting - these would be very useful :)


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: [phone]

Sirius Labs: http://www.siriusit.co.uk/labs