From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.25743.1685607206251183383 for ; Thu, 01 Jun 2023 01:13:26 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=FjykyitM; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id B759B63FBE for ; Thu, 1 Jun 2023 08:13:25 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 26C48C433EF for ; Thu, 1 Jun 2023 08:13:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1685607205; bh=DI1ig+qD+UPpKQIqMWWf13CEWP+sYnLo6vFcvY3PUZw=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=FjykyitMylL16tPIMXZQriUw0B2mHq1xJbE/uD4fn+Spr22NBIoObK/9cr+rN6gpn p+vCmlv4bWJTxnwHd2mcBJ2Yals32PDHm3hZLWm7ERTOH7RvuS5VMiAHJPoWcg3sKU VCzxz0yBVpG5hwKefo0mKiuFFK4JEHhP1nOVSCLVQuuTMa7AXwfa9kZyj4pKbX75oN ockYak6DbcRU8Pdr7iO7hN0Sn1yZRBrAOU/y1McZrJ1ZbWPT3+dyrF+bdbsyIltFfy vJ6yj8IYKRVplLJ5I5tWIAPAnAOzBcdGqYGawS2UWGwdUMi4mQQCwxbldnqVdyREUc SHNoib1bRy0uQ== Received: by mail-lj1-f177.google.com with SMTP id 38308e7fff4ca-2af24ee004dso7395421fa.0 for ; Thu, 01 Jun 2023 01:13:25 -0700 (PDT) X-Gm-Message-State: AC+VfDw10Ss0XSVfKdOyIb7tu8HCAdCnA/BJp9291CGhfrxDeLfv7gcD iysjxUXFthwyVmIDkXhcoPuUELpcJnmwZnij+PA= X-Google-Smtp-Source: ACHHUZ4YPDh+SiP9Qz2NlGLf0XGXICRgzpRZv+drjwW4E6+ivwlXCldqgtSqOcIbBj/WDiK+kPJ0o5WOphUkrx/tW54= X-Received: by 2002:a2e:6e18:0:b0:2a8:e480:a3c8 with SMTP id j24-20020a2e6e18000000b002a8e480a3c8mr4248614ljc.44.1685607203112; Thu, 01 Jun 2023 01:13:23 -0700 (PDT) MIME-Version: 1.0 References: <20230512142306.1323983-1-kraxel@redhat.com> <20230512142306.1323983-5-kraxel@redhat.com> In-Reply-To: <20230512142306.1323983-5-kraxel@redhat.com> From: "Ard Biesheuvel" Date: Thu, 1 Jun 2023 10:13:10 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 4/5] ArmVirt/PlatformBootManagerLib: set up virtio serial as console To: Gerd Hoffmann Cc: devel@edk2.groups.io, Jiewen Yao , Jordan Justen , Leif Lindholm , Pawel Polawski , Sami Mujawar , Oliver Steffen , Ard Biesheuvel Content-Type: text/plain; charset="UTF-8" On Fri, 12 May 2023 at 16:23, Gerd Hoffmann 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 > --- > .../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 >