| Message ID | 20180624172922.32698-1-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] x86/efi: remove pointless call to PciIo->Attributes() | expand |
Am Sun, Jun 24, 2018 at 07:29:22PM +0200 schrieb Ard Biesheuvel: [...] > As this was the only use of the attributes field, we can remove > the call to PciIo->Attributes() entirely, which is especially > nice because its prototype involves uint64_t type by-value > arguments which the EFI mixed mode has trouble dealing with. > > Cc: Wilfried Klaebe <linux-kernel@lebenslange-mailadresse.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Lukas Wunner <lukas@wunner.de> > Cc: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Works for me, radeon driver finds ATOM BIOS. Tested-by: Wilfried Klaebe <linux-kernel@lebenslange-mailadresse.de> Regards, Wilfried -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 24-06-18 19:29, Ard Biesheuvel wrote: > When it was first introduced, the EFI stub code that copies the > contents of PCI option ROMs originally only intended to do so if > the EFI_PCI_IO_ATTRIBUTE_EMBEDDED_ROM attribute was *not* set. > > The reason was that the UEFI spec permits PCI option ROM images > to be provided by the platform directly, rather than via the ROM > BAR, and in this case, the OS can only access them at runtime if > they are preserved at boot time by copying them from the areas > described by PciIo->RomImage and PciIo->RomSize. > > However, it implemented this check erroneously, as can be seen in > commit dd5fc854de5fd ("EFI: Stash ROMs if they're not in the PCI > BAR"): > > if (!attributes & EFI_PCI_IO_ATTRIBUTE_EMBEDDED_ROM) > continue; > > and given that the numeric value of EFI_PCI_IO_ATTRIBUTE_EMBEDDED_ROM > is 0x4000, this condition never becomes true, and so the option ROMs > were copied unconditionally. > > This was spotted and 'fixed' by commit 886d751a2ea99a160 > ("x86, efi: correct precedence of operators in setup_efi_pci"), > but inadvertently inverted the logic at the same time, defeating > the purpose of the code, since it now only preserves option ROM > images that can be read from the ROM BAR as well. > > Unsurprisingly, this broke some systems, and so the check was removed > entirely in commit 739701888f5d ("x86, efi: remove attribute check > from setup_efi_pci"). > > It is debatable whether this check should have been included in the > first place, since the option ROM image provided to the UEFI driver by > the firmware may be different from the one that is actually present in > the card's flash ROM, and so whatever PciIo->RomImage points at should > be preferred regardless of whether the attribute is set. > > As this was the only use of the attributes field, we can remove > the call to PciIo->Attributes() entirely, which is especially > nice because its prototype involves uint64_t type by-value > arguments which the EFI mixed mode has trouble dealing with. > > Cc: Wilfried Klaebe <linux-kernel@lebenslange-mailadresse.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Lukas Wunner <lukas@wunner.de> > Cc: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> I can confirm that this fixes the mixed mode UEFI boot issues: Tested-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > > This should fix the mixed mode issue reported by Hans after my fix > for 64-bit native mode was included in v4.18-rc2. > > arch/x86/boot/compressed/eboot.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c > index e57665b4ba1c..e98522ea6f09 100644 > --- a/arch/x86/boot/compressed/eboot.c > +++ b/arch/x86/boot/compressed/eboot.c > @@ -114,18 +114,12 @@ __setup_efi_pci(efi_pci_io_protocol_t *pci, struct pci_setup_rom **__rom) > struct pci_setup_rom *rom = NULL; > efi_status_t status; > unsigned long size; > - uint64_t attributes, romsize; > + uint64_t romsize; > void *romimage; > > - status = efi_call_proto(efi_pci_io_protocol, attributes, pci, > - EfiPciIoAttributeOperationGet, 0ULL, > - &attributes); > - if (status != EFI_SUCCESS) > - return status; > - > /* > - * Some firmware images contain EFI function pointers at the place where the > - * romimage and romsize fields are supposed to be. Typically the EFI > + * Some firmware images contain EFI function pointers at the place where > + * the romimage and romsize fields are supposed to be. Typically the EFI > * code is mapped at high addresses, translating to an unrealistically > * large romsize. The UEFI spec limits the size of option ROMs to 16 > * MiB so we reject any ROMs over 16 MiB in size to catch this. > -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26 June 2018 at 14:23, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 24-06-18 19:29, Ard Biesheuvel wrote: >> >> When it was first introduced, the EFI stub code that copies the >> contents of PCI option ROMs originally only intended to do so if >> the EFI_PCI_IO_ATTRIBUTE_EMBEDDED_ROM attribute was *not* set. >> >> The reason was that the UEFI spec permits PCI option ROM images >> to be provided by the platform directly, rather than via the ROM >> BAR, and in this case, the OS can only access them at runtime if >> they are preserved at boot time by copying them from the areas >> described by PciIo->RomImage and PciIo->RomSize. >> >> However, it implemented this check erroneously, as can be seen in >> commit dd5fc854de5fd ("EFI: Stash ROMs if they're not in the PCI >> BAR"): >> >> if (!attributes & EFI_PCI_IO_ATTRIBUTE_EMBEDDED_ROM) >> continue; >> >> and given that the numeric value of EFI_PCI_IO_ATTRIBUTE_EMBEDDED_ROM >> is 0x4000, this condition never becomes true, and so the option ROMs >> were copied unconditionally. >> >> This was spotted and 'fixed' by commit 886d751a2ea99a160 >> ("x86, efi: correct precedence of operators in setup_efi_pci"), >> but inadvertently inverted the logic at the same time, defeating >> the purpose of the code, since it now only preserves option ROM >> images that can be read from the ROM BAR as well. >> >> Unsurprisingly, this broke some systems, and so the check was removed >> entirely in commit 739701888f5d ("x86, efi: remove attribute check >> from setup_efi_pci"). >> >> It is debatable whether this check should have been included in the >> first place, since the option ROM image provided to the UEFI driver by >> the firmware may be different from the one that is actually present in >> the card's flash ROM, and so whatever PciIo->RomImage points at should >> be preferred regardless of whether the attribute is set. >> >> As this was the only use of the attributes field, we can remove >> the call to PciIo->Attributes() entirely, which is especially >> nice because its prototype involves uint64_t type by-value >> arguments which the EFI mixed mode has trouble dealing with. >> >> Cc: Wilfried Klaebe <linux-kernel@lebenslange-mailadresse.de> >> Cc: Ingo Molnar <mingo@redhat.com> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Cc: Lukas Wunner <lukas@wunner.de> >> Cc: Hans de Goede <hdegoede@redhat.com> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > I can confirm that this fixes the mixed mode UEFI boot issues: > > Tested-by: Hans de Goede <hdegoede@redhat.com> > Thanks all Ingo, mind applying this directly to tip:efi/urgent? (with T-bs from Wilfried and Hans) Thanks, Ard. >> --- >> >> This should fix the mixed mode issue reported by Hans after my fix >> for 64-bit native mode was included in v4.18-rc2. >> >> arch/x86/boot/compressed/eboot.c | 12 +++--------- >> 1 file changed, 3 insertions(+), 9 deletions(-) >> >> diff --git a/arch/x86/boot/compressed/eboot.c >> b/arch/x86/boot/compressed/eboot.c >> index e57665b4ba1c..e98522ea6f09 100644 >> --- a/arch/x86/boot/compressed/eboot.c >> +++ b/arch/x86/boot/compressed/eboot.c >> @@ -114,18 +114,12 @@ __setup_efi_pci(efi_pci_io_protocol_t *pci, struct >> pci_setup_rom **__rom) >> struct pci_setup_rom *rom = NULL; >> efi_status_t status; >> unsigned long size; >> - uint64_t attributes, romsize; >> + uint64_t romsize; >> void *romimage; >> - status = efi_call_proto(efi_pci_io_protocol, attributes, pci, >> - EfiPciIoAttributeOperationGet, 0ULL, >> - &attributes); >> - if (status != EFI_SUCCESS) >> - return status; >> - >> /* >> - * Some firmware images contain EFI function pointers at the place >> where the >> - * romimage and romsize fields are supposed to be. Typically the >> EFI >> + * Some firmware images contain EFI function pointers at the place >> where >> + * the romimage and romsize fields are supposed to be. Typically >> the EFI >> * code is mapped at high addresses, translating to an >> unrealistically >> * large romsize. The UEFI spec limits the size of option ROMs to >> 16 >> * MiB so we reject any ROMs over 16 MiB in size to catch this. >> > -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index e57665b4ba1c..e98522ea6f09 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -114,18 +114,12 @@ __setup_efi_pci(efi_pci_io_protocol_t *pci, struct pci_setup_rom **__rom) struct pci_setup_rom *rom = NULL; efi_status_t status; unsigned long size; - uint64_t attributes, romsize; + uint64_t romsize; void *romimage; - status = efi_call_proto(efi_pci_io_protocol, attributes, pci, - EfiPciIoAttributeOperationGet, 0ULL, - &attributes); - if (status != EFI_SUCCESS) - return status; - /* - * Some firmware images contain EFI function pointers at the place where the - * romimage and romsize fields are supposed to be. Typically the EFI + * Some firmware images contain EFI function pointers at the place where + * the romimage and romsize fields are supposed to be. Typically the EFI * code is mapped at high addresses, translating to an unrealistically * large romsize. The UEFI spec limits the size of option ROMs to 16 * MiB so we reject any ROMs over 16 MiB in size to catch this.
When it was first introduced, the EFI stub code that copies the contents of PCI option ROMs originally only intended to do so if the EFI_PCI_IO_ATTRIBUTE_EMBEDDED_ROM attribute was *not* set. The reason was that the UEFI spec permits PCI option ROM images to be provided by the platform directly, rather than via the ROM BAR, and in this case, the OS can only access them at runtime if they are preserved at boot time by copying them from the areas described by PciIo->RomImage and PciIo->RomSize. However, it implemented this check erroneously, as can be seen in commit dd5fc854de5fd ("EFI: Stash ROMs if they're not in the PCI BAR"): if (!attributes & EFI_PCI_IO_ATTRIBUTE_EMBEDDED_ROM) continue; and given that the numeric value of EFI_PCI_IO_ATTRIBUTE_EMBEDDED_ROM is 0x4000, this condition never becomes true, and so the option ROMs were copied unconditionally. This was spotted and 'fixed' by commit 886d751a2ea99a160 ("x86, efi: correct precedence of operators in setup_efi_pci"), but inadvertently inverted the logic at the same time, defeating the purpose of the code, since it now only preserves option ROM images that can be read from the ROM BAR as well. Unsurprisingly, this broke some systems, and so the check was removed entirely in commit 739701888f5d ("x86, efi: remove attribute check from setup_efi_pci"). It is debatable whether this check should have been included in the first place, since the option ROM image provided to the UEFI driver by the firmware may be different from the one that is actually present in the card's flash ROM, and so whatever PciIo->RomImage points at should be preferred regardless of whether the attribute is set. As this was the only use of the attributes field, we can remove the call to PciIo->Attributes() entirely, which is especially nice because its prototype involves uint64_t type by-value arguments which the EFI mixed mode has trouble dealing with. Cc: Wilfried Klaebe <linux-kernel@lebenslange-mailadresse.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Lukas Wunner <lukas@wunner.de> Cc: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- This should fix the mixed mode issue reported by Hans after my fix for 64-bit native mode was included in v4.18-rc2. arch/x86/boot/compressed/eboot.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) -- 2.11.0 -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html