head -r345758: usefdt=1 style boot fails on PowerMac7,2 G5 (1 core per socket): Error -2 adding node /cpus/PowerpC,970 [G4 failures too, investigatory patch]
Mark Millard
marklmi at yahoo.com
Thu Apr 11 03:05:18 UTC 2019
[I show an investigatory patch and indicate its consequences. This
ends up indicating what is not a "extra '\0's in paths problem".]
On 2019-Apr-10, at 02:30, Mark Millard <marklmi at yahoo.com> wrote:
> On 2019-Apr-9, at 19:44, Mark Millard <marklmi at yahoo.com> wrote:
>
>> . . .
>
> I discovered a specific PowerMac11,2 vs. PowerMac7,2/PowerMac3,6
> difference that is involved:
>
> The difference is where nulls are vs. are not . . .
>
> I found a linux comment (after the later path evidence was
> observed):
>
> /* Fixup an Apple bug where they have bogus \0 chars in the
> * middle of the path in some properties, and extract
> * the unit name (everything after the last '/').
> */
>
> This was in the context of package-to-path use.
>
> I have evidence of this (though not from OF_package_to_path use but
> ofw_getprop_alloc use) . . .
>
> The PowerMac11,2 has, for example, in the notation of my
> dumping tool for looking at diff's of the (sorted) dumps:
>
> /device-tree/cpus/PowerPC,G5/cpu-version: hex_bytes_line# 0: 00 44 01 01
> /device-tree/cpus/PowerPC,G5/cpu-version: txt_bytes_line# 0: \000D\^A\^A
> /device-tree/cpus/PowerPC,G5/cpu-version: hex_bytes_line# 0: 00 44 01 01
> /device-tree/cpus/PowerPC,G5/cpu-version: txt_bytes_line# 0: \000D\^A\^A
> /device-tree/cpus/PowerPC,G5/cpu-version: hex_bytes_line# 0: 00 44 01 01
> /device-tree/cpus/PowerPC,G5/cpu-version: txt_bytes_line# 0: \000D\^A\^A
> /device-tree/cpus/PowerPC,G5/cpu-version: hex_bytes_line# 0: 00 44 01 01
> /device-tree/cpus/PowerPC,G5/cpu-version: txt_bytes_line# 0: \000D\^A\^A
>
> But the PowerMac7,2 and PowerMac3,6 have a null character in the analogous
> prefix (produced by the same criteria), here shown with ^@:
>
> /device-tree/cpus/PowerPC,970^@/cpu-version: hex_bytes_line# 0: 00 39 02 02
> /device-tree/cpus/PowerPC,970^@/cpu-version: txt_bytes_line# 0: \0009\^B\^B
> /device-tree/cpus/PowerPC,970^@/cpu-version: hex_bytes_line# 0: 00 39 02 02
> /device-tree/cpus/PowerPC,970^@/cpu-version: txt_bytes_line# 0: \0009\^B\^B
>
> /device-tree/cpus/PowerPC,G4^@/cpu-version: hex_bytes_line# 0: 80 01 03 03
> /device-tree/cpus/PowerPC,G4^@/cpu-version: txt_bytes_line# 0: \M^@\^A\^C\^C
> /device-tree/cpus/PowerPC,G4^@/cpu-version: hex_bytes_line# 0: 80 01 03 03
> /device-tree/cpus/PowerPC,G4^@/cpu-version: txt_bytes_line# 0: \M^@\^A\^C\^C
>
> The code that produces a name for a ofw node, as shown after a / above,
> is (C++17 notation):
>
> auto name_for_ofw_node= [&ofw_fd](auto ofw_nd) -> auto
> {
> std::string nd_id_text{};
>
> void* name_buf= nullptr;
> int name_buf_len= 0;
> auto const name_len= ofw_getprop_alloc(ofw_fd,ofw_nd,"name",&name_buf,&name_buf_len,1);
> if (0<name_len)
> nd_id_text+= std::string{static_cast<char const *>(name_buf), static_cast<std::string::size_type>(name_len-1)};
> else
> {
> . . . (does not happen) . . .
> }
>
> return nd_id_text;
> };
>
> [With name_len instead of name_len-1 there would be one more '\0'
> character in each such name after a / (before the first ":"):
> the terminating null character would be included.]
>
> If such a before-the-end-of-path ^@ shows up in the likes of
> add_node_to_fdt via its use of OF_package_to_path:
>
> char name[255], *lastprop, *subname;
> . . .
> for (node = OF_child(node); node > 0; node = OF_peer(node)) {
> OF_package_to_path(node, name, sizeof(name));
> subname = strrchr(name, '/');
> subname++;
> child_offset = fdt_add_subnode(buffer, fdt_offset, subname);
> if (child_offset < 0) {
> printf("Error %d adding node %s (%s), skipping\n",
> child_offset, name, subname);
> continue;
> }
>
> then the strrchr will not work as intended and the error message
> will result (as it does for PowerMac7,2/PowerMac3,6).
>
> I'll note that the return value of OF_package_to_path is ignored
> above but the description I find is:
>
> QUOTE
> package-to-path
> IN: phandle, [address] buf, buflen OUT: length
>
> Returns the fully qualified pathname corresponding to the node identifier phandle, storing, at most, buflen bytes as a null-terminated string in the memory buffer starting at the address buf. If the length of the null- terminated pathname is greater than buflen, the trailing characters and the null terminator are not stored. Length is the length of the fully qualified pathname excluding any null terminator, or –1 if phandle is invalid.
> END QUOTE
>
> The linux code does use the return value in order to not be fooled by
> null characters in the middle of the path.
The following investigatory patch prevents some of the
"Error -2 adding node . . ." notices. It:
A) makes no change for the PowerMac11,2 G5 (2 sockets, 2 cores each)
B) eliminates the message for PowerMac7,2 G5 (2 sockets, 1 core each),
eliminated is: Error -2 adding node /cpus/PowerPC,970 (PowerPC,970), skipping
C) eliminates one message for PowerMac3,6 G4 (2 sockets, 1 core each), the
one is: Error -2 adding node /cpus/PowerPC,G4 (PowerPC,G4), skipping
For (A): Apparently extra '\0's is not the reason the PowerMac11,2
gets one such message. I do not know why it does. Both powerpc64
and 32-bit powerpc FreeBSD still report:
Error -2 adding node /ht at 0,f2000000/pci at 8/mac-io at 7/i2c at 18000/i2c-bus at 0 (i2c-bus at 0), skipping
For (B): Eliminating the message is still followed by the boot
hanging up after "Kernel entry at . . .". It does not get as far
as clearing the console screen. Something else is going on for
this. The powerpc64 and 32-bit powerpc FreeBSD results are the
same. But I've no clue how to isolate it. (Booting without
usefdt mode works fine.)
For (C): Both CPUs are now used in usefdt mode but ethernet is
still not present. The messages still generated are:
Error -2 adding node /pci at f2000000//mac-io at 17/gpio at 50/gpio5 at 6f (gpio5 at 6f), skipping
Error -2 adding node /pci at f2000000//mac-io at 17/gpio at 50/gpio6 at 70 (gpio6 at 70), skipping
Error -2 adding node /pci at f2000000//mac-io at 17/gpio at 50/gpio11 at 75 (gpio11 at 75), skipping
Error -2 adding node /pci at f2000000//mac-io at 17/extint-gpio at 15@67 (extint-gpio at 15@67), skipping
While the patch is justified by Macintosh problems, it should
be valid for openfirmware that has no "extra '\0' character"
problems in paths. My only testing environment is the old
PowerMacs, however.
The patch has add_node_to_fdt eliminate the extra/internal '\0'
characters in paths/names it creates, instead of preserving them
carefully. Why? If preserved, lots of other code seems to need to
be modified to deal with them. Another type of alternative would
have been to replace the '\0' characters with some other, say '_'.
(I'm not aware of a reason that name and path lengths would need
to be preserved.)
The patch is:
(I do not claim to have coded for direct acceptance into FreeBSD's
code base: investigatory material.)
Index: /usr/src/stand/powerpc/ofw/ofwfdt.c
===================================================================
--- /usr/src/stand/powerpc/ofw/ofwfdt.c (revision 345758)
+++ /usr/src/stand/powerpc/ofw/ofwfdt.c (working copy)
@@ -45,7 +45,7 @@
add_node_to_fdt(void *buffer, phandle_t node, int fdt_offset)
{
int i, child_offset, error;
- char name[255], *lastprop, *subname;
+ char name[255+1], *lastprop, *subname; // +1 added for always having a trailing '\0' position.
void *propbuf;
ssize_t proplen;
@@ -77,9 +77,54 @@
&& !OF_hasprop(node, "ibm,phandle"))
fdt_setprop(buffer, fdt_offset, "phandle", &node, sizeof(node));
+ // WARNING: openfirmware's package-to-path(nd,nm,len) does not place a trailing '\0'
+ // character in nm when it returns a full_str_len with len<=full_str_len .
+ // For full_str_len<len, a trailing '\0' is placed at nm[full_str_len].
+
+ name[255]= '\0'; // Avoid OF_package_to_path leaving no '\0' at end of long path.
for (node = OF_child(node); node > 0; node = OF_peer(node)) {
- OF_package_to_path(node, name, sizeof(name));
- subname = strrchr(name, '/');
+ int full_str_len= OF_package_to_path(node, name, sizeof(name)-1); // Avoids having trailing '\0' missing.
+ if (-1==full_str_len) { // Highly unlikely.
+ printf("add_node_to_fdt got -1 return from OF_packakge_to_path\n");
+ continue;
+ }
+
+ // WARNING: For some Macintoshes, name can sometimes contain '\0' characters
+ // in the middle (before what will be subname)!
+
+ // full_str_len omits the offical trailing '\0' position.
+ // full_str_len is *not* limited by the sizeof(name)-1 value above: it reports
+ // the space needed to get all the text (ignoring the official trailing '\0').
+ if (0==full_str_len) { // Highly unlikely.
+ printf("Error: Node name has no bytes before trailing null byte\n");
+ continue;
+ }
+ if (255<full_str_len) {
+ printf("Error: Node path %s (truncated), needs more than 255+1 bytes\n", name);
+ continue;
+ }
+
+ // Eliminate internal '\0' path characters: make saved results more standard.
+ char *from_pos= name;
+ char *to_pos= name;
+ char *original_end= name+full_str_len;
+ while (from_pos<original_end) {
+ if ('\0'!=*from_pos) {
+ *to_pos++= *from_pos;
+ }
+ from_pos++;
+ }
+ *to_pos= '\0'; // Make sure '\0' terminated.
+ if (to_pos==name) { // Highly unlikely.
+ printf("Error: Adjusted node name has no bytes before trailing null byte\n");
+ continue;
+ }
+
+ subname= strrchr(name,'/');
+ if (NULL==subname) { // Highly unlikely.
+ printf("Error: Node name %s has no '/'\n", name);
+ continue;
+ }
subname++;
child_offset = fdt_add_subnode(buffer, fdt_offset, subname);
if (child_offset < 0) {
===
Mark Millard
marklmi at yahoo.com
( dsl-only.net went
away in early 2018-Mar)
More information about the freebsd-ppc
mailing list