[U-Boot,v4,07/16] efi: sandbox: Add distroboot support

Message ID 20180516154233.21457-8-sjg@chromium.org
State Superseded
Delegated to: Alexander Graf
Headers show
Series
  • efi: Enable basic sandbox support for EFI loader
Related show

Commit Message

Simon Glass May 16, 2018, 3:42 p.m.
With sandbox these values depend on the host system. Let's assume that it
is x86_64 for now.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 include/config_distro_bootcmd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alexander Graf May 24, 2018, 12:32 p.m. | #1
On 16.05.18 17:42, Simon Glass wrote:
> With sandbox these values depend on the host system. Let's assume that it
> is x86_64 for now.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  include/config_distro_bootcmd.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> index 8d5feb3ac77..97d6baab4be 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -246,7 +246,7 @@
>  #elif defined(CONFIG_ARM)
>  #define BOOTENV_EFI_PXE_ARCH "0xa"
>  #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00010:UNDI:003000"
> -#elif defined(CONFIG_X86)
> +#elif defined(CONFIG_X86) || defined(CONFIG_SANDBOX)

Why not

#elif defined(CONFIG_X86) || (defined(CONFIG_SANDBOX) &&
defined(__x86_64__))

and similar for other architectures? That way we should be quite safe in
determining our target architecture, no?


Alex

>  /* Always assume we're running 64bit */
>  #define BOOTENV_EFI_PXE_ARCH "0x7"
>  #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00007:UNDI:003000"
>
Simon Glass June 12, 2018, 5:27 a.m. | #2
Hi Alex,

On 24 May 2018 at 06:32, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 16.05.18 17:42, Simon Glass wrote:
>> With sandbox these values depend on the host system. Let's assume that it
>> is x86_64 for now.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>  include/config_distro_bootcmd.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
>> index 8d5feb3ac77..97d6baab4be 100644
>> --- a/include/config_distro_bootcmd.h
>> +++ b/include/config_distro_bootcmd.h
>> @@ -246,7 +246,7 @@
>>  #elif defined(CONFIG_ARM)
>>  #define BOOTENV_EFI_PXE_ARCH "0xa"
>>  #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00010:UNDI:003000"
>> -#elif defined(CONFIG_X86)
>> +#elif defined(CONFIG_X86) || defined(CONFIG_SANDBOX)
>
> Why not
>
> #elif defined(CONFIG_X86) || (defined(CONFIG_SANDBOX) &&
> defined(__x86_64__))
>
> and similar for other architectures? That way we should be quite safe in
> determining our target architecture, no?

I suspect that would work, although I think it would need to be done
centrally, rather than ad-hoc in files that need to know the sandbox
host architecture.

We are not currently aware of the sandbox host architecture, but I
wonder whether we are going to have to teach the build system about
it. Does U-Boot sandbox actually run on ARM platforms? I have not
tried it.

Regards,
Simon
Alexander Graf June 12, 2018, 5:42 a.m. | #3
On 12.06.18 07:27, Simon Glass wrote:
> Hi Alex,
> 
> On 24 May 2018 at 06:32, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 16.05.18 17:42, Simon Glass wrote:
>>> With sandbox these values depend on the host system. Let's assume that it
>>> is x86_64 for now.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>> Changes in v4: None
>>> Changes in v3: None
>>> Changes in v2: None
>>>
>>>  include/config_distro_bootcmd.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
>>> index 8d5feb3ac77..97d6baab4be 100644
>>> --- a/include/config_distro_bootcmd.h
>>> +++ b/include/config_distro_bootcmd.h
>>> @@ -246,7 +246,7 @@
>>>  #elif defined(CONFIG_ARM)
>>>  #define BOOTENV_EFI_PXE_ARCH "0xa"
>>>  #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00010:UNDI:003000"
>>> -#elif defined(CONFIG_X86)
>>> +#elif defined(CONFIG_X86) || defined(CONFIG_SANDBOX)
>>
>> Why not
>>
>> #elif defined(CONFIG_X86) || (defined(CONFIG_SANDBOX) &&
>> defined(__x86_64__))
>>
>> and similar for other architectures? That way we should be quite safe in
>> determining our target architecture, no?
> 
> I suspect that would work, although I think it would need to be done
> centrally, rather than ad-hoc in files that need to know the sandbox
> host architecture.
> 
> We are not currently aware of the sandbox host architecture, but I
> wonder whether we are going to have to teach the build system about
> it. Does U-Boot sandbox actually run on ARM platforms? I have not
> tried it.

