I just noticed this patch -- I missed it because the cover message seemed far more harmless so I didn't notice this change. THIS PATCH IS FATALLY WRONG AND NEEDS TO BE IMMEDIATELY REVERTED BEFORE ANYONE STARTS RELYING ON IT; IT HAS THE POTENTIAL OF BREAKING THE BOOTLOADER PROTOCOL FOR ALL FUTURE. It seems to be based on fundamental misconceptions about the various data structures in the protocol, and does so in a way that completely breaks the way the protocol is designed to work. The protocol is specifically designed such that fields are not version dependencies. The version number is strictly to inform the boot loader about which capabilities the kernel has, so that the boot loader can know if a certain data field is meaningful and/or honored. > +Protocol 2.14: (Kernel 4.20) Added acpi_rsdp_addr holding the physical > + address of the ACPI RSDP table. > + The bootloader updates version with: > + 0x8000 | min(kernel-version, bootloader-version) > + kernel-version being the protocol version supported by > + the kernel and bootloader-version the protocol version > + supported by the bootloader. [...] > **** MEMORY LAYOUT > > The traditional memory map for the kernel loader, used for Image or > @@ -197,6 +209,7 @@ Offset Proto Name Meaning > 0258/8 2.10+ pref_address Preferred loading address > 0260/4 2.10+ init_size Linear memory required during initialization > 0264/4 2.11+ handover_offset Offset of handover entry point > +0268/8 2.14+ acpi_rsdp_addr Physical address of RSDP table NO. That is not how struct setup_header works, nor does this belong here. struct setup_header contains *initialized data*, and has a length byte at offset 0x201. The bootloader is responsible for copying the full structure into the appropriate offset (0x1f1) in struct boot_params. The length byte isn't actually a requirement, since the maximum possible size of this structure is 144 bytes, and the kernel will (obviously) not look at the older fields anyway, but it is good practice. The kernel or any other entity is free to zero out the bytes past this length pointer. There are only 24 bytes left in this structure, and this would occupy 8 of them for no valid reason. The *only* valid reason to put a zero-initialized field in struct setup_header is if it used by the 16-bit legacy BIOS boot, which is obviously not the case here. This field thus belongs in struct boot_params, not struct setup_header. > > @@ -317,6 +330,12 @@ Protocol: 2.00+ > e.g. 0x0204 for version 2.04, and 0x0a11 for a hypothetical version > 10.17. > > + Up to protocol version 2.13 this information is only read by the > + bootloader. From protocol version 2.14 onwards the bootloader will > + write the used protocol version ored with 0x8000 to the field. The > + used protocol version will be the minimum of the supported protocol > + versions of the bootloader and the kernel. > + Again, this is completely wrong. The version number is communication to the bootloader, which may end up going through multiple stages. Modifying this field breaks this invariant in a not-very-subtle way. Fields in struct setup_header are to be initialized from the image provided in the kernel header. Fields in struct boot_params are to be initialized to zero. There is a field called "sentinel" which attempts to detect broken bootloaders which do not do this correctly; however, when enabling new bootloaders the Right Thing to do is to make sure they adhere to the protocol as defined, rather than pushing a new hack onto the kernel. Thus: 1. Please revert this patch immediately, and destroy any boot loaders which tries to implement this. 2. Add the acpi_rsdp_addr to struct boot_params. 3. DO NOT modify the boot protocol version header field. Instead make sure that the bootloader follows the protocol and zeroes all unknown fields in struct boot_params. 4. Possibly make the kernel panic if it notices that the boot version header has been mucked with, in case some of these boot loaders have already escaped into the field.
