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

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 17020
Build 16888: arc lint + arc unit

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

This really should be OF_getencprop() anyway.

313

This as well.

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
313

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.

323

Same here as above.

This revision now requires changes to proceed.Jun 5 2018, 9:16 PM
sys/powerpc/ofw/ofw_machdep.c
313

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?

sys/powerpc/ofw/ofw_machdep.c
313

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
sys/powerpc/ofw/ofw_machdep.c
303

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

sys/powerpc/ofw/ofw_machdep.c
303

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
303

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.

sys/powerpc/ofw/ofw_machdep.c
303

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.

This revision is now accepted and ready to land.Jun 7 2018, 8:43 PM
This revision now requires review to proceed.Jun 7 2018, 8:44 PM
This revision is now accepted and ready to land.Jun 7 2018, 8:48 PM
This revision was automatically updated to reflect the committed changes.