'fastboot boot' TPL

'fastboot boot' TPL

[Michael Zimmermann]

>From looking at the code it seems to me that StartImage is called from
TPL_CALLBACK.
According to the Spec StartImage can only be called from <TPL_CALLBACK.

If the current code actually works it means that there are at least 3
problems that should be addressed:
- call StartImage from TPL_APPLICATION
- ASSERT the tpl in LoadImage and StartImage
- ASSERT the tpl in ExitBootServices

Thanks
Michael

Re: 'fastboot boot' TPL

[Ni, Ruiyu]

On 2/28/2018 2:06 PM, Michael Zimmermann wrote:
>  From looking at the code it seems to me that StartImage is called from
> TPL_CALLBACK.
> According to the Spec StartImage can only be called from <TPL_CALLBACK.
> 
> If the current code actually works it means that there are at least 3
> problems that should be addressed:
> - call StartImage from TPL_APPLICATION
> - ASSERT the tpl in LoadImage and StartImage
> - ASSERT the tpl in ExitBootServices
> 
> Thanks
> Michael
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 
NO, LoadImage and StartImage are called at TPL_APPLICATION.

-- 
Thanks,
Ray

Re: 'fastboot boot' TPL

[Michael Zimmermann]

Are you sure?

If you look at this file:
https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c

The DataReady Event is a TPL_CALLBACK event.
>From there the call chain goes as follows:
AcceptCmd -> HandleBoot -> BootAndroidBootImg -> StartEfiApplication ->
"gBS->StartImage"

Thanks
Michael

On Wed, Feb 28, 2018 at 8:29 AM, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:

> On 2/28/2018 2:06 PM, Michael Zimmermann wrote:
>
>>  From looking at the code it seems to me that StartImage is called from
>> TPL_CALLBACK.
>> According to the Spec StartImage can only be called from <TPL_CALLBACK.
>>
>> If the current code actually works it means that there are at least 3
>> problems that should be addressed:
>> - call StartImage from TPL_APPLICATION
>> - ASSERT the tpl in LoadImage and StartImage
>> - ASSERT the tpl in ExitBootServices
>>
>> Thanks
>> Michael
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>> NO, LoadImage and StartImage are called at TPL_APPLICATION.
>
> --
> Thanks,
> Ray
>

Re: 'fastboot boot' TPL

[Andrew Fish]

Violating the spec is undefined behavior. If it works that is bad luck, or good luck depending on your point of view.
> On Feb 27, 2018, at 11:33 PM, Michael Zimmermann <sigmaepsilon92@gmail.com> wrote:
> 
> Are you sure?
> 
> If you look at this file:
> https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
> 
> The DataReady Event is a TPL_CALLBACK event.
> From there the call chain goes as follows:
> AcceptCmd -> HandleBoot -> BootAndroidBootImg -> StartEfiApplication ->
> "gBS->StartImage"
> 
> Thanks
> Michael
> 
>> On Wed, Feb 28, 2018 at 8:29 AM, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>> 
>>> On 2/28/2018 2:06 PM, Michael Zimmermann wrote:
>>> 
>>> From looking at the code it seems to me that StartImage is called from
>>> TPL_CALLBACK.
>>> According to the Spec StartImage can only be called from <TPL_CALLBACK.
>>> 
>>> If the current code actually works it means that there are at least 3
>>> problems that should be addressed:
>>> - call StartImage from TPL_APPLICATION
>>> - ASSERT the tpl in LoadImage and StartImage
>>> - ASSERT the tpl in ExitBootServices
>>> 
>>> Thanks
>>> Michael
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>> 
>>> NO, LoadImage and StartImage are called at TPL_APPLICATION.
>> 
>> --
>> Thanks,
>> Ray
>> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: 'fastboot boot' TPL

[Michael Zimmermann]

I feel like both of you misunderstood my intention.
As I said in my initial mail, I'm not arguing the spec - I know that
StartImage must be called from TPL_APPLICATION.

I'm just discussing a bug inside EmbeddedPkg/Application/AndroidFastboot -
because they actually do call StartImage from TPL_CALLBACK.
As I proposed in my initial mail, we should not only fix that, but also add
tpl ASSERTs inside several BootServices to prevent this from happening in
future.

