[Xen-devel] EFI development issues

[Xen-devel] EFI development issues

[Andrew Cooper]

Hello,

My recent boot pagetable changes have caused me to work with the EFI
build of Xen rather more than previously.

First, there is a dependency tracking bug in the build system.  Edits to
xen/arch/x86/efi/efi-boot.h don't cause xen.efi to be regenerated.  From
what I can tell, the file doesn't even get recompiled, because syntax
errors even go unnoticed.

Second, and the main point of the email.

The EFI code has some logic which does:

if ( !base_video )
{
    ...

    if ( best != StdOut->Mode->Mode )
        StdOut->SetMode(StdOut, best);
}

just before printing out the Xen banner.  This has a side effect of
causing all further use of StdOut/StdErr to cease working, and
interfering completely with debugging activities.  (Waiting for a
keypress on StdIn however does work, which is how I eventually diagnosed
that it was an output problem.)  Skipping this logic allows debugging to
work.

The code appeared in bf6501a62 "x86-64: EFI boot code" and has no
specific description of what it is doing and/or trying to achieve.

It is also not entirely clear why it is gated on having a cfg file in
the first place (c/s ,c38cf865ec8, not that there is adequate context
for why) or why there is a Xen command line argument "-basevideo"
introduced in the beginning to skip this behaviour.

As a point of reference, I don't see Linux making any SetMode calls.

What is the purpose of changing to a different mode?  Certainly as far
as serial consoles go, sticking with the mode the loader uses certainly
feels like a safer option.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] EFI development issues

[Jan Beulich]

On 13.01.2020 17:02, Andrew Cooper wrote:
> My recent boot pagetable changes have caused me to work with the EFI
> build of Xen rather more than previously.
> 
> First, there is a dependency tracking bug in the build system.  Edits to
> xen/arch/x86/efi/efi-boot.h don't cause xen.efi to be regenerated.  From
> what I can tell, the file doesn't even get recompiled, because syntax
> errors even go unnoticed.

Not an issue here, I've just now tried it out. .boot.o.d also
correctly names the file here.

> Second, and the main point of the email.
> 
> The EFI code has some logic which does:
> 
> if ( !base_video )
> {
>     ...
> 
>     if ( best != StdOut->Mode->Mode )
>         StdOut->SetMode(StdOut, best);
> }
> 
> just before printing out the Xen banner.  This has a side effect of
> causing all further use of StdOut/StdErr to cease working, and
> interfering completely with debugging activities.

Interesting, and certainly unintended. Obviously the "normal" output
works (for me at least, with or without serial console, albeit I
don't think I've ever tried headless, i.e. _just_ a serial console),
so it's not exactly clear to me what other "debugging activities" it
may interfere with. A broken StdOut / StdErr protocol implementation
in the firmware?

>  (Waiting for a
> keypress on StdIn however does work, which is how I eventually diagnosed
> that it was an output problem.)  Skipping this logic allows debugging to
> work.

As should then do -basevideo.

> The code appeared in bf6501a62 "x86-64: EFI boot code" and has no
> specific description of what it is doing and/or trying to achieve.

efi_console_set_mode() is simple enough I think: It tries to maximize
screen dimensions. (There's some historical context here, as the
code wasn't written from scratch: Serial consoles often weren't
available when colleagues of mine and I did some of the original EFI
enabling work for a long canceled project. Plus there we had a rather
better (tm) kernel debugger, wanting to utilize as high resolution a
screen as possible to show as much useful information as possible at
any point in time.)

> It is also not entirely clear why it is gated on having a cfg file in
> the first place (c/s ,c38cf865ec8, not that there is adequate context
> for why)

The description of the cited commit is clear enough, isn't it?
This is just the same distinction (just placed differently) for
Arm as that between efi_start() (doing most of this stuff) and
efi_multiboot2() (not doing so, in particular the command line
parsing, and e.g. not even providing a means to suppress the
call to efi_console_set_mode()).

For anything beyond this I have to defer to the Arm folks, who
wanted it this way.

> or why there is a Xen command line argument "-basevideo"
> introduced in the beginning to skip this behaviour.

Traditionally video mode setting had its problems, hence we
anticipated there may be problems also with EFI doing so. As a
result we wanted, from the very beginning, a simply means to
get past any such.

