image-hd: add support for overridding optional fields of the MBR

[tpetazzoni]

The 'holes' property allows to describe holes in an image, for example to allow writing an image that spans accross the MBR partition table. The image-hd code creates a fake partition spanning from 440 bytes to 512 bytes to protect the area used by the partition table.

However, it turns out that the Amlogic SoCs have a bootloader that is precisely written accross the partition table, with some bits before the partition table and some bits after. But those Amlogic SoCs use the first 444 bytes, instead of just the first 440 bytes (see [1]).

This is possible because the first 2 fields of the MBR are optional fields (see [2]). The first optional field is the disk signature (4 bytes), the second optional field is the "copy protect" field (2 bytes).

Therefore, to write a bootloader image for the Amlogic SoCs, we need to write something like:

image fip/u-boot.bin.sd.bin {
file {
holes = {"(444; 512)"}
}
}

But that isn't accepted because it overlaps with the fake MBR partition that starts at offset 440.

In order to allow this, the present commit adds a new 'mbr-skip-optionals' property, which tells genimage that we would like to skip writing those optional fields, allowing the full 446 bytes to be used, as is needed for Amlogic SoC bootloader images.

Implementation note: we would have preferred to write:

mbr_data += sizeof(struct mbr_tail_optionals);

but genimage has chosen to not allow arithmetic on void* pointers, so instead we have chosen to use:

mbr_data = &mbr.part_entry[0];

when adjusting the start of the MBR data that have to be inserted into the image.

This commit also adds two test cases for this new feature:

• One positive test case, where we verify that we can write an image where the hole is (444, 512) and the mbr-skip-optionals = "true" option is passed

• One negative test case, where we verify that we are not allowed to write an image where the hole is (444, 512) when mbr-skip-optionals = "false".

[1] http://docs.khadas.com/products/sbc/vim3/development/create-bootable-tf-card
[2] https://wiki.osdev.org/MBR_(x86)

Co-Developed-by: Romain Naour [email protected]

[a3f]

But those Amlogic SoCs use the first 444 bytes, instead of just the first 440 bytes (see [1]).

Here you mention it's 444 bytes.

allowing the full 446 bytes to be used, as is needed for Amlogic SoC bootloader images

But here it's 446 bytes.

---

Assuming you need just 444 bytes for Amlogic, how about we ditch the new mbr-skip-optionals option and instead consult the existing disk-signature option: If it's zero, the default value is zero-filled, but genimage doesn't complain if a hole overlaps it. If it's a non-zero value (or random), placing a hole there would result in an error.

I think this would be the most convenient for users. What do you think?

[tpetazzoni]

First of all, thanks for the timely and to-the-point feedback, much appreciated!

But those Amlogic SoCs use the first 444 bytes, instead of just the first 440 bytes (see [1]).

Here you mention it's 444 bytes.

allowing the full 446 bytes to be used, as is needed for Amlogic SoC bootloader images

But here it's 446 bytes.

The Amlogic stuff apparently only needs the first 444 bytes. I went for 446 bytes, because there are two fields that are optional, and combined together they allow to use the first 446 bytes. So even if for Amlogic I needed 444, I made it possible to use all the "extra" space that is offered by those two optional fields.

Assuming you need just 444 bytes for Amlogic, how about we ditch the new mbr-skip-optionals option and instead consult the existing disk-signature option: If it's zero, the default value is zero-filled, but genimage doesn't complain if a hole overlaps it. If it's a non-zero value (or random), placing a hole there would result in an error.

I think this would be the most convenient for users. What do you think?

I think that's a great idea! Should I go ahead and rework the change in that direction?

[a3f]

First of all, thanks for the timely and to-the-point feedback, much appreciated!

Thank you for your contribution. :)

I think that's a great idea! Should I go ahead and rework the change in that direction?

I haven't looked into the code yet and just thought to point this possible improvement out. If you like, you can also wait for @michaelolbrich's maintainer feedback first.

[michaelolbrich]

Sorry for the late reply, it has been a busy summer.

I would like to avoid adding a new options. But we need to be careful with the implementation. If the hole starts at 440 then we should overwrite the disk signature even if it's zero. That way we're backwards compatible.

So I think there are 3 cases:

1. hole at 440 with implicit or explicit disk signature -> ok
2. hole at 444 or 446 without disk signature -> ok
3. hole at 444 or 446 with disk signature -> error

[tpetazzoni]

Sorry for the delay. This obviously seems reasonable. I don't know when/if I'll get the chance to adjust the implementation.