[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Simon Glass]

Hi Alex,

On 14 November 2016 at 14:24, Alexander Graf <agraf at suse.de> wrote:
>
>
> On 14/11/2016 21:58, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 14 November 2016 at 13:46, Alexander Graf <agraf at suse.de> wrote:
>>>
>>>
>>>
>>> On 14/11/2016 21:44, Simon Glass wrote:
>>>>
>>>>
>>>> Hi Alex,
>>>>
>>>> On 11 November 2016 at 23:23, Alexander Graf <agraf at suse.de> wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> Am 11.11.2016 um 17:17 schrieb Simon Glass <sjg at chromium.org>:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>>> On 7 November 2016 at 09:32, Alexander Graf <agraf at suse.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>>> On 07/11/2016 10:46, Simon Glass wrote:
>>>>>>>>
>>>>>>>> Hi Alex,
>>>>>>>>
>>>>>>>>> On 19 October 2016 at 01:09, Alexander Graf <agraf at suse.de> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On 18/10/2016 22:37, Simon Glass wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Alex,
>>>>>>>>>>
>>>>>>>>>>> On 18 October 2016 at 01:14, Alexander Graf <agraf at suse.de>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> It is useful to have a basic sanity check for EFI loader
>>>>>>>>>>>> support.
>>>>>>>>>>>> Add
>>>>>>>>>>>> a
>>>>>>>>>>>> 'bootefi hello' command which loads HelloWord.efi and runs it
>>>>>>>>>>>> under
>>>>>>>>>>>> U-Boot.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> Changes in v3:
>>>>>>>>>>>> - Include a link to the program instead of adding it to the tree
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> So, uh, where is the link?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I put it in the README (see the arm patch).
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I'm really not convinced this command buys us anything yet. I do
>>>>>>>>>>> agree
>>>>>>>>>>> that
>>>>>>>>>>> we want automated testing - but can't we get that using QEMU and
>>>>>>>>>>> a
>>>>>>>>>>> downloadable image file that we pass in as disk and have the
>>>>>>>>>>> distro
>>>>>>>>>>> boot do
>>>>>>>>>>> its magic?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> That seems very heavyweight as a sanity check, although I agree it
>>>>>>>>>> is
>>>>>>>>>> useful.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> It's not really much more heavy weight. The "image file" could
>>>>>>>>> simply
>>>>>>>>> contain your hello world binary. But with this we don't just verify
>>>>>>>>> whether "bootefi" works, but also whether the default boot path
>>>>>>>>> works
>>>>>>>>> ok.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I don't think I understand what you mean by 'image file'. Is it
>>>>>>>> something other than the .efi file? Do you mean a disk image?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Yes. For reasonable test coverage, we should also verify that the
>>>>>>> distro
>>>>>>> defaults wrote a sane boot script that automatically searches for a
>>>>>>> default
>>>>>>> EFI binary in /efi/boot/bootx86.efi on the first partition of all
>>>>>>> devices
>>>>>>> and runs it.
>>>>>>>
>>>>>>> So if we just provide an SD card image or hard disk image to QEMU
>>>>>>> which
>>>>>>> contains a hello world .efi binary as that default boot file, we
>>>>>>> don't
>>>>>>> only
>>>>>>> test whether the "bootefi" command works, but also whether the distro
>>>>>>> boot
>>>>>>> script works.
>>>>>>
>>>>>>
>>>>>>
>>>>>> That's right.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Here I am just making sure that EFI programs can start, print
>>>>>>>>>> output
>>>>>>>>>> and exit. It is a test that we can easily run without a lot of
>>>>>>>>>> overhead, much less than a full distro boot.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Again, I don't think it's much more overhead and I do believe it
>>>>>>>>> gives
>>>>>>>>> us much cleaner separation between responsibilities of code (tests
>>>>>>>>> go
>>>>>>>>> where tests are).
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> You are talking about a functional test, something that tests things
>>>>>>>> end to end. I prefer to at least start with a smaller test. Granted
>>>>>>>> it
>>>>>>>> takes a little more work but it means there are fewer things to hunt
>>>>>>>> through when something goes wrong.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Yes, I personally find unit tests terribly annoying and unproductive
>>>>>>> and
>>>>>>> functional tests very helpful :). And in this case, the effort to
>>>>>>> write
>>>>>>> it
>>>>>>> is about the same for both, just that the functional test actually
>>>>>>> tells you
>>>>>>> that things work or don't work at the end of the day.
>>>>>>>
>>>>>>> With a code base like U-Boot, a simple functional test like the above
>>>>>>> plus
>>>>>>> git bisect should get you to an offending patch very quickly.
>>>>>>
>>>>>>
>>>>>>
>>>>>> This is not a unit test - in fact the EFI stuff has no unit tests. I
>>>>>> suppose if we are trying to find a name this is a small functional
>>>>>> test since it exercises the general functionality.
>>>>>>
>>>>>> I am much keener on small tests than large ones for finding simple
>>>>>> bugs. Of course you can generally bisect to find a bug, but the more
>>>>>> layers of software you need to look for the harder this is.
>>>>>>
>>>>>> We could definitely use a pytest which checks an EFI boot into an
>>>>>> image, but I don't think this obviates the need for a smaller targeted
>>>>>> test like this one.
>>>>>
>>>>>
>>>>>
>>>>> I think arguing over this is moot :). More tests is usually a good
>>>>> thing,
>>>>> so whoever gets to write them gets to push them ;). As long as the
>>>>> licenses
>>>>> are sound at least.
>>>>
>>>>
>>>>
>>>> OK good, well please can you review this at some point?
>>>
>>>
>>>
>>> Review what exactly?
>>
>>
>> I mean the patches. There should be ~14 in your queue.
>
>
> Interesting. I see them on patchwork, but neither in my U-Boot folder, my
> inbox nor my spam folder. I wonder what happened there.

I'm really not sure but I have seen similar things. I will see if I
can figure it out.

Regards,
Simon