> As a point of reference, I don't see Linux making any SetMode calls.

Well, if I'm not mistaken Xen's support for booting as an EFI
application predates Linux'es by quite a bit, so there was nothing
to reference there. As said, the origin of this code is elsewhere.

> What is the purpose of changing to a different mode?  Certainly as far
> as serial consoles go, sticking with the mode the loader uses certainly
> feels like a safer option.

Does a serial console report a "resolution" in the first place? And
if we were able to (sufficiently easily) tell video from serial
console, how would we deal with the case of StdOut / StdErr being
multiplexed to both?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] EFI development issues

[Andrew Cooper]

On 13/01/2020 16:46, Jan Beulich wrote:
> On 13.01.2020 17:02, Andrew Cooper wrote:
>> My recent boot pagetable changes have caused me to work with the EFI
>> build of Xen rather more than previously.
>>
>> First, there is a dependency tracking bug in the build system.  Edits to
>> xen/arch/x86/efi/efi-boot.h don't cause xen.efi to be regenerated.  From
>> what I can tell, the file doesn't even get recompiled, because syntax
>> errors even go unnoticed.
> Not an issue here, I've just now tried it out. .boot.o.d also
> correctly names the file here.

My .boot.o.d looks plausible as well, but there is clearly an issue
somewhere.  It is completely reliable on a Debian system with default
make and toolchain.

>
>> Second, and the main point of the email.
>>
>> The EFI code has some logic which does:
>>
>> if ( !base_video )
>> {
>>     ...
>>
>>     if ( best != StdOut->Mode->Mode )
>>         StdOut->SetMode(StdOut, best);
>> }
>>
>> just before printing out the Xen banner.  This has a side effect of
>> causing all further use of StdOut/StdErr to cease working, and
>> interfering completely with debugging activities.
> Interesting, and certainly unintended. Obviously the "normal" output
> works (for me at least, with or without serial console, albeit I
> don't think I've ever tried headless, i.e. _just_ a serial console),
> so it's not exactly clear to me what other "debugging activities" it
> may interfere with. A broken StdOut / StdErr protocol implementation
> in the firmware?

I have an HTML5 virtual KVM to the box.  Looking at it as well as the
serial (which is ultimately the same IPMI controller), the VGA screen
does expand its dimensions a little and show the debugging which is
missing on the serial.

>
>>   (Waiting for a
>> keypress on StdIn however does work, which is how I eventually diagnosed
>> that it was an output problem.)  Skipping this logic allows debugging to
>> work.
> As should then do -basevideo.

That is another observation.  It is very confusing having command line
parameters to xen.efi which are different to command line parameters to
Xen.  The parameters to xen.efi aren't documented anywhere, as far as I
can tell.

>
>> The code appeared in bf6501a62 "x86-64: EFI boot code" and has no
>> specific description of what it is doing and/or trying to achieve.
> efi_console_set_mode() is simple enough I think: It tries to maximize
> screen dimensions. (There's some historical context here, as the
> code wasn't written from scratch: Serial consoles often weren't
> available when colleagues of mine and I did some of the original EFI
> enabling work for a long canceled project. Plus there we had a rather
> better (tm) kernel debugger, wanting to utilize as high resolution a
> screen as possible to show as much useful information as possible at
> any point in time.)
>
>> It is also not entirely clear why it is gated on having a cfg file in
>> the first place (c/s ,c38cf865ec8, not that there is adequate context
>> for why)
> The description of the cited commit is clear enough, isn't it?

It states the changes being made.  It doesn't explain why this is a
helpful change to make.

> This is just the same distinction (just placed differently) for
> Arm as that between efi_start() (doing most of this stuff) and
> efi_multiboot2() (not doing so, in particular the command line
> parsing, and e.g. not even providing a means to suppress the
> call to efi_console_set_mode()).
>
> For anything beyond this I have to defer to the Arm folks, who
> wanted it this way.

Hmm.  This is need of abstracting out, so we don't have two totally
different ways of only doing a subset of EFI setup when actually started
via multiboot2.

