[PATCH 0/5] ArmVirt: add VirtioSerialDxe

[PATCH 0/5] ArmVirt: add VirtioSerialDxe

[Gerd Hoffmann]

Gerd Hoffmann (5):
  ArmVirt: add VirtioSerialDxe to ArmVirtQemu builds
  ArmVirt/PlatformBootManagerLib: factor out IsVirtio()
  ArmVirt/PlatformBootManagerLib: factor out IsVirtioPci()
  ArmVirt/PlatformBootManagerLib: set up virtio serial as console
  OvmfPkg/VirtioSerialDxe: use TPL_NOTIFY

 ArmVirtPkg/ArmVirtQemu.dsc                    |   1 +
 ArmVirtPkg/ArmVirtQemuKernel.dsc              |   1 +
 .../PlatformBootManagerLib/PlatformBm.c       | 215 +++++++++++++++++-
 OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c    |   4 +-
 ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc          |   1 +
 5 files changed, 210 insertions(+), 12 deletions(-)

-- 
2.40.1

[PATCH 1/5] ArmVirt: add VirtioSerialDxe to ArmVirtQemu builds

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ArmVirtPkg/ArmVirtQemu.dsc           | 1 +
 ArmVirtPkg/ArmVirtQemuKernel.dsc     | 1 +
 ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
 3 files changed, 3 insertions(+)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 449e73b9e132..d998f816b2b3 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -448,6 +448,7 @@ [Components.common]
   OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
   OvmfPkg/VirtioNetDxe/VirtioNet.inf
   OvmfPkg/VirtioRngDxe/VirtioRng.inf
+  OvmfPkg/VirtioSerialDxe/VirtioSerial.inf
 
   #
   # FAT filesystem + GPT/MBR partitioning + UDF filesystem + virtio-fs
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index 3cb9120e4e10..c9d2b18392b9 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -353,6 +353,7 @@ [Components.common]
   OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
   OvmfPkg/VirtioNetDxe/VirtioNet.inf
   OvmfPkg/VirtioRngDxe/VirtioRng.inf
+  OvmfPkg/VirtioSerialDxe/VirtioSerial.inf
 
   #
   # FAT filesystem + GPT/MBR partitioning + UDF filesystem + virtio-fs
diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
index 8a063bac04ac..2894bc853a46 100644
--- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
+++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
@@ -98,6 +98,7 @@ [FV.FvMain]
   INF OvmfPkg/VirtioNetDxe/VirtioNet.inf
   INF OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
   INF OvmfPkg/VirtioRngDxe/VirtioRng.inf
+  INF OvmfPkg/VirtioSerialDxe/VirtioSerial.inf
 
   INF ShellPkg/Application/Shell/Shell.inf
   INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
-- 
2.40.1

[PATCH 2/5] ArmVirt/PlatformBootManagerLib: factor out IsVirtio()

[Gerd Hoffmann]

IsVirtioRng() becomes just a thin wrapper for IsVirtio().
This allows to add similar thin wrappers for other virtio
devices in the future.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 .../PlatformBootManagerLib/PlatformBm.c       | 26 +++++++++++++++----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 10c815378cd5..5eb6f0f9c14a 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -269,15 +269,16 @@ IsPciDisplay (
 }
 
 /**
-  This FILTER_FUNCTION checks if a handle corresponds to a Virtio RNG device at
-  the VIRTIO_DEVICE_PROTOCOL level.
+  This function checks if a handle corresponds to the Virtio Device ID given
+  at the VIRTIO_DEVICE_PROTOCOL level.
 **/
 STATIC
 BOOLEAN
 EFIAPI
