Page MenuHomeFreeBSD
Differential D15667

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

Authored by lffpires_ruabrasil.org on Jun 5 2018, 12:35 PM.
Tags
Referenced Files
Unknown Object (File)
Sun, Nov 17, 2:37 PM
Unknown Object (File)
Fri, Nov 8, 4:50 AM
Unknown Object (File)
Sun, Nov 3, 2:46 PM
Unknown Object (File)
Thu, Oct 31, 7:41 PM
Unknown Object (File)
Tue, Oct 29, 12:43 PM
Unknown Object (File)
Tue, Oct 29, 11:32 AM
Unknown Object (File)
Tue, Oct 22, 1:27 PM
Unknown Object (File)
Oct 17 2024, 12:34 PM
View All Files
Subscribers
imp
leonardo.bianconi_eldorado.org.br

Details

Reviewers
jhibbits
breno.leitao_gmail.com
Commits
rS334816: Fix excise_initrd_region() to support 32- and 64-bit initrd params.
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 Passed
Unit
No Test Coverage
Build Status
Buildable 17050
Build 16913: arc lint + arc unit

Event Timeline

lffpires_ruabrasil.org created this revision.Jun 5 2018, 12:35 PM
Herald added subscribers: leonardo.bianconi_eldorado.org.br, jhibbits. · View Herald TranscriptJun 5 2018, 12:35 PM
Harbormaster completed remote builds in B17020: Diff 43334.Jun 5 2018, 12:35 PM
lffpires_ruabrasil.org updated this revision to Diff 43335.Jun 5 2018, 2:00 PM

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

Harbormaster completed remote builds in B17021: Diff 43335.Jun 5 2018, 2:00 PM
jhibbits added a comment.Jun 5 2018, 2:02 PM
In D15667#331142, @lffpires_ruabrasil.org wrote:

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
303–304

This really should be OF_getencprop() anyway.

317

This as well.

lffpires_ruabrasil.org updated this revision to Diff 43336.Jun 5 2018, 2:02 PM

Removed dummy comment

Harbormaster completed remote builds in B17022: Diff 43336.Jun 5 2018, 2:02 PM
lffpires_ruabrasil.org updated this revision to Diff 43339.Jun 5 2018, 6:39 PM

Rework after comments

Harbormaster completed remote builds in B17025: Diff 43339.Jun 5 2018, 6:39 PM
lffpires_ruabrasil.org marked 2 inline comments as done.Jun 5 2018, 6:42 PM
In D15667#331145, @jhibbits wrote:
In D15667#331142, @lffpires_ruabrasil.org wrote:

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.

lffpires_ruabrasil.org edited the summary of this revision. (Show Details)Jun 5 2018, 6:43 PM
lffpires_ruabrasil.org added reviewers: jhibbits, breno.leitao_gmail.com.
lffpires_ruabrasil.org added a project: PowerPC.
lffpires_ruabrasil.org removed a subscriber: jhibbits.
breno.leitao_gmail.com added a comment.Jun 5 2018, 9:09 PM

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.

breno.leitao_gmail.com accepted this revision.Jun 5 2018, 9:09 PM
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
317

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.

333

Same here as above.

This revision now requires changes to proceed.Jun 5 2018, 9:16 PM
lffpires_ruabrasil.org added inline comments.Jun 6 2018, 2:34 PM
sys/powerpc/ofw/ofw_machdep.c
317

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
317

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.

lffpires_ruabrasil.org updated this revision to Diff 43371.Jun 6 2018, 4:48 PM
  • Improved linux,initrd-{start,end} length checking
Harbormaster completed remote builds in B17040: Diff 43371.Jun 6 2018, 4:48 PM
lffpires_ruabrasil.org marked 2 inline comments as done.Jun 6 2018, 4:49 PM
lffpires_ruabrasil.org updated this revision to Diff 43374.Jun 6 2018, 5:10 PM
  • Removed extra printf param
Harbormaster completed remote builds in B17042: Diff 43374.Jun 6 2018, 5:10 PM
breno.leitao_gmail.com added inline comments.Jun 6 2018, 6:40 PM
sys/powerpc/ofw/ofw_machdep.c
304

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

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.

lffpires_ruabrasil.org added inline comments.Jun 6 2018, 7:26 PM
sys/powerpc/ofw/ofw_machdep.c
304

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

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

lffpires_ruabrasil.org updated this revision to Diff 43387.Jun 6 2018, 9:04 PM

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

Harbormaster completed remote builds in B17050: Diff 43387.Jun 6 2018, 9:04 PM
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
lffpires_ruabrasil.org updated this revision to Diff 43436.Jun 7 2018, 8:44 PM
  • Improved size checking
This revision now requires review to proceed.Jun 7 2018, 8:44 PM
Harbormaster completed remote builds in B17084: Diff 43436.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
breno.leitao_gmail.com accepted this revision.Jun 7 2018, 8:57 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
Closed by commit rS334816: Fix excise_initrd_region() to support 32- and 64-bit initrd params. (authored by leitao). · Explain WhyJun 7 2018, 9:24 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: imp. · View Herald TranscriptJun 7 2018, 9:24 PM

Revision Contents
Changeset List

PathSize
sys/
powerpc/
ofw/
27 lines

Diff 43387

View Options

sys/powerpc/ofw/ofw_machdep.c

Loading...