Re: [PATCH 4/5] ArmVirt/PlatformBootManagerLib: set up virtio serial as console

["Ard Biesheuvel" <ardb@kernel.org>]

On Fri, 12 May 2023 at 16:23, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> In case a virtio serial device is found in the system register the first
> console port as EFI console, by updating ConIn, ConOut and ErrOut.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  .../PlatformBootManagerLib/PlatformBm.c       | 163 ++++++++++++++++++
>  1 file changed, 163 insertions(+)
>
> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index ed38c42a43ee..7010d73c1388 100644
> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -312,6 +312,21 @@ IsVirtioRng (
>    return IsVirtio (Handle, ReportText, VIRTIO_SUBSYSTEM_ENTROPY_SOURCE);
>  }
>
> +/**
> +  This FILTER_FUNCTION checks if a handle corresponds to a Virtio serial device at
> +  the VIRTIO_DEVICE_PROTOCOL level.
> +**/
> +STATIC
> +BOOLEAN
> +EFIAPI
> +IsVirtioSerial (
> +  IN EFI_HANDLE    Handle,
> +  IN CONST CHAR16  *ReportText
> +  )
> +{
> +  return IsVirtio (Handle, ReportText, VIRTIO_SUBSYSTEM_CONSOLE);
> +}
> +
>  /**
>    This function checks if a handle corresponds to the Virtio Device ID given
>    at the EFI_PCI_IO_PROTOCOL level.
> @@ -446,6 +461,21 @@ IsVirtioPciRng (
>    return IsVirtioPci (Handle, ReportText, VIRTIO_SUBSYSTEM_ENTROPY_SOURCE);
>  }
>
> +/**
> +  This FILTER_FUNCTION checks if a handle corresponds to a Virtio serial device at
> +  the EFI_PCI_IO_PROTOCOL level.
> +**/
> +STATIC
> +BOOLEAN
> +EFIAPI
> +IsVirtioPciSerial (
> +  IN EFI_HANDLE    Handle,
> +  IN CONST CHAR16  *ReportText
> +  )
> +{
> +  return IsVirtioPci (Handle, ReportText, VIRTIO_SUBSYSTEM_CONSOLE);
> +}
> +
>  /**
>    This CALLBACK_FUNCTION attempts to connect a handle non-recursively, asking
>    the matching driver to produce all first-level child handles.
> @@ -534,6 +564,133 @@ AddOutput (
>      ));
>  }
>
> +/**
> +  This CALLBACK_FUNCTION retrieves the EFI_DEVICE_PATH_PROTOCOL from
> +  the handle, appends serial, uart and terminal nodes, finally updates
> +  ConIn, ConOut and ErrOut.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +SetupVirtioSerial (
> +  IN EFI_HANDLE    Handle,
> +  IN CONST CHAR16  *ReportText
> +  )
> +{
> +  STATIC ACPI_HID_DEVICE_PATH  SerialNode = {
> +    {
> +      ACPI_DEVICE_PATH,
> +      ACPI_DP,
> +      {
> +        (UINT8)(sizeof (ACPI_HID_DEVICE_PATH)),
> +        (UINT8)((sizeof (ACPI_HID_DEVICE_PATH)) >> 8)
> +      },
> +    },
> +    EISA_PNP_ID (0x0501),
> +    0
> +  };
> +
> +  STATIC UART_DEVICE_PATH  UartNode = {
> +    {
> +      MESSAGING_DEVICE_PATH,
> +      MSG_UART_DP,
> +      {
> +        (UINT8)(sizeof (UART_DEVICE_PATH)),
> +        (UINT8)((sizeof (UART_DEVICE_PATH)) >> 8)
> +      },
> +    },
> +    0,
> +    115200,
> +    8,
> +    1,
> +    1
> +  };
> +
> +  STATIC VENDOR_DEVICE_PATH  TerminalNode = {
> +    {
> +      MESSAGING_DEVICE_PATH,
> +      MSG_VENDOR_DP,
> +      {
> +        (UINT8)(sizeof (VENDOR_DEVICE_PATH)),
> +        (UINT8)((sizeof (VENDOR_DEVICE_PATH)) >> 8)
> +      },
> +    },
> +    DEVICE_PATH_MESSAGING_VT_UTF8
> +  };
> +

Please make these STATIC CONST

> +  EFI_STATUS                Status;
> +  EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
> +
> +  DevicePath = DevicePathFromHandle (Handle);
> +
> +  if (DevicePath == NULL) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: %s: handle %p: device path not found\n",
> +      __func__,
> +      ReportText,
> +      Handle
> +      ));
> +    return;
> +  }
> +
> +  DevicePath = AppendDevicePathNode (
> +                 DevicePath,
> +                 (EFI_DEVICE_PATH_PROTOCOL *)&SerialNode

You can use &SerialNode.Header and drop the cast (same below)

> +                 );
> +  DevicePath = AppendDevicePathNode (
> +                 DevicePath,
> +                 (EFI_DEVICE_PATH_PROTOCOL *)&UartNode
> +                 );
> +  DevicePath = AppendDevicePathNode (
> +                 DevicePath,
> +                 (EFI_DEVICE_PATH_PROTOCOL *)&TerminalNode
> +                 );
> +

This leaks two copies of DevicePath - better to assign to a temp
variable and FreePool() it between calls.

DevicePath = DevicePathFromHandle()
DevicePath = AppendDevicePathNode(DevicePath)
TempDp = AppendDevicePathNode(DevicePath)
Free(DevicePath)
DevicePath = AppendDevicePathNode(TempDp)
Free(TempDp)

and then free DevicePath at the end too.


> +  Status = EfiBootManagerUpdateConsoleVariable (ConIn, DevicePath, NULL);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: %s: adding to ConIn: %r\n",
> +      __func__,
> +      ReportText,
> +      Status
> +      ));
> +    return;
> +  }
> +
> +  Status = EfiBootManagerUpdateConsoleVariable (ConOut, DevicePath, NULL);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: %s: adding to ConOut: %r\n",
> +      __func__,
> +      ReportText,
> +      Status
> +      ));
> +    return;
> +  }
> +
> +  Status = EfiBootManagerUpdateConsoleVariable (ErrOut, DevicePath, NULL);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: %s: adding to ErrOut: %r\n",
> +      __func__,
> +      ReportText,
> +      Status
> +      ));
> +    return;
> +  }
> +
> +  DEBUG ((
> +    DEBUG_VERBOSE,
> +    "%a: %s: added to ConIn, ConOut and ErrOut\n",
> +    __func__,
> +    ReportText
> +    ));
> +}
> +
>  STATIC
>  VOID
>  PlatformRegisterFvBootOption (
> @@ -932,6 +1089,12 @@ PlatformBootManagerBeforeConsole (
>    // instances on Virtio PCI RNG devices.
>    //
>    FilterAndProcess (&gEfiPciIoProtocolGuid, IsVirtioPciRng, Connect);
> +
> +  //
> +  // Register Virtio serial devices as console.
> +  //
> +  FilterAndProcess (&gVirtioDeviceProtocolGuid, IsVirtioSerial, SetupVirtioSerial);
> +  FilterAndProcess (&gEfiPciIoProtocolGuid, IsVirtioPciSerial, SetupVirtioSerial);
>  }
>
>  /**
> --
> 2.40.1
>