fix Moto sanders DTB

[playday3008]

Closes: #559

• Stock boot.img of Motorola G5s Plus (sanders) have:
  • compressed QCDT via LZ4
    • added documentation for repacking stock image
  • vendor specific qcom,board-id values
    • can be handled by lk2nd, by splitting dts into separate dt's
• Wrong format of DT was used for Motorola G5s Plus (sanders) (this closes Moto G5s Plus (msm8953) fails to boot with lk2nd – "Error: failed to load kernel" #559)

[playday3008]

This PR is complete (in my opinion)

[stephan-gh]

Can you separate the new features into a separate PR so that there is only the actual fix for #559 in this PR?

I appreciate the work you did to enable booting downstream boot images, but I'm afraid I'm not entirely sure yet if we can accept that change. Generally, booting downstream with lk2nd is only supported on a "least effort" basis. It's good if it works, but the problem is that there are tons of subtly incompatible vendor modifications to the boot image, DTB selection or QCDT format and it would be an extreme effort to support all of those. With the 150+ devices we support nowadays, there is also a significant risk that implementing the modifications of one vendor breaks booting on the devices from another vendor.

In this case for example I'm concerned that changing the struct dt_table definition (splitting up version into multiple fields) might break booting on some other devices. There is also quite some maintenance overhead to add a copy of the lz4 library just to decompress the DTBs for downstream on some Motorola devices...

What is the use case you have in mind for these changes? How often would one really boot downstream with lk2nd on these devices?

[playday3008]

significant risk that implementing the modifications of one vendor breaks booting on the devices from another vendor.

True, but.
LZ4 decompression only occurs when DT starts with LZ4F_MAGICNUMBER, which is impossible for normal DT, no matter how broken vendor made it, when LZ4 does not occur, nothing changes in code flow, as any changes are guarded by magic check.
Motorola specific check only returns true, when mmi_unit_info is not null, which can only be non-null on Motorola devices. So again, code flow does not change for non Motorola devices.

In this case for example I'm concerned that changing the struct dt_table definition (splitting up version into multiple fields) might break booting on some other devices.

About splitting struct dt_table, this might break something only when used on big endian QCDT, which I'm not even sure exists, as it's structure inside a file, and not inside a code.

There is also quite some maintenance overhead to add a copy of the lz4 library just to decompress the DTBs for downstream on some Motorola devices...

Adding lz4 increases the size of the final binary for everyone, and a non-significant amount of people actually needs this. But I'm not sure that we know about all instances where adding lz4 would actually resolve a problem that somebody had. So, I think that adding lz4 have still more advantages than disadvantages. Also, I didn't add a specific commit of lz4, but just tag 1.10.0 which can be observed in lz4.h, and changes that had to be made to make it work in lk2nd are in a separate commit (7fefd3f), so we know what has to be done to lz4 if we wanted to update it someday.

lk2nd/lib/lz4/lz4.h

Lines 131 to 133 in 20c6ebb

	#define LZ4_VERSION_MAJOR 1 /* for breaking interface changes */
	#define LZ4_VERSION_MINOR 10 /* for new (non-breaking) interface capabilities */
	#define LZ4_VERSION_RELEASE 0 /* for tweaks, bug-fixes, or development */

What is the use case you have in mind for these changes? How often would one really boot downstream with lk2nd on these devices?

It's very useful to have "dual boot" with lk2nd, because we can have stock kernel in phone, and WIP mainline from sdcard. Which allows anybody to still have a working phone while working on mainline kernel to support said phone. (Not only mainline, maybe some custom kernel from stock, IDK, I just provided an example that relevant to me)

Can you separate the new features into a separate PR so that there is only the actual fix for #559 in this PR?

If you really-really want only this change, just cherry-pick d2d0b98 commit into main, this will also automatically close the issue. But please reconsider.

[stephan-gh]

About splitting struct dt_table, this might break something only when used on big endian QCDT, which I'm not even sure exists, as it's structure inside a file, and not inside a code.

That is true, but the problem I meant was that there are multiple vendors who modified this structure differently. I'm pretty sure we've had some weird QCDT table from OPPO before that we decided against supporting. If we start adding all sorts of different formats it will become really difficult to maintain (as in: required effort, but also making sure none of the many vendor-specific modifications break when making changes).

As it stands, lk2nd wouldn't integrate perfectly into such a setup anyway. You would need to flash the downstream boot image manually on each update (since the OTA process won't respect the lk2nd offset). While doing that, couldn't you also repack the downstream image to fit the expectations of lk2nd better?