Thanks
Michael

On Wed, Feb 28, 2018 at 8:42 AM, Andrew Fish <afish@apple.com> wrote:

> Violating the spec is undefined behavior. If it works that is bad luck, or
> good luck depending on your point of view.
>
> Sent from my iPhone
>
> > On Feb 27, 2018, at 11:33 PM, Michael Zimmermann <
> sigmaepsilon92@gmail.com> wrote:
> >
> > Are you sure?
> >
> > If you look at this file:
> > https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/
> Application/AndroidFastboot/AndroidFastbootApp.c
> >
> > The DataReady Event is a TPL_CALLBACK event.
> > From there the call chain goes as follows:
> > AcceptCmd -> HandleBoot -> BootAndroidBootImg -> StartEfiApplication ->
> > "gBS->StartImage"
> >
> > Thanks
> > Michael
> >
> >> On Wed, Feb 28, 2018 at 8:29 AM, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> >>
> >>> On 2/28/2018 2:06 PM, Michael Zimmermann wrote:
> >>>
> >>> From looking at the code it seems to me that StartImage is called from
> >>> TPL_CALLBACK.
> >>> According to the Spec StartImage can only be called from <TPL_CALLBACK.
> >>>
> >>> If the current code actually works it means that there are at least 3
> >>> problems that should be addressed:
> >>> - call StartImage from TPL_APPLICATION
> >>> - ASSERT the tpl in LoadImage and StartImage
> >>> - ASSERT the tpl in ExitBootServices
> >>>
> >>> Thanks
> >>> Michael
> >>> _______________________________________________
> >>> edk2-devel mailing list
> >>> edk2-devel@lists.01.org
> >>> https://lists.01.org/mailman/listinfo/edk2-devel
> >>>
> >>> NO, LoadImage and StartImage are called at TPL_APPLICATION.
> >>
> >> --
> >> Thanks,
> >> Ray
> >>
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
>

Re: 'fastboot boot' TPL

[Ard Biesheuvel]

On 28 February 2018 at 07:56, Michael Zimmermann
<sigmaepsilon92@gmail.com> wrote:
> I feel like both of you misunderstood my intention.
> As I said in my initial mail, I'm not arguing the spec - I know that
> StartImage must be called from TPL_APPLICATION.
>
> I'm just discussing a bug inside EmbeddedPkg/Application/AndroidFastboot -
> because they actually do call StartImage from TPL_CALLBACK.

I think the confusion is a result of the fact that you never mentioned
that particular module/file until now.

> As I proposed in my initial mail, we should not only fix that, but also add
> tpl ASSERTs inside several BootServices to prevent this from happening in
> future.
>

I agree. Did you run into any issues due to this?

Re: 'fastboot boot' TPL

[Michael Zimmermann]

> I agree. Did you run into any issues due to this?
Surprisingly no. I was just trying to understand the fastboot
implementation before I use it on my platform
and was surprised that this works at all. I can imagine that's because this
is supposed to load linux's efistub which probably doesn't do anything but
calling SetVirtualMemoryMap and ExitBootServices.
I can imagine that more complex loaders like the one used for Windows
wouldn't work this way.

On Wed, Feb 28, 2018 at 9:10 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org>
wrote:

> On 28 February 2018 at 07:56, Michael Zimmermann
> <sigmaepsilon92@gmail.com> wrote:
> > I feel like both of you misunderstood my intention.
> > As I said in my initial mail, I'm not arguing the spec - I know that
> > StartImage must be called from TPL_APPLICATION.
> >
> > I'm just discussing a bug inside EmbeddedPkg/Application/AndroidFastboot
> -
> > because they actually do call StartImage from TPL_CALLBACK.
>
> I think the confusion is a result of the fact that you never mentioned
> that particular module/file until now.
>
> > As I proposed in my initial mail, we should not only fix that, but also
> add
> > tpl ASSERTs inside several BootServices to prevent this from happening in
> > future.
> >
>
> I agree. Did you run into any issues due to this?
>

Re: 'fastboot boot' TPL

[Ard Biesheuvel]

