Re: [PATCH] EmbeddedPkg: add driver to set graphical/serial console preference

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 20 October 2017 at 14:50, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Oct 20, 2017 at 02:33:37PM +0100, Ard Biesheuvel wrote:
>> Linux on ARM/arm64 will infer from the presence of a /chosen/stdout-path
>> DT property or of a SPCR ACPI table that the primary console is the serial
>> port, even if a graphical console is available as well.
>>
>> So let's introduce a driver that allows the user to set a preference
>> between graphical and serial if both are available. If the preference
>> is set to 'Graphical', and any GOP protocol instances have been installed
>> by the time the ReadyToBoot event is signalled, remove the DT property
>> and the table entirely.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c   | 284 ++++++++++++++++++++
>>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.h   |  31 +++
>>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.inf |  62 +++++
>>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefHii.uni |  27 ++
>>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefHii.vfr |  51 ++++
>>  5 files changed, 455 insertions(+)
>>
>> diff --git a/EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c b/EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c
>> new file mode 100644
>> index 000000000000..cabe4f652cf5
>> --- /dev/null
>> +++ b/EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c
>> @@ -0,0 +1,284 @@
>> +/** @file
>> +*
>> +*  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>> +*
>> +*  This program and the accompanying materials
>> +*  are licensed and made available under the terms and conditions of the BSD License
>> +*  which accompanies this distribution.  The full text of the license may be found at
>> +*  http://opensource.org/licenses/bsd-license.php
>> +*
>> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +*
>> +**/
>> +
>> +#include <IndustryStandard/Acpi.h>
>> +#include <libfdt.h>
>> +#include <Library/BaseLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/DevicePathLib.h>
>> +#include <Library/HiiLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/UefiDriverEntryPoint.h>
>> +#include <Library/UefiLib.h>
>> +#include <Library/UefiRuntimeServicesTableLib.h>
>> +
>> +#include <Protocol/AcpiTable.h>
>> +#include <Protocol/AcpiSystemDescriptionTable.h>
>> +
>> +#include "ConsolePrefDxe.h"
>> +
>> +#define SPCR_SIG    EFI_ACPI_2_0_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE
>> +
>> +extern  UINT8                     ConsolePrefHiiBin[];
>> +extern  UINT8                     ConsolePrefDxeStrings[];
>> +
>> +typedef struct {
>> +  VENDOR_DEVICE_PATH              VendorDevicePath;
>> +  EFI_DEVICE_PATH_PROTOCOL        End;
>> +} HII_VENDOR_DEVICE_PATH;
>> +
>> +STATIC HII_VENDOR_DEVICE_PATH     mConsolePrefDxeVendorDevicePath = {
>> +  {
>> +    {
>> +      HARDWARE_DEVICE_PATH,
>> +      HW_VENDOR_DP,
>> +      {
>> +        (UINT8) (sizeof (VENDOR_DEVICE_PATH)),
>> +        (UINT8) ((sizeof (VENDOR_DEVICE_PATH)) >> 8)
>> +      }
>> +    },
>> +    CONSOLE_PREF_FORMSET_GUID
>> +  },
>> +  {
>> +    END_DEVICE_PATH_TYPE,
>> +    END_ENTIRE_DEVICE_PATH_SUBTYPE,
>> +    {
>> +      (UINT8) (END_DEVICE_PATH_LENGTH),
>> +      (UINT8) ((END_DEVICE_PATH_LENGTH) >> 8)
>> +    }
>> +  }
>> +};
>> +
>> +STATIC EFI_EVENT                  mReadyToBootEvent;
>> +STATIC UINTN                      mSpcrTableKey;
>> +
>> +STATIC
>> +EFI_STATUS
>> +InstallHiiPages (
>> +  VOID
>> +  )
>> +{
>> +  EFI_STATUS                      Status;
>> +  EFI_HII_HANDLE                  HiiHandle;
>> +  EFI_HANDLE                      DriverHandle;
>> +
>> +  DriverHandle = NULL;
>> +  Status = gBS->InstallMultipleProtocolInterfaces (&DriverHandle,
>> +                  &gEfiDevicePathProtocolGuid,
>> +                  &mConsolePrefDxeVendorDevicePath,
>> +                  NULL);
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  HiiHandle = HiiAddPackages (&gConsolePrefFormSetGuid,
>> +                              DriverHandle,
>> +                              ConsolePrefDxeStrings,
>> +                              ConsolePrefHiiBin,
>> +                              NULL);
>> +
>> +  if (HiiHandle == NULL) {
>> +    gBS->UninstallMultipleProtocolInterfaces (DriverHandle,
>> +                  &gEfiDevicePathProtocolGuid,
>> +                  &mConsolePrefDxeVendorDevicePath,
>> +                  NULL);
>
> Something funky going on with indentation here.
>
> If you address that:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
> Thanks for putting this together.
>

Thanks. But it seems the SPCR handling is not robust yet: it worked
with ArmVirtQemu, but not with a baremetal platform, due to dispatch
order issues etc.