If you decompress the LZ4 QCDT, we could already drop that extra dependency. That would be easy to do with a simple script.

The is_stock_match() function you add for DTB selection could perhaps reuse the lk2nd_dt_override functionality. It does more or less the same already if you would fill it from motorola-unit-info.

You could probably also drop the extra field from the struct dt_table while repacking the boot image. Given that lk2nd boots with just QCDTBS +=, apparently Motorola's bootloader supports the non-customized QCDT format as well? So the repacked/differently built boot images would likely also work with the stock bootloader?

More generally: I'm not opposed to add a couple of simple and straightforward lines to improve booting behavior where possible, but it needs to be as minimal as possible. lk2nd is already extremely complex to maintain with so many supported devices, I'm afraid we don't have the capacity to maintain larger amounts of device or vendor-specific code. I wish this was different, but that's unfortunately how it is today.

[playday3008]

That is true, but the problem I meant was that there are multiple vendors who modified this structure differently. I'm pretty sure we've had some weird QCDT table from OPPO before that we decided against supporting. If we start adding all sorts of different formats it will become really difficult to maintain (as in: required effort, but also making sure none of the many vendor-specific modifications break when making changes).

I've had an idea of instead of changing struct dt_table structure, just do table-> version & 0xFF everywhere, but changing struct looked less painful.

The is_stock_match() function you add for DTB selection could perhaps reuse the lk2nd_dt_override functionality. It does more or less the same already if you would fill it from motorola-unit-info.

Actually, I wish I knew how to utilize that functionality, I couldn't find any example or anything that will explain how and with what lk2nd_dt_override is getting populated.

Couldn't you also repack the downstream image to fit the expectations of lk2nd better? If you decompress the LZ4 QCDT, we could already drop that extra dependency. That would be easy to do with a simple script.

I could repack it in the way that will not require lz4 dependency in lk2nd, but QCDT structure will still be extended, that needs to be accounted for.

You could probably also drop the extra field from the struct dt_table while repacking the boot image. Given that lk2nd boots with just QCDTBS +=, apparently Motorola's bootloader supports the non-customized QCDT format as well? So the repacked/differently built boot images would likely also work with the stock bootloader?

You mean changing QCDT structure itself in boot.img? Like removing name/model field from it? IDK how painlessly do that (means automate it, I know how to do it manually)

Example

From:

To:

lk2nd is already extremely complex to maintain with so many supported devices

Yeah, I wanted to completely rewrite it someday in Zig, and I gave up on 1%.

[stephan-gh]

Actually, I wish I knew how to utilize that functionality, I couldn't find any example or anything that will explain how and with what lk2nd_dt_override is getting populated.

Yeah it's pretty unobvious, but it's filled by dev_tree_override_match:

lk2nd/platform/msm_shared/dev_tree.c

Lines 2479 to 2496 in 0cf7784

	#if WITH_LK2ND_DEVICE
	struct dt_entry *dev_tree_override_match(const void *fdt, int offset)
	{
	struct dt_entry_node node = {
	.node = LIST_INITIAL_VALUE(node.node),
	.dt_entry_m = &lk2nd_dt_override,
	};
	if (!dev_tree_compatible(fdt, fdt, fdt_totalsize(fdt), offset, &node))
	return NULL;
	if (platform_dt_absolute_match(node.dt_entry_m, NULL))
	node.dt_entry_m->offset = 0; /* No need to override */
	else
	dprintf(INFO, "Unexpected DTB selected by bootloader, need to override\n");
	return &lk2nd_dt_override;
	}
	#endif

Basically that thing goes through the parsing/matching sequence for a qcom,board-id defined in a lk2nd device node. This is used e.g. for some Samsung devices:

lk2nd/lk2nd/device/dts/msm8916/msm8916-samsung.dts

Lines 32 to 33 in 0cf7784

	qcom,msm-id = <QCOM_ID_MSM8916 0>;
	qcom,board-id = <0xCE08FF01 0>;

The reason why it won't work for you is that you have multiple qcom,board-id entries in the sanders DT, so it doesn't know which one to use. You would need to expose lk2nd_dt_override or add some function to set the fields inside there from the motorola-unit-info code.

