[U-Boot] [PATCH 14/18] efi: stub: Pass EFI GOP information to U-Boot payload

[Bin Meng]

Hi Alex,

On Mon, Jun 11, 2018 at 4:33 PM, Alexander Graf <agraf at suse.de> wrote:
> On 06/11/2018 09:44 AM, Bin Meng wrote:
>>
>> Hi Alex,
>>
>> On Mon, Jun 11, 2018 at 3:34 PM, Alexander Graf <agraf at suse.de> wrote:
>>>
>>> On 06/11/2018 08:01 AM, Bin Meng wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On Mon, Jun 11, 2018 at 1:52 PM, Alexander Graf <agraf at suse.de> wrote:
>>>>>
>>>>>
>>>>> On 11.06.18 01:29, Bin Meng wrote:
>>>>>>
>>>>>> On Mon, Jun 11, 2018 at 3:16 AM, Alexander Graf <agraf at suse.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 10.06.18 15:25, Bin Meng wrote:
>>>>>>>>
>>>>>>>> If UEFI BIOS has the graphics output protocol (GOP), let's pass its
>>>>>>>> information to U-Boot payload so that U-Boot can utilize it (eg:
>>>>>>>> an EFI framebuffer driver).
>>>>>>>>
>>>>>>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>>>>>>>
>>>>>>> Why can't the FB drive determine all of this on its own and just fail
>>>>>>> probe if no GOP protocol can be found?
>>>>>>>
>>>>>> It cannot. Once U-Boot payload is running, the boot services are gone.
>>>>>> There is no way to determine the GOP protocol.
>>>>>
>>>>> Interesting. Is there a particular reason you're not preserving boot
>>>>> services?
>>>>>
>>>> This is EFI payload support with CONFIG_EFI_STUB. Preserving boot
>>>> services is EFI application, with CONFIG_EFI_APP. For example, see
>>>> serial_efi.c which is the serial driver that uses EFI's boot services
>>>> to output characters on the serial port.
>>>
>>>
>>> Oh, I see. That makes sense now.
>>>
>>> Do people actually need CONFIG_EFI_STUB then?
>>
>> I think you wanted to say: Do people actually need CONFIG_EFI_APP
>> then? CONFIG_EFI_STUB is more useful than CONFIG_EFI_APP. The
>> application support is very limited.
>>
>> As the README.u-boot_on_efi says:
>>
>> Running U-Boot on EFI is useful in several situations:
>>
>> - You have EFI running on a board but U-Boot does not natively support it
>> fully yet. You can boot into U-Boot from EFI and use that until U-Boot is
>> fully ported
>>
>> - You need to use an EFI implementation (e.g. UEFI) because your vendor
>> requires it in order to provide support
>>
>> - You plan to use coreboot to boot into U-Boot but coreboot support does
>> not currently exist for your platform. In the meantime you can use U-Boot
>> on EFI and then move to U-Boot on coreboot when ready
>>
>> - You use EFI but want to experiment with a simpler alternative like
>> U-Boot
>>
>> Right now, I have one Intel SkyLake board, and before I get a native
>> U-Boot up and running as the 1st stage bootloader on this board, I can
>> run U-Boot as the EFI payload to experiment something, which is very
>> handy.
>
>
> Oh, I see. So it's mostly used as bringup aid. In that case I agree that the
> stub bit makes a lot of sense.
>
> The one thing that really bites us with the stub and different bitness is
> when you want to use efi_api.h again from within U-Boot. The obvious example
> is a 32bit U-Boot built as 64bit payload, allowing 32bit UEFI applications
> to run inside.
>
> Do you think we could just agree on efi.h to not consume any stub config
> options and just determine things from its build environment instead? That
> way it automatically adapts according to the -m32/-m64 build flag for the
> stub and we would not need to worry about the stub in generic code.
>

Great, this way is smarter!

> Something like the patch below? If yes, I'll resend it with proper
> indentation :).
>

Yes, and I belive we will also need remove -DEFI_STUB in lib/efi/Makefile.

>
> diff --git a/include/efi.h b/include/efi.h
> index 98bddbac1a..5e1e8ac78c 100644
> --- a/include/efi.h
> +++ b/include/efi.h
> @@ -19,12 +19,12 @@
>  #include <linux/string.h>
>  #include <linux/types.h>
>
> -#if CONFIG_EFI_STUB_64BIT || (!defined(CONFIG_EFI_STUB) &&
> defined(__x86_64__))
> -/* EFI uses the Microsoft ABI which is not the default for GCC */
> +/* EFI on x86_64 uses the Microsoft ABI which is not the default for GCC */
> +#ifdef __x86_64__
>  #define EFIAPI __attribute__((ms_abi))
>  #else
>  #define EFIAPI asmlinkage
> -#endif
> +#endif /* __x86_64__ */
>
>  struct efi_device_path;
>
> @@ -32,16 +32,7 @@ typedef struct {
>      u8 b[16];
>  } efi_guid_t;
>
> -#define EFI_BITS_PER_LONG    BITS_PER_LONG
> -
> -/*
> - * With 64-bit EFI stub, EFI_BITS_PER_LONG has to be 64. EFI_STUB is set
> - * in lib/efi/Makefile, when building the stub.
> - */
> -#if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB)
> -#undef EFI_BITS_PER_LONG
> -#define EFI_BITS_PER_LONG    64
> -#endif
> +#define EFI_BITS_PER_LONG    (sizeof(long) * 8)
>
>  /* Bit mask for EFI status code with error */
>  #define EFI_ERROR_MASK (1UL << (EFI_BITS_PER_LONG - 1))
>

Regards,
Bin