[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-efi
Subject:    Re: [RFC PATCH] x86/efi: remove pointless call to PciIo->Attributes()
From:       Ard Biesheuvel <ard.biesheuvel () linaro ! org>
Date:       2018-06-26 12:57:24
Message-ID: CAKv+Gu-fT+3ZjJDHyLy45gquEFu-7gFhmcecdTXd2bSs66vJGw () mail ! gmail ! com
[Download RAW message or body]

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
[prev in list] [next in list] [prev in thread] [next in thread]