On 24 June 2018 at 15:16, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > Hi Ard, > > > On 23-06-18 23:19, Ard Biesheuvel wrote: >> >> Commit 2c3625cb9fa2 >> >> efi/x86: Fold __setup_efi_pci32() and __setup_efi_pci64() into one >> function >> >> merged the two versions of __setup_efi_pciXX(), without taking into >> account that the 32-bit version used a rather dodgy trick to pass an >> immediate 0 constant as argument for a uint64_t parameter. >> >> The issue is caused by the fact that on x86, UEFI protocol method calls >> are redirected via struct efi_config::call(), which is a variadic >> function, >> and so the compiler has to infer the types of the parameters from the >> arguments rather than from the prototype. As the 32-bit x86 calling >> convention passes arguments via the stack, passing the unqualified >> constant 0 twice is the same as passing 0ULL, which is why the 32-bit >> code in __setup_efi_pci32() contained the following call: >> >> status = efi_early->call(pci->attributes, pci, >> EfiPciIoAttributeOperationGet, 0, 0, >> &attributes); >> >> to invoke this UEFI protocol method: >> >> typedef >> EFI_STATUS >> (EFIAPI *EFI_PCI_IO_PROTOCOL_ATTRIBUTES) ( >> IN EFI_PCI_IO_PROTOCOL *This, >> IN EFI_PCI_IO_PROTOCOL_ATTRIBUTE_OPERATION Operation, >> IN UINT64 Attributes, >> OUT UINT64 *Result OPTIONAL >> ); >> >> After the merge, we inadvertently ended up with this version for both >> 32-bit and 64-bit builds, breaking the latter. >> >> So replace the two zeroes with the explicitly typed constant 0ULL, >> which works as expected on both 32-bit and 64-bit builds. >> >> Reported-by: Wilfried Klaebe <linux-kernel@xxxxxxxxxxxxxxxxxxxxxxxxxx> >> Tested-by: Wilfried Klaebe <linux-kernel@xxxxxxxxxxxxxxxxxxxxxxxxxx> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> >> --- >> >> Wilfried tested the 64-bit build, and I checked the generated assembly >> of a 32-bit build with and without this patch, and they are identical. > > > Ard, thank you for Cc-ing me on this. > > I've tested a 64 bit kernel build on a 32 bit UEFI firmware (so mixed mode) > and this patch causes a reboot loop there. I do get grub (no surprise there > as grub is unchanged), but as soon as the kernel loads the device resets. > > So I think we need some special casing there to deal with a 64 bit kernel > calling into 32 bit UEFI. > OK, so mixed mode rears its ugly hand again :-( Considering we had other weird issues involving uint64_t types with the TPM code just this week, I wonder if this isn't a fundamental problem with the mixed mode thunking, and so I need some help from the x86 gurus (Ingo?) If the thunking code simply maps each 64-bit argument onto a 32-bit stack slot, it is obvious that passing uint64_t type arguments is impossible. So would it be possible to have a efi_config::call() variant that is annotated as expecting the i386 calling convention, and let the compiler handle this? All we'd need to do in the mixed mode thunking code is pushing an additional word [as we do know] for the function pointer. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