On 28 February 2018 at 08:15, Michael Zimmermann
<sigmaepsilon92@gmail.com> wrote:
>> I agree. Did you run into any issues due to this?
> Surprisingly no. I was just trying to understand the fastboot implementation
> before I use it on my platform
> and was surprised that this works at all. I can imagine that's because this
> is supposed to load linux's efistub which probably doesn't do anything but
> calling SetVirtualMemoryMap and ExitBootServices.

The ARM/Linux EFI stub does quite a bit more than that, actually. It
uses the various memory allocation and protocol handling services, to
carve out an allocation for the kernel, initrd and device tree, and to
access the command line, the EFI_RNG_PROTOCOL (if available) and to
interrogate the protocol database for GOP instances.

> I can imagine that more complex loaders like the one used for Windows
> wouldn't work this way.
>

No, and this is indeed something that should be fixed. Any clue as to how?

Re: 'fastboot boot' TPL

[Michael Zimmermann]

Well since the fastboot implementation already is an application (and not a
driver) all we need to do is to use WaitEvent instead of a notify callback.

Once that's fixed I'd add ASSERTs on the current tpl to the relevant API
functions so you know immediately when you try to violate the spec.

On Feb 28, 2018 9:19 AM, "Ard Biesheuvel" <ard.biesheuvel@linaro.org> wrote:

> On 28 February 2018 at 08:15, Michael Zimmermann
> <sigmaepsilon92@gmail.com> wrote:
> >> I agree. Did you run into any issues due to this?
> > Surprisingly no. I was just trying to understand the fastboot
> implementation
> > before I use it on my platform
> > and was surprised that this works at all. I can imagine that's because
> this
> > is supposed to load linux's efistub which probably doesn't do anything
> but
> > calling SetVirtualMemoryMap and ExitBootServices.
>
> The ARM/Linux EFI stub does quite a bit more than that, actually. It
> uses the various memory allocation and protocol handling services, to
> carve out an allocation for the kernel, initrd and device tree, and to
> access the command line, the EFI_RNG_PROTOCOL (if available) and to
> interrogate the protocol database for GOP instances.
>
> > I can imagine that more complex loaders like the one used for Windows
> > wouldn't work this way.
> >
>
> No, and this is indeed something that should be fixed. Any clue as to how?
>

Re: 'fastboot boot' TPL

[Laszlo Ersek]

On 02/28/18 09:19, Ard Biesheuvel wrote:
> On 28 February 2018 at 08:15, Michael Zimmermann
> <sigmaepsilon92@gmail.com> wrote:
>>> I agree. Did you run into any issues due to this?
>> Surprisingly no. I was just trying to understand the fastboot implementation
>> before I use it on my platform
>> and was surprised that this works at all. I can imagine that's because this
>> is supposed to load linux's efistub which probably doesn't do anything but
>> calling SetVirtualMemoryMap and ExitBootServices.
> 
> The ARM/Linux EFI stub does quite a bit more than that, actually. It
> uses the various memory allocation and protocol handling services, to
> carve out an allocation for the kernel, initrd and device tree, and to
> access the command line, the EFI_RNG_PROTOCOL (if available) and to
> interrogate the protocol database for GOP instances.
> 
>> I can imagine that more complex loaders like the one used for Windows
>> wouldn't work this way.
>>
> 
> No, and this is indeed something that should be fixed. Any clue as to how?

- Change the type of the event "ReceiveEvent" from EVT_NOTIFY_SIGNAL to
zero (0)

- remove the DataReady() function

- continue passing "ReceiveEvent" to mTransport->Start()

- in the WaitForEvent() call near the end of FastbootAppEntryPoint(),
also wait for "ReceiveEvent"

- whenever "ReceiveEvent" fires, implement the same logic as seen
originally in DataReady(), but now near the end of FastbootAppEntryPoint().

The idea -- already used with "mFinishedEvent" BTW -- is that an event
can have type 0, which means it can be signaled and waited for without
ever queueing a notification function (at either signaling or waiting).
So mTransport should continue signaling ReceiveEvent whenever data is
ready. But, readiness should be observed in the "main loop", and the
data should be read, parsed and acted upon in that context as well, not
in a callback.

Thanks,
Laszlo