I haven't tried it either, but IMHO it's a bug if it doesn't run :).

I also don't quite understand why CONFIG_SANDBOX contradicts CONFIG_X86.
They probably should both be set for an x86 host system. That way you
wouldn't have to double-check these conditionals all over the code base.


Alex
Simon Glass June 12, 2018, 6:05 a.m. | #4
Hi Alex,

On 11 June 2018 at 23:42, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 12.06.18 07:27, Simon Glass wrote:
>> Hi Alex,
>>
>> On 24 May 2018 at 06:32, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>> On 16.05.18 17:42, Simon Glass wrote:
>>>> With sandbox these values depend on the host system. Let's assume that it
>>>> is x86_64 for now.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>> ---
>>>>
>>>> Changes in v4: None
>>>> Changes in v3: None
>>>> Changes in v2: None
>>>>
>>>>  include/config_distro_bootcmd.h | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
>>>> index 8d5feb3ac77..97d6baab4be 100644
>>>> --- a/include/config_distro_bootcmd.h
>>>> +++ b/include/config_distro_bootcmd.h
>>>> @@ -246,7 +246,7 @@
>>>>  #elif defined(CONFIG_ARM)
>>>>  #define BOOTENV_EFI_PXE_ARCH "0xa"
>>>>  #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00010:UNDI:003000"
>>>> -#elif defined(CONFIG_X86)
>>>> +#elif defined(CONFIG_X86) || defined(CONFIG_SANDBOX)
>>>
>>> Why not
>>>
>>> #elif defined(CONFIG_X86) || (defined(CONFIG_SANDBOX) &&
>>> defined(__x86_64__))
>>>
>>> and similar for other architectures? That way we should be quite safe in
>>> determining our target architecture, no?
>>
>> I suspect that would work, although I think it would need to be done
>> centrally, rather than ad-hoc in files that need to know the sandbox
>> host architecture.
>>
>> We are not currently aware of the sandbox host architecture, but I
>> wonder whether we are going to have to teach the build system about
>> it. Does U-Boot sandbox actually run on ARM platforms? I have not
>> tried it.
>
> I haven't tried it either, but IMHO it's a bug if it doesn't run :).

Well until someone tests it, it doesn't work. Any changes made to
attempt to make it work without testing are just conjecture, IMO.

>
> I also don't quite understand why CONFIG_SANDBOX contradicts CONFIG_X86.
> They probably should both be set for an x86 host system. That way you
> wouldn't have to double-check these conditionals all over the code base.

The point here is that they are separate U-Boot architectures.

What I'm getting at is that in U-Boot we have the concept of a host
architecture and we can use HOSTCC. But with sandbox this takes on a
lot more meaning, since the host architecture is actually the one on
which sandbox is running.

So you could say that we have ARM sandbox or x86_64 sandbox. At
present we have not such concept. I can see that it could be useful as
things get more complicated.

I suppose whoever tries U-Boot sandbox out on ARM first will hit there
problems and send a patch?

Regards,
Simon

Patch

diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
index 8d5feb3ac77..97d6baab4be 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -246,7 +246,7 @@ 
 #elif defined(CONFIG_ARM)
 #define BOOTENV_EFI_PXE_ARCH "0xa"
 #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00010:UNDI:003000"
-#elif defined(CONFIG_X86)
+#elif defined(CONFIG_X86) || defined(CONFIG_SANDBOX)
 /* Always assume we're running 64bit */
 #define BOOTENV_EFI_PXE_ARCH "0x7"
 #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00007:UNDI:003000"