From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Graf Date: Thu, 29 Sep 2016 10:06:55 +0200 Subject: [U-Boot] [PATCH v2 4/8] x86: Tidy up selection of building the EFI stub In-Reply-To: References: <1474838857-25045-1-git-send-email-sjg@chromium.org> <1474838857-25045-4-git-send-email-sjg@chromium.org> <57ECBF11.4080004@suse.de> Message-ID: <57ECCB9F.7040903@suse.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 09/29/2016 09:28 AM, Bin Meng wrote: > Hi Alex, > > On Thu, Sep 29, 2016 at 3:13 PM, Alexander Graf wrote: >> On 09/29/2016 07:37 AM, Bin Meng wrote: >>> Hi Alex, >>> >>> On Thu, Sep 29, 2016 at 1:08 PM, Alexander Graf wrote: >>>> >>>> Am 29.09.2016 um 05:37 schrieb Bin Meng : >>>> >>>> Hi Simon, >>>> >>>> On Wed, Sep 28, 2016 at 10:43 PM, Simon Glass wrote: >>>> >>>> Hi Bin, >>>> >>>> >>>> On 27 September 2016 at 19:23, Bin Meng wrote: >>>> >>>> Hi Simon, >>>> >>>> >>>> On Wed, Sep 28, 2016 at 1:55 AM, Simon Glass wrote: >>>> >>>> Hi Bin, >>>> >>>> >>>> On 26 September 2016 at 20:44, Bin Meng wrote: >>>> >>>> Hi Simon, >>>> >>>> >>>> On Tue, Sep 27, 2016 at 8:35 AM, Simon Glass wrote: >>>> >>>> Hi Bin, >>>> >>>> >>>> On 26 September 2016 at 02:50, Bin Meng wrote: >>>> >>>> Hi Simon, >>>> >>>> >>>> On Mon, Sep 26, 2016 at 5:27 AM, Simon Glass 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 >>>> >>>> --- >>>> >>>> >>>> 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 :). > 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. Alex