-IsVirtioRng (
+IsVirtio (
   IN EFI_HANDLE    Handle,
-  IN CONST CHAR16  *ReportText
+  IN CONST CHAR16  *ReportText,
+  IN UINT16        VirtIoDeviceId
   )
 {
   EFI_STATUS              Status;
@@ -293,7 +294,22 @@ IsVirtioRng (
   }
 
   return (BOOLEAN)(VirtIo->SubSystemDeviceId ==
-                   VIRTIO_SUBSYSTEM_ENTROPY_SOURCE);
+                   VirtIoDeviceId);
+}
+
+/**
+  This FILTER_FUNCTION checks if a handle corresponds to a Virtio RNG device at
+  the VIRTIO_DEVICE_PROTOCOL level.
+**/
+STATIC
+BOOLEAN
+EFIAPI
+IsVirtioRng (
+  IN EFI_HANDLE    Handle,
+  IN CONST CHAR16  *ReportText
+  )
+{
+  return IsVirtio (Handle, ReportText, VIRTIO_SUBSYSTEM_ENTROPY_SOURCE);
 }
 
 /**
-- 
2.40.1

[PATCH 3/5] ArmVirt/PlatformBootManagerLib: factor out IsVirtioPci()

[Gerd Hoffmann]

IsVirtioPciRng() becomes just a thin wrapper for IsVirtioPci().
This allows to add similar thin wrappers for other virtio
devices in the future.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 .../PlatformBootManagerLib/PlatformBm.c       | 30 ++++++++++++++-----
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 5eb6f0f9c14a..ed38c42a43ee 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -313,15 +313,16 @@ IsVirtioRng (
 }
 
 /**
-  This FILTER_FUNCTION checks if a handle corresponds to a Virtio RNG device at
-  the EFI_PCI_IO_PROTOCOL level.
+  This function checks if a handle corresponds to the Virtio Device ID given
+  at the EFI_PCI_IO_PROTOCOL level.
 **/
 STATIC
 BOOLEAN
 EFIAPI