You mean changing QCDT structure itself in boot.img? Like removing name/model field from it? IDK how painlessly do that (means automate it, I know how to do it manually)

Right. I would probably prepare some simple Python script to do this. lk2nd has a script for replacing the kernel inside an Android boot image for example:

https://github.com/msm8916-mainline/lk2nd/blob/main/lk2nd/scripts/patch-boot-img.py

Also, in a different repository we have a script to unpack QCDT images: https://github.com/msm8916-mainline/linux-mdss-dsi-panel-driver-generator/blob/master/tools/unpackqcdt.py

Should be easy to modify this to unpack the Motorola-extended format and then produce the standard format instead.

[playday3008]

lk2nd/lk2nd/device/dts/msm8916/msm8916-samsung.dts

Lines 32 to 33 in 0cf7784

	qcom,msm-id = <QCOM_ID_MSM8916 0>;
	qcom,board-id = <0xCE08FF01 0>;

The reason why it won't work for you is that you have multiple qcom,board-id entries in the sanders DT, so it doesn't know which one to use. You would need to expose lk2nd_dt_override or add some function to set the fields inside there from the motorola-unit-info code.

Will splitting dts into 6 different versions help?
It's done "almost" like that in the stock linux kernel:

Also, I'll look into writing a Python code to patch moto dt, but where should I put it, make it a part of patch-boot-img.py, maybe to gist and provide a link?

[stephan-gh]

Also, I'll look into writing a Python code to patch moto dt, but where should I put it, make it a part of patch-boot-img.py, maybe to gist and provide a link?

patch-boot-img.py is intended for lk2nd testing, so not really meant for your purpose. I guess a gist with the script and some instructions would be helpful.

Will splitting dts into 6 different versions help?
It's done "almost" like that in the stock linux kernel:

That would work, yes. In this specific case I do wonder if anyone actually has p1/p2/p3. Sounds a bit like preproduction versions that were never sold.

[playday3008]

@stephan-gh That's better?

[barni2000]

Other motos are not splitted by revisions maybe it is also not necessary for sanders.

[playday3008]

Other motos are not splitted by revisions maybe it is also not necessary for sanders.

It's necessary for a stock boot.img, for non-stock it doesn't matter.

[playday3008]

As lk2nd overriding feature only works with dts where and/or qcom,board-id, qcom,msm-id, qcom,pmic-id are having single value, here's code below that checks for it.

lk2nd/platform/msm_shared/dev_tree.c

Lines 970 to 974 in 0cf7784

	if (dtb_list->dt_entry_m && (board_data_count > 1 || msm_data_count > 1 || pmic_data_count > 1)) {
	if (model)
	free(model);
	return false;
	}

And overriding feature is required for stock boot.img to boot successfully.
Overriding feature requires complete rewrite to be able to handle overriding without splitting dtb's.

[barni2000]

It seems sanders are very similar for potter it needs QCDT image, but only initramfs compressed with lz4, kernel is using gzip.

2048                               0x800                              gzip compressed data, operating system: Unix, timestamp: 1970-01-01 00:00:00, total size: 9235526 bytes
9238528                            0x8CF800                           LZMA compressed data, properties: 0x5D, dictionary size: 8388608 bytes, compressed size: 8929225 bytes, uncompressed
                                                                      size: -1 bytes
18169856                           0x1154000                          Device tree blob (DTB), version: 17, CPU ID: 0, total size: 235053 bytes
18405376                           0x118D800                          Device tree blob (DTB), version: 17, CPU ID: 0, total size: 235053 bytes
18640896                           0x11C7000                          Device tree blob (DTB), version: 17, CPU ID: 0, total size: 235061 bytes
18876416                           0x1200800                          Device tree blob (DTB), version: 17, CPU ID: 0, total size: 234840 byte

deviceinfo_generate_bootimg="true"
deviceinfo_flash_pagesize="2048"
deviceinfo_qcdt_type="qcom"
deviceinfo_flash_offset_base="0x80000000"
deviceinfo_flash_offset_kernel="0x00008000"
deviceinfo_flash_offset_ramdisk="0x01000000"
deviceinfo_flash_offset_second="0x00f00000"
deviceinfo_flash_offset_tags="0x00000100"