[U-Boot] [PATCH v2 4/8] x86: Tidy up selection of building the EFI stub

[Bin Meng]

Hi Alex,

On Thu, Sep 29, 2016 at 4:06 PM, Alexander Graf <agraf at suse.de> wrote:
> On 09/29/2016 09:28 AM, Bin Meng wrote:
>>
>> Hi Alex,
>>
>> On Thu, Sep 29, 2016 at 3:13 PM, Alexander Graf <agraf at suse.de> wrote:
>>>
>>> On 09/29/2016 07:37 AM, Bin Meng wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On Thu, Sep 29, 2016 at 1:08 PM, Alexander Graf <agraf at suse.de> wrote:
>>>>>
>>>>>
>>>>> Am 29.09.2016 um 05:37 schrieb Bin Meng <bmeng.cn at gmail.com>:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>> On Wed, Sep 28, 2016 at 10:43 PM, Simon Glass <sjg at chromium.org> wrote:
>>>>>
>>>>> Hi Bin,
>>>>>
>>>>>
>>>>> On 27 September 2016 at 19:23, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>>
>>>>> On Wed, Sep 28, 2016 at 1:55 AM, Simon Glass <sjg at chromium.org> wrote:
>>>>>
>>>>> Hi Bin,
>>>>>
>>>>>
>>>>> On 26 September 2016 at 20:44, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>>
>>>>> On Tue, Sep 27, 2016 at 8:35 AM, Simon Glass <sjg at chromium.org> wrote:
>>>>>
>>>>> Hi Bin,
>>>>>
>>>>>
>>>>> On 26 September 2016 at 02:50, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>>
>>>>> On Mon, Sep 26, 2016 at 5:27 AM, Simon Glass <sjg at chromium.org> wrote:
>>>>>
>>>>> At present we use a CONFIG option in efi.h to determine whether we are
>>>>>
>>>>> building the EFI stub or not. This means that the same header cannot be
>>>>>
>>>>> used for EFI_LOADER support. The CONFIG option will be enabled for the
>>>>>
>>>>> whole build, even when not building the stub.
>>>>>
>>>>>
>>>>> Use a different define instead, set up just for the files that make up
>>>>> the
>>>>>
>>>>> stub.
>>>>>
>>>>>
>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>>>
>>>>> ---
>>>>>
>>>>>
>>>>> Changes in v2:
>>>>>
>>>>> - Add new patch to tidy up selection of building the EFI stub
>>>>>
>>>>>
>>>>> include/efi.h    | 7 +++++--
>>>>>
>>>>> lib/efi/Makefile | 4 ++--
>>>>>
>>>>> 2 files changed, 7 insertions(+), 4 deletions(-)
>>>>>
>>>>>
>>>>> diff --git a/include/efi.h b/include/efi.h
>>>>>
>>>>> index d07187c..3d58780 100644
>>>>>
>>>>> --- a/include/efi.h
>>>>>
>>>>> +++ b/include/efi.h
>>>>>
>>>>> @@ -30,8 +30,11 @@ struct efi_device_path;
>>>>>
>>>>>
>>>>> #define EFI_BITS_PER_LONG      BITS_PER_LONG
>>>>>
>>>>>
>>>>> -/* With 64-bit EFI stub, EFI_BITS_PER_LONG has to be 64 */
>>>>>
>>>>> -#ifdef CONFIG_EFI_STUB_64BIT
>>>>>
>>>>> +/*
>>>>>
>>>>> + * 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)
>>>>>
>>>>>
>>>>> I don't understand why this is needed?
>>>>>
>>>>>
>>>>> If building the 64-bit EFI stub, we need to use 64-bit ints for the
>>>>>
>>>>> stub, but 32-bits for the rest of U-Boot. So this header gets used
>>>>>
>>>>> both ways.
>>>>>
>>>>>
>>>>>
>>>>> For 32-bit EFI stub or U-Boot itself, EFI_BITS_PER_LONG is defined as
>>>>>
>>>>> BITS_PER_LONG which is 32.
>>>>>
>>>>>
>>>>> Yes that's right. But in the case of the stub, it can be different
>>>>>
>>>>> from U-Boot itself. This case takes care of that.
>>>>>
>>>>>
>>>>>
>>>>> Sorry but I still don't get it. What's broken without this change?
>>>>>
>>>>>
>>>>> When building U-Boot with CONFIG_EFI_STUB_64BIT enabled, at present
>>>>>
>>>>> EFI_BITS_PER_LONG will be 64.
>>>>>
>>>>>
>>>>> This is fine for building the stub.
>>>>>
>>>>>
>>>>>
>>>>> Yes
>>>>>
>>>>> But for building U-Boot, we still want it to be 32.
>>>>>
>>>>>
>>>>>
>>>>> Yes
>>>>>
>>>>> At present the code overrides EFI_BITS_PER_LONG, setting it to 64 if
>>>>>
>>>>> CONFIG_EFI_STUB_64BIT is enabled.
>>>>>
>>>>>
>>>>> This means that EFI_LOADER support does not build properly, since it
>>>>>
>>>>> uses 64 instead of 32.
>>>>>
>>>>>
>>>>>
>>>>> So you want CONFIG_EFI_STUB_64BIT and CONFIG_EFI_LOADER both be
>>>>> defined? I don't think it is a valid configuration.
>>>>>
>>>>>
>>>>> Why not?
>>>>>
>>>> So the board has a 64-bit UEFI BIOS, and with CONFIG_EFI_STUB_64BIT we
>>>> build U-Boot as a 64-bit UEFI payload and let the UEFI BIOS boot the
>>>> payload (U-Boot), then with CONFIG_EFI_LOADER we are trying to provide
>>>> the UEFI runtime environment within the U-Boot. What value are we
>>>> looking for? This is asking for troubles.
>>>
>>>
>>> Why is this asking for trouble? The inner uefi payload has no idea that
>>> the
>>> outer uefi firmware exists. It only ever talks to u-boot. I would argue
>>> the
>>> other way around: If we can't make it work, we have a layering problem.
>>>
>> This shows no value to me. In the end, providing EFI loader in the
>> U-Boot is to load some EFI apps. But this can be done from the
>> original UEFI BIOS without the need to have the middle-stage U-Boot
>> payload.
>
>
> You could say the same about running U-Boot on EFI. You can as well just
> load your payload from the original uEFI firmware :).

Wait, they are different things. EFI loader in U-Boot assumes U-Boot
is the bootloader and we want a EFI runtime environment to load EFI
payload without the need to have a full UEFI BIOS. I see value in
supporting EFI loader in U-Boot.

>
>> What layering problem do we want to fix here? Are you saying
>> testing EFI loader in the U-Boot is not enough, so that we should
>> support such configuration for additional testing?
>
>
> I'm saying that I don't see anything that speaks against it working, so why
> should we disable it? At least for prototyping it's a convenient option to
> have and in general divergence is a bad thing.

But this one, value-wise, no, but yes, technically we can make such
configuration work :)

Regards,
Bin