-IsVirtioPciRng (
+IsVirtioPci (
   IN EFI_HANDLE    Handle,
-  IN CONST CHAR16  *ReportText
+  IN CONST CHAR16  *ReportText,
+  IN UINT16        VirtIoDeviceId
   )
 {
   EFI_STATUS           Status;
@@ -387,11 +388,11 @@ IsVirtioPciRng (
   //
   // From DeviceId and RevisionId, determine whether the device is a
   // modern-only Virtio 1.0 device. In case of Virtio 1.0, DeviceId can
-  // immediately be restricted to VIRTIO_SUBSYSTEM_ENTROPY_SOURCE, and
+  // immediately be restricted to VirtIoDeviceId, and
   // SubsystemId will only play a sanity-check role. Otherwise, DeviceId can
   // only be sanity-checked, and SubsystemId will decide.
   //
-  if ((DeviceId == 0x1040 + VIRTIO_SUBSYSTEM_ENTROPY_SOURCE) &&
+  if ((DeviceId == 0x1040 + VirtIoDeviceId) &&
       (RevisionId >= 0x01))
   {
     Virtio10 = TRUE;
@@ -419,7 +420,7 @@ IsVirtioPciRng (
     return TRUE;
   }
 
-  if (!Virtio10 && (SubsystemId == VIRTIO_SUBSYSTEM_ENTROPY_SOURCE)) {
+  if (!Virtio10 && (SubsystemId == VirtIoDeviceId)) {
     return TRUE;
   }
 
@@ -430,6 +431,21 @@ IsVirtioPciRng (
   return FALSE;
 }
 
+/**
+  This FILTER_FUNCTION checks if a handle corresponds to a Virtio RNG device at
+  the EFI_PCI_IO_PROTOCOL level.
+**/
+STATIC
+BOOLEAN
+EFIAPI
+IsVirtioPciRng (
+  IN EFI_HANDLE    Handle,
+  IN CONST CHAR16  *ReportText
+  )
+{
+  return IsVirtioPci (Handle, ReportText, VIRTIO_SUBSYSTEM_ENTROPY_SOURCE);
+}
+
 /**
   This CALLBACK_FUNCTION attempts to connect a handle non-recursively, asking
   the matching driver to produce all first-level child handles.
-- 
2.40.1

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

[Gerd Hoffmann]

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
+  };
+
+  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
+                 );
+  DevicePath = AppendDevicePathNode (
+                 DevicePath,
+                 (EFI_DEVICE_PATH_PROTOCOL *)&UartNode
+                 );
+  DevicePath = AppendDevicePathNode (
+                 DevicePath,
+                 (EFI_DEVICE_PATH_PROTOCOL *)&TerminalNode
+                 );
+
+  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

[PATCH 5/5] OvmfPkg/VirtioSerialDxe: use TPL_NOTIFY

[Gerd Hoffmann]

Apparently TPL_CALLBACK is too low, code runs into an ASSERT
complaining the new TPL is lower than the old TPL.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c b/OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c
index 522b25e9698c..e4a58deff162 100644
--- a/OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c
+++ b/OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c
@@ -158,7 +158,7 @@ VirtioSerialIoWrite (
 
   VirtioSerialRingClearTx (SerialIo->Dev, PortTx (SerialIo->PortId));
 
-  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
+  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
   if (SerialIo->WriteOffset &&
       (SerialIo->WriteOffset + *BufferSize > PORT_TX_BUFSIZE))
   {
@@ -201,7 +201,7 @@ VirtioSerialIoRead (
     goto NoData;
   }
 
-  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
+  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
   if (SerialIo->WriteOffset) {
     DEBUG ((DEBUG_VERBOSE, "%a:%d: WriteFlush %d\n", __func__, __LINE__, SerialIo->WriteOffset));
     VirtioSerialRingSendBuffer (
-- 
2.40.1

Re: [PATCH 0/5] ArmVirt: add VirtioSerialDxe

[Gerd Hoffmann]

On Fri, May 12, 2023 at 04:23:01PM +0200, Gerd Hoffmann wrote:
> 
> Gerd Hoffmann (5):
>   ArmVirt: add VirtioSerialDxe to ArmVirtQemu builds
>   ArmVirt/PlatformBootManagerLib: factor out IsVirtio()
>   ArmVirt/PlatformBootManagerLib: factor out IsVirtioPci()
>   ArmVirt/PlatformBootManagerLib: set up virtio serial as console
>   OvmfPkg/VirtioSerialDxe: use TPL_NOTIFY

Ping.  Any comments on this series?

take care,
  Gerd

Re: [PATCH 5/5] OvmfPkg/VirtioSerialDxe: use TPL_NOTIFY

[Ard Biesheuvel]

On Fri, 12 May 2023 at 16:23, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Apparently TPL_CALLBACK is too low, code runs into an ASSERT
> complaining the new TPL is lower than the old TPL.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

I'll take this as a separate fix.

> ---
>  OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c b/OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c
> index 522b25e9698c..e4a58deff162 100644
> --- a/OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c
> +++ b/OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c
> @@ -158,7 +158,7 @@ VirtioSerialIoWrite (
>
>    VirtioSerialRingClearTx (SerialIo->Dev, PortTx (SerialIo->PortId));
>
> -  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
> +  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
>    if (SerialIo->WriteOffset &&
>        (SerialIo->WriteOffset + *BufferSize > PORT_TX_BUFSIZE))
>    {
> @@ -201,7 +201,7 @@ VirtioSerialIoRead (
>      goto NoData;
>    }
>
> -  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
> +  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
>    if (SerialIo->WriteOffset) {
>      DEBUG ((DEBUG_VERBOSE, "%a:%d: WriteFlush %d\n", __func__, __LINE__, SerialIo->WriteOffset));
>      VirtioSerialRingSendBuffer (
> --
> 2.40.1
>

Re: [PATCH 2/5] ArmVirt/PlatformBootManagerLib: factor out IsVirtio()

[Ard Biesheuvel]

On Fri, 12 May 2023 at 16:23, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> IsVirtioRng() becomes just a thin wrapper for IsVirtio().
> This allows to add similar thin wrappers for other virtio
> devices in the future.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>  .../PlatformBootManagerLib/PlatformBm.c       | 26 +++++++++++++++----
>  1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 10c815378cd5..5eb6f0f9c14a 100644
> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -269,15 +269,16 @@ IsPciDisplay (
>  }
>
>  /**
> -  This FILTER_FUNCTION checks if a handle corresponds to a Virtio RNG device at
> -  the VIRTIO_DEVICE_PROTOCOL level.
> +  This function checks if a handle corresponds to the Virtio Device ID given
> +  at the VIRTIO_DEVICE_PROTOCOL level.
>  **/
>  STATIC
>  BOOLEAN
>  EFIAPI
> -IsVirtioRng (
> +IsVirtio (
>    IN EFI_HANDLE    Handle,
> -  IN CONST CHAR16  *ReportText
> +  IN CONST CHAR16  *ReportText,
> +  IN UINT16        VirtIoDeviceId
>    )
>  {
>    EFI_STATUS              Status;
> @@ -293,7 +294,22 @@ IsVirtioRng (
>    }
>
>    return (BOOLEAN)(VirtIo->SubSystemDeviceId ==
> -                   VIRTIO_SUBSYSTEM_ENTROPY_SOURCE);
> +                   VirtIoDeviceId);
> +}
> +
> +/**
> +  This FILTER_FUNCTION checks if a handle corresponds to a Virtio RNG device at
> +  the VIRTIO_DEVICE_PROTOCOL level.
> +**/
> +STATIC
> +BOOLEAN
> +EFIAPI
> +IsVirtioRng (
> +  IN EFI_HANDLE    Handle,
> +  IN CONST CHAR16  *ReportText
> +  )
> +{
> +  return IsVirtio (Handle, ReportText, VIRTIO_SUBSYSTEM_ENTROPY_SOURCE);
>  }
>
>  /**
> --
> 2.40.1
>

Re: [edk2-devel] [PATCH 1/5] ArmVirt: add VirtioSerialDxe to ArmVirtQemu builds

[Ard Biesheuvel]

On Fri, 12 May 2023 at 16:23, Gerd Hoffmann <kraxel@redhat.com> wrote:
>

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>  ArmVirtPkg/ArmVirtQemu.dsc           | 1 +
>  ArmVirtPkg/ArmVirtQemuKernel.dsc     | 1 +
>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
>  3 files changed, 3 insertions(+)
>
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 449e73b9e132..d998f816b2b3 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -448,6 +448,7 @@ [Components.common]
>    OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
>    OvmfPkg/VirtioNetDxe/VirtioNet.inf
>    OvmfPkg/VirtioRngDxe/VirtioRng.inf
> +  OvmfPkg/VirtioSerialDxe/VirtioSerial.inf
>
>    #
>    # FAT filesystem + GPT/MBR partitioning + UDF filesystem + virtio-fs
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 3cb9120e4e10..c9d2b18392b9 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -353,6 +353,7 @@ [Components.common]
>    OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
>    OvmfPkg/VirtioNetDxe/VirtioNet.inf
>    OvmfPkg/VirtioRngDxe/VirtioRng.inf
> +  OvmfPkg/VirtioSerialDxe/VirtioSerial.inf
>
>    #
>    # FAT filesystem + GPT/MBR partitioning + UDF filesystem + virtio-fs
> diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> index 8a063bac04ac..2894bc853a46 100644
> --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> @@ -98,6 +98,7 @@ [FV.FvMain]
>    INF OvmfPkg/VirtioNetDxe/VirtioNet.inf
>    INF OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
>    INF OvmfPkg/VirtioRngDxe/VirtioRng.inf
> +  INF OvmfPkg/VirtioSerialDxe/VirtioSerial.inf
>
>    INF ShellPkg/Application/Shell/Shell.inf
>    INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> --
> 2.40.1
>
>
>
> 
>
>

Re: [PATCH 3/5] ArmVirt/PlatformBootManagerLib: factor out IsVirtioPci()

[Ard Biesheuvel]

On Fri, 12 May 2023 at 16:23, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> IsVirtioPciRng() becomes just a thin wrapper for IsVirtioPci().
> This allows to add similar thin wrappers for other virtio
> devices in the future.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>  .../PlatformBootManagerLib/PlatformBm.c       | 30 ++++++++++++++-----
>  1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 5eb6f0f9c14a..ed38c42a43ee 100644
> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -313,15 +313,16 @@ IsVirtioRng (
>  }
>
>  /**
> -  This FILTER_FUNCTION checks if a handle corresponds to a Virtio RNG device at
> -  the EFI_PCI_IO_PROTOCOL level.
> +  This function checks if a handle corresponds to the Virtio Device ID given
> +  at the EFI_PCI_IO_PROTOCOL level.
>  **/
>  STATIC
>  BOOLEAN
>  EFIAPI
> -IsVirtioPciRng (
> +IsVirtioPci (
>    IN EFI_HANDLE    Handle,
> -  IN CONST CHAR16  *ReportText
> +  IN CONST CHAR16  *ReportText,
> +  IN UINT16        VirtIoDeviceId
>    )
>  {
>    EFI_STATUS           Status;
> @@ -387,11 +388,11 @@ IsVirtioPciRng (
>    //
>    // From DeviceId and RevisionId, determine whether the device is a
>    // modern-only Virtio 1.0 device. In case of Virtio 1.0, DeviceId can
> -  // immediately be restricted to VIRTIO_SUBSYSTEM_ENTROPY_SOURCE, and
> +  // immediately be restricted to VirtIoDeviceId, and
>    // SubsystemId will only play a sanity-check role. Otherwise, DeviceId can
>    // only be sanity-checked, and SubsystemId will decide.
>    //
> -  if ((DeviceId == 0x1040 + VIRTIO_SUBSYSTEM_ENTROPY_SOURCE) &&
> +  if ((DeviceId == 0x1040 + VirtIoDeviceId) &&
>        (RevisionId >= 0x01))
>    {
>      Virtio10 = TRUE;
> @@ -419,7 +420,7 @@ IsVirtioPciRng (
>      return TRUE;
>    }
>
> -  if (!Virtio10 && (SubsystemId == VIRTIO_SUBSYSTEM_ENTROPY_SOURCE)) {
> +  if (!Virtio10 && (SubsystemId == VirtIoDeviceId)) {
>      return TRUE;
>    }
>
> @@ -430,6 +431,21 @@ IsVirtioPciRng (
>    return FALSE;
>  }
>
> +/**
> +  This FILTER_FUNCTION checks if a handle corresponds to a Virtio RNG device at
> +  the EFI_PCI_IO_PROTOCOL level.
> +**/
> +STATIC
> +BOOLEAN
> +EFIAPI
> +IsVirtioPciRng (
> +  IN EFI_HANDLE    Handle,
> +  IN CONST CHAR16  *ReportText
> +  )
> +{
> +  return IsVirtioPci (Handle, ReportText, VIRTIO_SUBSYSTEM_ENTROPY_SOURCE);
> +}
> +
>  /**
>    This CALLBACK_FUNCTION attempts to connect a handle non-recursively, asking
>    the matching driver to produce all first-level child handles.
> --
> 2.40.1
>

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

[Ard Biesheuvel]

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
>

Re: [PATCH 5/5] OvmfPkg/VirtioSerialDxe: use TPL_NOTIFY

[Ard Biesheuvel]

On Thu, 1 Jun 2023 at 09:56, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 12 May 2023 at 16:23, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > Apparently TPL_CALLBACK is too low, code runs into an ASSERT
> > complaining the new TPL is lower than the old TPL.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>
> I'll take this as a separate fix.
>

Merged as #4461

> > ---
> >  OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c b/OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c
> > index 522b25e9698c..e4a58deff162 100644
> > --- a/OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c
> > +++ b/OvmfPkg/VirtioSerialDxe/VirtioSerialPort.c
> > @@ -158,7 +158,7 @@ VirtioSerialIoWrite (
> >
> >    VirtioSerialRingClearTx (SerialIo->Dev, PortTx (SerialIo->PortId));
> >
> > -  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
> > +  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
> >    if (SerialIo->WriteOffset &&
> >        (SerialIo->WriteOffset + *BufferSize > PORT_TX_BUFSIZE))
> >    {
> > @@ -201,7 +201,7 @@ VirtioSerialIoRead (
> >      goto NoData;
> >    }
> >
> > -  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
> > +  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
> >    if (SerialIo->WriteOffset) {
> >      DEBUG ((DEBUG_VERBOSE, "%a:%d: WriteFlush %d\n", __func__, __LINE__, SerialIo->WriteOffset));
> >      VirtioSerialRingSendBuffer (
> > --
> > 2.40.1
> >