| Message ID | 1413471323-19304-1-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Oct 16, 2014, at 7:55 AM, Laszlo Ersek <lersek@redhat.com> wrote: > The ConPlatformDxe driver examines Simple Text Input and Simple Text > Output protocol instances, and decides whether they are suitable for > console input and output splitting (ie. whether the given STI or STO > protocol should be included in the set of consoles that ConSplitterDxe > multiplexes). > > ConPlatformDxe signals its decision by installing Console In Device, > Console Out Device, and Standard Error Device protocol instances on the > handle in question. > > ConSplitterDxe then looks for these latter protocol instances and builds > the set of multiplexed input and output consoles based on them. If the > Console In Device protocol is not available on a handle, then that handle > won't be used for console input, and the output case works similarly. > > In order for ConPlatformDxe to allow ConSplitterDxe to multiplex a given > STI or STO (which is signalled by the installation of CID or COD), > ConPlatformDxe requires the device path of the STI or STO in question to > be present in the ConIn or ConOut non-volatile variable, respectively. > > At the very first boot of the VM, the ConIn and ConOut (and ErrOut) > non-volatile variables are missing (for a while). This causes > ConPlatformDxe to filter out the serial terminal (which provides the only > STI and STO on ArmVirtualizationQemu), and that in turn prevents > ConSplitterDxe from multiplexing input and output to/from the terminal, in > effect leaving the VM without any console. > Laszlo, Usually the BDS has policy to set the variables if they are missing prior to , or as part of, the BDS logic to connect the console. I don’t see how the ConSpliter would change this, as a console still needs to be connected? Thanks, Andrew Fish > Given that the "virt" machine type of qemu-system-aarch64 has no graphical > display, and ArmVirtualizationQemu in fact has a single (serial) console, > drop: > - ConSplitterDxe -- no splitting needed, > - ConPlatformDxe -- since we're dropping ConSplitterDxe, > - GraphicsConsoleDxe -- because no GOP is produced to begin with. > > This way the ConIn, ConOut, StdErr fields of the UEFI System Table will be > associated with the terminal driver's STI/STO directly, rather than with > the STI/STO of the backend-less console splitter. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc | 6 +++--- > ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf | 5 +---- > 2 files changed, 4 insertions(+), 7 deletions(-) > > diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc > index 61689b7..871641d 100644 > --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc > +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc > @@ -221,9 +221,9 @@ > EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf > EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf > > - MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf > - MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf > - MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf > + # > + # Single Console IO support > + # > MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf > EmbeddedPkg/SerialDxe/SerialDxe.inf > > diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf > index 2b71d61..434fdd9 100644 > --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf > +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf > @@ -124,11 +124,8 @@ READ_LOCK_STATUS = TRUE > INF MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf > > # > - # Multiple Console IO support > + # Single Console IO support > # > - INF MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf > - INF MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf > - INF MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf > INF MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf > INF EmbeddedPkg/SerialDxe/SerialDxe.inf > > -- > 1.8.3.1 > > > ------------------------------------------------------------------------------ > Comprehensive Server Monitoring with Site24x7. > Monitor 10 servers for $9/Month. > Get alerted through email, SMS, voice calls or mobile push notifications. > Take corrective actions from your mobile device. > http://p.sf.net/sfu/Zoho > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Comprehensive Server Monitoring with Site24x7. Monitor 10 servers for $9/Month. Get alerted through email, SMS, voice calls or mobile push notifications. Take corrective actions from your mobile device. http://p.sf.net/sfu/Zoho
On 10/16/14 17:16, Andrew Fish wrote: > > On Oct 16, 2014, at 7:55 AM, Laszlo Ersek <lersek@redhat.com> wrote: > >> The ConPlatformDxe driver examines Simple Text Input and Simple Text >> Output protocol instances, and decides whether they are suitable for >> console input and output splitting (ie. whether the given STI or STO >> protocol should be included in the set of consoles that ConSplitterDxe >> multiplexes). >> >> ConPlatformDxe signals its decision by installing Console In Device, >> Console Out Device, and Standard Error Device protocol instances on the >> handle in question. >> >> ConSplitterDxe then looks for these latter protocol instances and builds >> the set of multiplexed input and output consoles based on them. If the >> Console In Device protocol is not available on a handle, then that handle >> won't be used for console input, and the output case works similarly. >> >> In order for ConPlatformDxe to allow ConSplitterDxe to multiplex a given >> STI or STO (which is signalled by the installation of CID or COD), >> ConPlatformDxe requires the device path of the STI or STO in question to >> be present in the ConIn or ConOut non-volatile variable, respectively. >> >> At the very first boot of the VM, the ConIn and ConOut (and ErrOut) >> non-volatile variables are missing (for a while). This causes >> ConPlatformDxe to filter out the serial terminal (which provides the only >> STI and STO on ArmVirtualizationQemu), and that in turn prevents >> ConSplitterDxe from multiplexing input and output to/from the terminal, in >> effect leaving the VM without any console. >> > > Laszlo, > > Usually the BDS has policy to set the variables if they are missing > prior to , or as part of, the BDS logic to connect the console. > I don’t see how the ConSpliter would change this, as a console still > needs to be connected? The InitializeConsole() function in "ArmPlatformPkg/Bds/Bds.c" first calls GetConsoleDevicePathFromVariable() -- three times -- which sets the variables if they are missing. Then it calls InitializeConsolePipe() -- again three times -- which calls BdsConnectDevicePath() which calls BdsConnectAndUpdateDevicePath() which calls gBS->ConnectController(). So, it appears to conform to what you describe, but if the variables are missing up-front, the serial console doesn't work. (ConSplitterDxe installs its own STI/STO protocols into the gST, but there is no "backend".) If I apply this patch, things just work, even if the variables are absent up-front. It might be an obscure bug in ARM BDS, but that code is incredibly hairy. Given that we have only one console on this platform anyway, I decided to go with this patch -- either it is applied (and then the bug, wherever it is, is avoided), or it triggers people to help find the ARM BDS bug (if it's really there). Thanks! Laszlo > Thanks, > > Andrew Fish > >> Given that the "virt" machine type of qemu-system-aarch64 has no graphical >> display, and ArmVirtualizationQemu in fact has a single (serial) console, >> drop: >> - ConSplitterDxe -- no splitting needed, >> - ConPlatformDxe -- since we're dropping ConSplitterDxe, >> - GraphicsConsoleDxe -- because no GOP is produced to begin with. >> >> This way the ConIn, ConOut, StdErr fields of the UEFI System Table will be >> associated with the terminal driver's STI/STO directly, rather than with >> the STI/STO of the backend-less console splitter. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc | 6 +++--- >> ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf | 5 +---- >> 2 files changed, 4 insertions(+), 7 deletions(-) >> >> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc >> index 61689b7..871641d 100644 >> --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc >> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc >> @@ -221,9 +221,9 @@ >> EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf >> EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf >> >> - MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf >> - MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf >> - MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf >> + # >> + # Single Console IO support >> + # >> MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf >> EmbeddedPkg/SerialDxe/SerialDxe.inf >> >> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf >> index 2b71d61..434fdd9 100644 >> --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf >> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf >> @@ -124,11 +124,8 @@ READ_LOCK_STATUS = TRUE >> INF MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf >> >> # >> - # Multiple Console IO support >> + # Single Console IO support >> # >> - INF MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf >> - INF MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf >> - INF MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf >> INF MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf >> INF EmbeddedPkg/SerialDxe/SerialDxe.inf >> >> -- >> 1.8.3.1 >> >> >> ------------------------------------------------------------------------------ >> Comprehensive Server Monitoring with Site24x7. >> Monitor 10 servers for $9/Month. >> Get alerted through email, SMS, voice calls or mobile push notifications. >> Take corrective actions from your mobile device. >> http://p.sf.net/sfu/Zoho >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/edk2-devel > > > ------------------------------------------------------------------------------ > Comprehensive Server Monitoring with Site24x7. > Monitor 10 servers for $9/Month. > Get alerted through email, SMS, voice calls or mobile push notifications. > Take corrective actions from your mobile device. > http://p.sf.net/sfu/Zoho > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel > ------------------------------------------------------------------------------ Comprehensive Server Monitoring with Site24x7. Monitor 10 servers for $9/Month. Get alerted through email, SMS, voice calls or mobile push notifications. Take corrective actions from your mobile device. http://p.sf.net/sfu/Zoho
If there is a bug in the ARM BDS then the bug should be the lack of error messages when there is no default ConIn/ConOut/ConErr device path and ST.ConIn/ST.ConOut/ST.ConErr are NULL. And actually, I thought I added this code (see InitializeConsolePipe()) as I had this issue a couple of times when ding platform bringup. But maybe this code is now shown up correctly. There are facilities to define default device paths for Con(In|Out): gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths and gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths. Your patch looks like a workaround to me. If one day someone decide to write a second output driver (in addition to the serial console) for the 'virt' platform then he/she would have to restore the console splitter and this person would have the issue. > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: 16 October 2014 16:36 > To: edk2-devel@lists.sourceforge.net > Subject: Re: [edk2] [PATCH] ArmVirtualizationQemu: fix the console when > ConIn/ConOut/ErrOut are missing > > On 10/16/14 17:16, Andrew Fish wrote: > > > > On Oct 16, 2014, at 7:55 AM, Laszlo Ersek <lersek@redhat.com> wrote: > > > >> The ConPlatformDxe driver examines Simple Text Input and Simple Text > >> Output protocol instances, and decides whether they are suitable for > >> console input and output splitting (ie. whether the given STI or STO > >> protocol should be included in the set of consoles that > ConSplitterDxe > >> multiplexes). > >> > >> ConPlatformDxe signals its decision by installing Console In Device, > >> Console Out Device, and Standard Error Device protocol instances on > the > >> handle in question. > >> > >> ConSplitterDxe then looks for these latter protocol instances and > builds > >> the set of multiplexed input and output consoles based on them. If > the > >> Console In Device protocol is not available on a handle, then that > handle > >> won't be used for console input, and the output case works > similarly. > >> > >> In order for ConPlatformDxe to allow ConSplitterDxe to multiplex a > given > >> STI or STO (which is signalled by the installation of CID or COD), > >> ConPlatformDxe requires the device path of the STI or STO in > question to > >> be present in the ConIn or ConOut non-volatile variable, > respectively. > >> > >> At the very first boot of the VM, the ConIn and ConOut (and ErrOut) > >> non-volatile variables are missing (for a while). This causes > >> ConPlatformDxe to filter out the serial terminal (which provides the > only > >> STI and STO on ArmVirtualizationQemu), and that in turn prevents > >> ConSplitterDxe from multiplexing input and output to/from the > terminal, in > >> effect leaving the VM without any console. > >> > > > > Laszlo, > > > > Usually the BDS has policy to set the variables if they are missing > > prior to , or as part of, the BDS logic to connect the console. > > I don't see how the ConSpliter would change this, as a console still > > needs to be connected? > > The InitializeConsole() function in "ArmPlatformPkg/Bds/Bds.c" first > calls GetConsoleDevicePathFromVariable() -- three times -- which sets > the variables if they are missing. > > Then it calls InitializeConsolePipe() -- again three times -- which > calls BdsConnectDevicePath() which calls > BdsConnectAndUpdateDevicePath() > which calls gBS->ConnectController(). > > So, it appears to conform to what you describe, but if the variables > are > missing up-front, the serial console doesn't work. (ConSplitterDxe > installs its own STI/STO protocols into the gST, but there is no > "backend".) If I apply this patch, things just work, even if the > variables are absent up-front. > > It might be an obscure bug in ARM BDS, but that code is incredibly > hairy. Given that we have only one console on this platform anyway, I > decided to go with this patch -- either it is applied (and then the > bug, > wherever it is, is avoided), or it triggers people to help find the ARM > BDS bug (if it's really there). > > Thanks! > Laszlo > > > Thanks, > > > > Andrew Fish > > > >> Given that the "virt" machine type of qemu-system-aarch64 has no > graphical > >> display, and ArmVirtualizationQemu in fact has a single (serial) > console, > >> drop: > >> - ConSplitterDxe -- no splitting needed, > >> - ConPlatformDxe -- since we're dropping ConSplitterDxe, > >> - GraphicsConsoleDxe -- because no GOP is produced to begin with. > >> > >> This way the ConIn, ConOut, StdErr fields of the UEFI System Table > will be > >> associated with the terminal driver's STI/STO directly, rather than > with > >> the STI/STO of the backend-less console splitter. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > >> --- > >> ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc | 6 > +++--- > >> ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf | 5 +- > --- > >> 2 files changed, 4 insertions(+), 7 deletions(-) > >> > >> diff --git > a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc > b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc > >> index 61689b7..871641d 100644 > >> --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc > >> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc > >> @@ -221,9 +221,9 @@ > >> EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf > >> EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf > >> > >> - MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf > >> - MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf > >> - > MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.in > f > >> + # > >> + # Single Console IO support > >> + # > >> MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf > >> EmbeddedPkg/SerialDxe/SerialDxe.inf > >> > >> diff --git > a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf > b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf > >> index 2b71d61..434fdd9 100644 > >> --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf > >> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf > >> @@ -124,11 +124,8 @@ READ_LOCK_STATUS = TRUE > >> INF MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf > >> > >> # > >> - # Multiple Console IO support > >> + # Single Console IO support > >> # > >> - INF > MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf > >> - INF > MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf > >> - INF > MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.in > f > >> INF MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf > >> INF EmbeddedPkg/SerialDxe/SerialDxe.inf > >> > >> -- > >> 1.8.3.1 > >> > >> > >> -------------------------------------------------------------------- > ---------- > >> Comprehensive Server Monitoring with Site24x7. > >> Monitor 10 servers for $9/Month. > >> Get alerted through email, SMS, voice calls or mobile push > notifications. > >> Take corrective actions from your mobile device. > >> http://p.sf.net/sfu/Zoho > >> _______________________________________________ > >> edk2-devel mailing list > >> edk2-devel@lists.sourceforge.net > >> https://lists.sourceforge.net/lists/listinfo/edk2-devel > > > > > > --------------------------------------------------------------------- > --------- > > Comprehensive Server Monitoring with Site24x7. > > Monitor 10 servers for $9/Month. > > Get alerted through email, SMS, voice calls or mobile push > notifications. > > Take corrective actions from your mobile device. > > http://p.sf.net/sfu/Zoho > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > > > > ----------------------------------------------------------------------- > ------- > Comprehensive Server Monitoring with Site24x7. > Monitor 10 servers for $9/Month. > Get alerted through email, SMS, voice calls or mobile push > notifications. > Take corrective actions from your mobile device. > http://p.sf.net/sfu/Zoho > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Comprehensive Server Monitoring with Site24x7. Monitor 10 servers for $9/Month. Get alerted through email, SMS, voice calls or mobile push notifications. Take corrective actions from your mobile device. http://p.sf.net/sfu/Zoho
On 10/16/14 18:05, Olivier Martin wrote: > If there is a bug in the ARM BDS then the bug should be the lack of > error messages when there is no default ConIn/ConOut/ConErr device > path and ST.ConIn/ST.ConOut/ST.ConErr are NULL. > And actually, I thought I added this code (see > InitializeConsolePipe()) as I had this issue a couple of times when > ding platform bringup. But maybe this code is now shown up correctly. > > There are facilities to define default device paths for Con(In|Out): > gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths and > gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths. Yeah, those are set in the DSC file, correctly. > Your patch looks like a workaround to me. > If one day someone decide to write a second output driver (in addition > to the serial console) for the 'virt' platform then he/she would have > to restore the console splitter and this person would have the issue. I think I got closer to the root of the problem. BdsEntry() DefineDefaultBootEntries() BdsConnectAllDrivers() <---- I added this call InitializeConsole() sets ConIn/ConOut/ErrOut Thanks to my "extra" BdsConnectAllDrivers() call (see below), ConPlatformDxe's ConPlatformTextInDriverBindingStart() and ConPlatformTextOutDriverBindingStart() functions run and reject TerminalDxe's STI/STO before BDS would have a chance to set ConIn / ConOut to anything (ie. what ConPlatformDxe filters against). ConPlatformMatchDevicePaths() then gets NULL for the variable value, and returns EFI_INVALID_PARAMETER. At the second boot ConIn and ConOut have the right contents by the time of the filtering, but not at the first. ... This happens because DefineDefaultBootEntries() connects the terminal (and recursively the rest of the drivers) before InitializeConsole() sets the variables. And that connection happens because I added a BdsConnectAllDrivers() call to DefineDefaultBootEntries() -- because I needed some logic to search automatically for an EFI system partition on all possible devices. What would you think of the following patch then: > diff --git a/ArmPlatformPkg/Bds/Bds.c b/ArmPlatformPkg/Bds/Bds.c > index 81ba029..29158a1 100644 > --- a/ArmPlatformPkg/Bds/Bds.c > +++ b/ArmPlatformPkg/Bds/Bds.c > @@ -615,23 +615,23 @@ BdsEntry ( > gRT->SetVariable (L"BootCurrent", &gEfiGlobalVariableGuid, > EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS, > 0, NULL); > } > > - // If Boot Order does not exist then create a default entry > - DefineDefaultBootEntries (); > - > // Now we need to setup the EFI System Table with information about the console devices. > InitializeConsole (); > > // > // Update the CRC32 in the EFI System Table header > // > gST->Hdr.CRC32 = 0; > Status = gBS->CalculateCrc32 ((VOID*)gST, gST->Hdr.HeaderSize, &gST->Hdr.CRC32); > ASSERT_EFI_ERROR (Status); > > + // If Boot Order does not exist then create a default entry > + DefineDefaultBootEntries (); > + > // Timer before initiating the default boot selection > StartDefaultBootOnTimeout (); > > // Start the Boot Menu > Status = BootMenuMain (); This just moves the DefineDefaultBootEntries() call -- and my custom BdsConnectAllDrivers() call with it -- after the ConIn / ConOut setup: BdsEntry() InitializeConsole() sets ConIn/ConOut/ErrOut DefineDefaultBootEntries() BdsConnectAllDrivers() <---- I added this call Thanks! Laszlo ------------------------------------------------------------------------------ Comprehensive Server Monitoring with Site24x7. Monitor 10 servers for $9/Month. Get alerted through email, SMS, voice calls or mobile push notifications. Take corrective actions from your mobile device. http://p.sf.net/sfu/Zoho
> -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: 17 October 2014 00:04 > To: edk2-devel@lists.sourceforge.net > Subject: Re: [edk2] [PATCH] ArmVirtualizationQemu: fix the console when > ConIn/ConOut/ErrOut are missing > > On 10/16/14 18:05, Olivier Martin wrote: > > If there is a bug in the ARM BDS then the bug should be the lack of > > error messages when there is no default ConIn/ConOut/ConErr device > > path and ST.ConIn/ST.ConOut/ST.ConErr are NULL. > > And actually, I thought I added this code (see > > InitializeConsolePipe()) as I had this issue a couple of times when > > ding platform bringup. But maybe this code is now shown up correctly. > > > > There are facilities to define default device paths for Con(In|Out): > > gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths and > > gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths. > > Yeah, those are set in the DSC file, correctly. > > > Your patch looks like a workaround to me. > > If one day someone decide to write a second output driver (in > addition > > to the serial console) for the 'virt' platform then he/she would have > > to restore the console splitter and this person would have the issue. > > I think I got closer to the root of the problem. > > BdsEntry() > DefineDefaultBootEntries() > BdsConnectAllDrivers() <---- I added this call > InitializeConsole() > sets ConIn/ConOut/ErrOut > > Thanks to my "extra" BdsConnectAllDrivers() call (see below), > ConPlatformDxe's ConPlatformTextInDriverBindingStart() and > ConPlatformTextOutDriverBindingStart() functions run and reject > TerminalDxe's STI/STO before BDS would have a chance to set ConIn / > ConOut to anything (ie. what ConPlatformDxe filters against). > > ConPlatformMatchDevicePaths() then gets NULL for the variable value, > and > returns EFI_INVALID_PARAMETER. > > At the second boot ConIn and ConOut have the right contents by the time > of the filtering, but not at the first. > > ... This happens because DefineDefaultBootEntries() connects the > terminal (and recursively the rest of the drivers) before > InitializeConsole() sets the variables. And that connection happens > because I added a BdsConnectAllDrivers() call to > DefineDefaultBootEntries() -- because I needed some logic to search > automatically for an EFI system partition on all possible devices. I cannot see where DefineDefaultBootEntries() connects the terminal. Actually this function does not connect any driver. InitializeConsole() create the NV variable that define the default Con(In|Out|Err) device path(s) if this NV UEFI variable does not exist. This function also connect the terminal device path. STATIC EFI_STATUS InitializeConsolePipe ( IN EFI_DEVICE_PATH *ConsoleDevicePaths, IN EFI_GUID *Protocol, OUT EFI_HANDLE *Handle, OUT VOID* *Interface ) { EFI_STATUS Status; UINTN Size; UINTN NoHandles; EFI_HANDLE *Buffer; EFI_DEVICE_PATH_PROTOCOL* DevicePath; // Connect all the Device Path Consoles while (ConsoleDevicePaths != NULL) { DevicePath = GetNextDevicePathInstance (&ConsoleDevicePaths, &Size); Status = BdsConnectDevicePath (DevicePath, Handle, NULL); // If the console splitter driver is not supported by the platform then use the first Device Path // instance for the console interface. if (!EFI_ERROR(Status) && (*Interface == NULL)) { Status = gBS->HandleProtocol (*Handle, Protocol, Interface); } } (...) } > > What would you think of the following patch then: > > > diff --git a/ArmPlatformPkg/Bds/Bds.c b/ArmPlatformPkg/Bds/Bds.c > > index 81ba029..29158a1 100644 > > --- a/ArmPlatformPkg/Bds/Bds.c > > +++ b/ArmPlatformPkg/Bds/Bds.c > > @@ -615,23 +615,23 @@ BdsEntry ( > > gRT->SetVariable (L"BootCurrent", &gEfiGlobalVariableGuid, > > EFI_VARIABLE_BOOTSERVICE_ACCESS | > EFI_VARIABLE_RUNTIME_ACCESS, > > 0, NULL); > > } > > > > - // If Boot Order does not exist then create a default entry > > - DefineDefaultBootEntries (); > > - > > // Now we need to setup the EFI System Table with information > about the console devices. > > InitializeConsole (); > > > > // > > // Update the CRC32 in the EFI System Table header > > // > > gST->Hdr.CRC32 = 0; > > Status = gBS->CalculateCrc32 ((VOID*)gST, gST->Hdr.HeaderSize, > &gST->Hdr.CRC32); > > ASSERT_EFI_ERROR (Status); > > > > + // If Boot Order does not exist then create a default entry > > + DefineDefaultBootEntries (); > > + > > // Timer before initiating the default boot selection > > StartDefaultBootOnTimeout (); > > > > // Start the Boot Menu > > Status = BootMenuMain (); > > This just moves the DefineDefaultBootEntries() call -- and my custom > BdsConnectAllDrivers() call with it -- after the ConIn / ConOut setup: > > BdsEntry() > InitializeConsole() > sets ConIn/ConOut/ErrOut > DefineDefaultBootEntries() > BdsConnectAllDrivers() <---- I added this call > > Thanks! > Laszlo > > ----------------------------------------------------------------------- > ------- > Comprehensive Server Monitoring with Site24x7. > Monitor 10 servers for $9/Month. > Get alerted through email, SMS, voice calls or mobile push > notifications. > Take corrective actions from your mobile device. > http://p.sf.net/sfu/Zoho > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------
On 10/28/14 17:23, Olivier Martin wrote: > > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: 17 October 2014 00:04 >> To: edk2-devel@lists.sourceforge.net >> Subject: Re: [edk2] [PATCH] ArmVirtualizationQemu: fix the console when >> ConIn/ConOut/ErrOut are missing >> >> On 10/16/14 18:05, Olivier Martin wrote: >>> If there is a bug in the ARM BDS then the bug should be the lack of >>> error messages when there is no default ConIn/ConOut/ConErr device >>> path and ST.ConIn/ST.ConOut/ST.ConErr are NULL. >>> And actually, I thought I added this code (see >>> InitializeConsolePipe()) as I had this issue a couple of times when >>> ding platform bringup. But maybe this code is now shown up correctly. >>> >>> There are facilities to define default device paths for Con(In|Out): >>> gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths and >>> gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths. >> >> Yeah, those are set in the DSC file, correctly. >> >>> Your patch looks like a workaround to me. >>> If one day someone decide to write a second output driver (in >> addition >>> to the serial console) for the 'virt' platform then he/she would have >>> to restore the console splitter and this person would have the issue. >> >> I think I got closer to the root of the problem. >> >> BdsEntry() >> DefineDefaultBootEntries() >> BdsConnectAllDrivers() <---- I added this call >> InitializeConsole() >> sets ConIn/ConOut/ErrOut >> >> Thanks to my "extra" BdsConnectAllDrivers() call (see below), >> ConPlatformDxe's ConPlatformTextInDriverBindingStart() and >> ConPlatformTextOutDriverBindingStart() functions run and reject >> TerminalDxe's STI/STO before BDS would have a chance to set ConIn / >> ConOut to anything (ie. what ConPlatformDxe filters against). >> >> ConPlatformMatchDevicePaths() then gets NULL for the variable value, >> and >> returns EFI_INVALID_PARAMETER. >> >> At the second boot ConIn and ConOut have the right contents by the time >> of the filtering, but not at the first. >> >> ... This happens because DefineDefaultBootEntries() connects the >> terminal (and recursively the rest of the drivers) before >> InitializeConsole() sets the variables. And that connection happens >> because I added a BdsConnectAllDrivers() call to >> DefineDefaultBootEntries() -- because I needed some logic to search >> automatically for an EFI system partition on all possible devices. > > I cannot see where DefineDefaultBootEntries() connects the terminal. Sorry, I wasn't clear enough. The BdsConnectAllDrivers() call in question (which happens to connect the terminal as well, within DefineDefaultBootEntries()) is in a downstream-only patch of mine. So, basically, I introduced the bug myself; it's not present in the upstream repo. But, I thought that the patch below that fixes my "private bug" might also be acceptable for upstream. It wouldn't cause any observable change in behavior (for upstream), but it would decrease divergence for me. Anyway please feel free to just drop this thread. There's no bug in upstream. Thanks! Laszlo > Actually this function does not connect any driver. > > InitializeConsole() create the NV variable that define the default > Con(In|Out|Err) device path(s) if this NV UEFI variable does not exist. > This function also connect the terminal device path. > > STATIC > EFI_STATUS > InitializeConsolePipe ( > IN EFI_DEVICE_PATH *ConsoleDevicePaths, > IN EFI_GUID *Protocol, > OUT EFI_HANDLE *Handle, > OUT VOID* *Interface > ) > { > EFI_STATUS Status; > UINTN Size; > UINTN NoHandles; > EFI_HANDLE *Buffer; > EFI_DEVICE_PATH_PROTOCOL* DevicePath; > > // Connect all the Device Path Consoles > while (ConsoleDevicePaths != NULL) { > DevicePath = GetNextDevicePathInstance (&ConsoleDevicePaths, &Size); > > Status = BdsConnectDevicePath (DevicePath, Handle, NULL); > > // If the console splitter driver is not supported by the platform then > use the first Device Path > // instance for the console interface. > if (!EFI_ERROR(Status) && (*Interface == NULL)) { > Status = gBS->HandleProtocol (*Handle, Protocol, Interface); > } > } > > (...) > } > >> >> What would you think of the following patch then: >> >>> diff --git a/ArmPlatformPkg/Bds/Bds.c b/ArmPlatformPkg/Bds/Bds.c >>> index 81ba029..29158a1 100644 >>> --- a/ArmPlatformPkg/Bds/Bds.c >>> +++ b/ArmPlatformPkg/Bds/Bds.c >>> @@ -615,23 +615,23 @@ BdsEntry ( >>> gRT->SetVariable (L"BootCurrent", &gEfiGlobalVariableGuid, >>> EFI_VARIABLE_BOOTSERVICE_ACCESS | >> EFI_VARIABLE_RUNTIME_ACCESS, >>> 0, NULL); >>> } >>> >>> - // If Boot Order does not exist then create a default entry >>> - DefineDefaultBootEntries (); >>> - >>> // Now we need to setup the EFI System Table with information >> about the console devices. >>> InitializeConsole (); >>> >>> // >>> // Update the CRC32 in the EFI System Table header >>> // >>> gST->Hdr.CRC32 = 0; >>> Status = gBS->CalculateCrc32 ((VOID*)gST, gST->Hdr.HeaderSize, >> &gST->Hdr.CRC32); >>> ASSERT_EFI_ERROR (Status); >>> >>> + // If Boot Order does not exist then create a default entry >>> + DefineDefaultBootEntries (); >>> + >>> // Timer before initiating the default boot selection >>> StartDefaultBootOnTimeout (); >>> >>> // Start the Boot Menu >>> Status = BootMenuMain (); >> >> This just moves the DefineDefaultBootEntries() call -- and my custom >> BdsConnectAllDrivers() call with it -- after the ConIn / ConOut setup: >> >> BdsEntry() >> InitializeConsole() >> sets ConIn/ConOut/ErrOut >> DefineDefaultBootEntries() >> BdsConnectAllDrivers() <---- I added this call >> >> Thanks! >> Laszlo >> >> ----------------------------------------------------------------------- >> ------- >> Comprehensive Server Monitoring with Site24x7. >> Monitor 10 servers for $9/Month. >> Get alerted through email, SMS, voice calls or mobile push >> notifications. >> Take corrective actions from your mobile device. >> http://p.sf.net/sfu/Zoho >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/edk2-devel > > > > > > ------------------------------------------------------------------------------ > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel > ------------------------------------------------------------------------------
diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc index 61689b7..871641d 100644 --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc @@ -221,9 +221,9 @@ EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf - MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf - MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf - MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf + # + # Single Console IO support + # MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf EmbeddedPkg/SerialDxe/SerialDxe.inf diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf index 2b71d61..434fdd9 100644 --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf @@ -124,11 +124,8 @@ READ_LOCK_STATUS = TRUE INF MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf # - # Multiple Console IO support + # Single Console IO support # - INF MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf - INF MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf - INF MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf INF MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf INF EmbeddedPkg/SerialDxe/SerialDxe.inf
The ConPlatformDxe driver examines Simple Text Input and Simple Text Output protocol instances, and decides whether they are suitable for console input and output splitting (ie. whether the given STI or STO protocol should be included in the set of consoles that ConSplitterDxe multiplexes). ConPlatformDxe signals its decision by installing Console In Device, Console Out Device, and Standard Error Device protocol instances on the handle in question. ConSplitterDxe then looks for these latter protocol instances and builds the set of multiplexed input and output consoles based on them. If the Console In Device protocol is not available on a handle, then that handle won't be used for console input, and the output case works similarly. In order for ConPlatformDxe to allow ConSplitterDxe to multiplex a given STI or STO (which is signalled by the installation of CID or COD), ConPlatformDxe requires the device path of the STI or STO in question to be present in the ConIn or ConOut non-volatile variable, respectively. At the very first boot of the VM, the ConIn and ConOut (and ErrOut) non-volatile variables are missing (for a while). This causes ConPlatformDxe to filter out the serial terminal (which provides the only STI and STO on ArmVirtualizationQemu), and that in turn prevents ConSplitterDxe from multiplexing input and output to/from the terminal, in effect leaving the VM without any console. Given that the "virt" machine type of qemu-system-aarch64 has no graphical display, and ArmVirtualizationQemu in fact has a single (serial) console, drop: - ConSplitterDxe -- no splitting needed, - ConPlatformDxe -- since we're dropping ConSplitterDxe, - GraphicsConsoleDxe -- because no GOP is produced to begin with. This way the ConIn, ConOut, StdErr fields of the UEFI System Table will be associated with the terminal driver's STI/STO directly, rather than with the STI/STO of the backend-less console splitter. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc | 6 +++--- ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf | 5 +---- 2 files changed, 4 insertions(+), 7 deletions(-)