>> or why there is a Xen command line argument "-basevideo"
>> introduced in the beginning to skip this behaviour.
> Traditionally video mode setting had its problems, hence we
> anticipated there may be problems also with EFI doing so. As a
> result we wanted, from the very beginning, a simply means to
> get past any such.
>
>> As a point of reference, I don't see Linux making any SetMode calls.
> Well, if I'm not mistaken Xen's support for booting as an EFI
> application predates Linux'es by quite a bit, so there was nothing
> to reference there. As said, the origin of this code is elsewhere.

I wasn't going for heritage.  Simply for "what do other things in a
similar position to Xen do".

>
>> What is the purpose of changing to a different mode?  Certainly as far
>> as serial consoles go, sticking with the mode the loader uses certainly
>> feels like a safer option.
> Does a serial console report a "resolution" in the first place?

How would I find out?

> And
> if we were able to (sufficiently easily) tell video from serial
> console, how would we deal with the case of StdOut / StdErr being
> multiplexed to both?

In my case they are multiplexed, and all work fine before set_mode() call.

After set_mode(), VGA still works, while serial sees nothing further on
StdOut/Stderr (pre-existing lines are still in place).

StdIn still works for both as well.  I had inserted a wait-for-keypress,
in case there was a buffering issue causing the StdOut content not to be
emitted before the triple fault, and ended up with no debugging on the
serial, and a system which triple faulted once it had seen a keypress.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] EFI development issues

[Jan Beulich]

On 16.01.2020 21:28, Andrew Cooper wrote:
> On 13/01/2020 16:46, Jan Beulich wrote:
>> On 13.01.2020 17:02, Andrew Cooper wrote:
>>>   (Waiting for a
>>> keypress on StdIn however does work, which is how I eventually diagnosed
>>> that it was an output problem.)  Skipping this logic allows debugging to
>>> work.
>> As should then do -basevideo.
> 
> That is another observation.  It is very confusing having command line
> parameters to xen.efi which are different to command line parameters to
> Xen.  The parameters to xen.efi aren't documented anywhere, as far as I
> can tell.

Well, if "xen.efi -help" doesn't count, then yes, this may want adding
to efi.pandoc. The -cfg option is mentioned there (albeit not in a
systematic manner), and the only other possibly useful option that was
there from the beginning was -basevideo; -mapbs appeared only later.
When creating that doc, focus was clearly on the commonly used aspects.

>>> What is the purpose of changing to a different mode?  Certainly as far
>>> as serial consoles go, sticking with the mode the loader uses certainly
>>> feels like a safer option.
>> Does a serial console report a "resolution" in the first place?
> 
> How would I find out?

You could log the values found in case of a pure serial console
setup (i.e. headless). But the multiplexed case is going to be the
more involved one anyway.

>> And
>> if we were able to (sufficiently easily) tell video from serial
>> console, how would we deal with the case of StdOut / StdErr being
>> multiplexed to both?
> 
> In my case they are multiplexed, and all work fine before set_mode() call.
> 
> After set_mode(), VGA still works, while serial sees nothing further on
> StdOut/Stderr (pre-existing lines are still in place).

Unfortunately the two of the three EFI boxes I routinely work with
and that I've tried mux-ed configurations on had no such issue, so
I'm afraid I couldn't easily debug this myself even if I wanted to.
But anyway, situations like this one are exactly why -basevideo has
been there from the very beginning.

For systems with a video device, the mode set here may be the mode
an eventual X driver also uses; sadly Linux'es kernel mode setting
looks to be having a high (and increasing) failure rate according
to my experience. Possibly much like the high rate of EFI issues
you observe / get reported, as opposed to me not having seen most
of them on the boxes I work with all the time (i.e. simply bad vs
good luck).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] EFI development issues

[Jan Beulich]

On 13.01.2020 17:02, Andrew Cooper wrote:
> First, there is a dependency tracking bug in the build system.  Edits to
> xen/arch/x86/efi/efi-boot.h don't cause xen.efi to be regenerated.  From
> what I can tell, the file doesn't even get recompiled, because syntax
> errors even go unnoticed.

I've just now seen this too, and also see why it is: My enforce-unique-
symbols change causes .*.o.d files to start like this

.apic.o.tmp: apic.c \
 /build/xen/unstable-git/2019-12-20-livepatch/xen/include/generated/config.h \
 ...

when the option is enabled. I'll need to figure how to tell the compiler
to emit the proper apic.o instead of the intermediate file. Iirc there's
a command line option ...

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel