Page MenuHomeFreeBSD

Changed excise_initrd_region to support both 32- and 64-bit values for linux,initrd-start and linux,initrd-end
ClosedPublic

Authored by lffpires_ruabrasil.org on Jun 5 2018, 12:35 PM.

Details

Summary

Fix excise_initrd_region() to support 32- and 64-bit initrd params.

Changed excise_initrd_region to support both 32- and 64-bit
values for linux,initrd-start and linux,initrd-end.

This fixes the boot problem on some machines after rS334485.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

After further investigation, linux,initrd-start/end are always 32-bit values

After further investigation, linux,initrd-start/end are always 32-bit values

That's odd, because they're 64-bit values in the device tree:

Node 0xe38: chosen

linux,initrd-end:
  00 00 00 00 00 00 00 00
linux,initrd-start:
  00 00 00 00 00 00 00 00
sys/powerpc/ofw/ofw_machdep.c
302 ↗(On Diff #43335)

This really should be OF_getencprop() anyway.

306 ↗(On Diff #43335)

This as well.

Removed dummy comment

Rework after comments

lffpires_ruabrasil.org marked 2 inline comments as done.Jun 5 2018, 6:42 PM

After further investigation, linux,initrd-start/end are always 32-bit values

That's odd, because they're 64-bit values in the device tree:
Node 0xe38: chosen

linux,initrd-end:
  00 00 00 00 00 00 00 00
linux,initrd-start:
  00 00 00 00 00 00 00 00

Since they're 64-bit on power9 and 32-bit on power8, we'll have to handle both cases again.

I tested it on my POWER9 and it continues to boot. This change seems to make sense, and fixes 32-bits initrd addresses, but I am not sure how it broke Justin's P9, and if it is going to fix.

This revision is now accepted and ready to land.Jun 5 2018, 9:09 PM
jhibbits requested changes to this revision.Jun 5 2018, 9:16 PM
jhibbits added inline comments.
sys/powerpc/ofw/ofw_machdep.c
308 ↗(On Diff #43339)

I wouldn't make this a KASSERT(). Problems with the device tree should error appropriately, not panic. It's possible the device tree may have a 12-byte entry here, or longer. It's a broken tree, but not a broken kernel.

KASSERT() is for an invariant violation, not an external dependency violation. Just return early here. An error is appropriate, though.

318 ↗(On Diff #43339)

Same here as above.

This revision now requires changes to proceed.Jun 5 2018, 9:16 PM
sys/powerpc/ofw/ofw_machdep.c
308 ↗(On Diff #43339)

What is an appropriate way to do error handling here? Should I printf() an error message and return (asz), allowing the boot process to continue?

jhibbits added inline comments.Jun 6 2018, 2:44 PM
sys/powerpc/ofw/ofw_machdep.c
308 ↗(On Diff #43339)

Yes. Even if there is an initrd, if it's not valid it can't be used, so don't try, and warn the user.

  • Improved linux,initrd-{start,end} length checking
lffpires_ruabrasil.org marked 2 inline comments as done.Jun 6 2018, 4:49 PM
  • Removed extra printf param
sys/powerpc/ofw/ofw_machdep.c
304 ↗(On Diff #43374)

Why do you have 3 entries for cell? Shouldn't it be 2?

jhibbits added inline comments.Jun 6 2018, 6:47 PM
sys/powerpc/ofw/ofw_machdep.c
304 ↗(On Diff #43374)

You only need 2 cells. OF_getencprop() returns the actual size, not the size read. It will only write up to the size passed in.

sys/powerpc/ofw/ofw_machdep.c
304 ↗(On Diff #43374)

Why do you have 3 entries for cell? Shouldn't it be 2?

I used 3 to be able to detect errors in the device tree where initrd-{start,end} are longer than 8 bytes. If I used only 2 cells here, I wouldn't be able to tell that.
I could also do separate calls to OF_getproplen(), but I thought it would be simpler this way.

You only need 2 cells. OF_getencprop() returns the actual size, not the size read. It will only write up to the size passed in.

OF_getencprop() is returning the size read, only limited to the size passed in.

Looking at sys/dev/ofw/openfirm.c, it returns the value it got from OF_getprop(). And ofw_fdt_getprop() in sys/dev/ofw/ofw_fdt.c trims the length returned from the fdt to the buffer length that was passed in.

I double-checked this by trying to read another value (ibm,architecture-vec-5) whose length is 6, and verified that OF_getencprop() returns 6 when called with a buffer with 2 cells, but 4 when the buffer had just 1.

jhibbits added inline comments.Jun 6 2018, 8:11 PM
sys/powerpc/ofw/ofw_machdep.c
304 ↗(On Diff #43374)

OF_getencprop() is returning the size read, only limited to the size passed in.

That's contrary to the man page, which states that it returns the actual size in bytes, not the size copied. Does it behave this way with a real OF machine (Apple PowerMac, or a different IBM environment)? If so, the man page needs updated, and your change is appropriate. If not, ofw_fdt_getprop() needs to be fixed in sys/dev/ofw/ofw_fdt.c

Changed to reflect the correct behavior of OF_getencprop().
A patch to fix ofw_fdt_getprop() was submitted as D15680.

jhibbits accepted this revision.Jun 7 2018, 8:43 PM
This revision is now accepted and ready to land.Jun 7 2018, 8:43 PM
  • Improved size checking
This revision now requires review to proceed.Jun 7 2018, 8:44 PM
jhibbits accepted this revision.Jun 7 2018, 8:48 PM
This revision is now accepted and ready to land.Jun 7 2018, 8:48 PM
lffpires_ruabrasil.org edited the summary of this revision. (Show Details)Jun 7 2018, 9:01 PM
lffpires_ruabrasil.org edited the summary of this revision. (Show Details)Jun 7 2018, 9:09 PM
This revision was automatically updated to reflect the committed changes.