From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id A77A6D8021D for ; Fri, 6 Oct 2023 17:08:24 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=rmv/OA2NU2Pp7UED+TrORoWomsRfW6irx+U7TkXcCiw=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1696612103; v=1; b=vTa2TLcsr8oDbL+Ui8kKt7cWLOlgoatTVdnx8izI+9E2rFP9PNyXbNfcxXlzyDsrj03zTzHl 4l32vBi630kXoVX0AMaW1ZOGnF35qLvb7vcmA/GC3+wtfH20/g/oHD3PmQZQjuxqmtZKOaDMNVH 0m9pkz1o9r/5KVM9zPCuHaVA= X-Received: by 127.0.0.2 with SMTP id IkwyYY7687511xnJlmizlM4V; Fri, 06 Oct 2023 10:08:23 -0700 X-Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.18606.1696612102705404171 for ; Fri, 06 Oct 2023 10:08:22 -0700 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id E6FBD61283 for ; Fri, 6 Oct 2023 17:08:21 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id 09EDFC433D9 for ; Fri, 6 Oct 2023 17:08:21 +0000 (UTC) X-Received: by mail-lj1-f169.google.com with SMTP id 38308e7fff4ca-2c008042211so28212891fa.2 for ; Fri, 06 Oct 2023 10:08:20 -0700 (PDT) X-Gm-Message-State: VusmtbX6YoH2qoHXUFq0dNE9x7686176AA= X-Google-Smtp-Source: AGHT+IGVRJfu/DpVF9aDyV26MuGf40+atjd/qwV1z6F2C75m9fsiU04lDHmzBVf4E+A9wtPfzsyjiIVVorAYfYS03fQ= X-Received: by 2002:a05:6512:e96:b0:500:b3fe:916e with SMTP id bi22-20020a0565120e9600b00500b3fe916emr9413808lfb.2.1696612099124; Fri, 06 Oct 2023 10:08:19 -0700 (PDT) MIME-Version: 1.0 References: <20230930212343.130015-1-lersek@redhat.com> In-Reply-To: From: "Ard Biesheuvel" Date: Fri, 6 Oct 2023 19:08:07 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: initialize implicitly To: devel@edk2.groups.io, lersek@redhat.com Cc: Ard Biesheuvel , Gerd Hoffmann , Leif Lindholm , Oliver Smith-Denny , Oliver Steffen , Sami Mujawar , Sean Brogan Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=vTa2TLcs; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (policy=none) On Mon, 2 Oct 2023 at 16:47, Laszlo Ersek wrote: > > On 9/30/23 23:23, Laszlo Ersek wrote: > > FdtPL011SerialPortLib claims that it's usable from the DXE_CORE. That's > > not correct: the DXE_CORE calls DEBUG() and ASSERT() before it calls > > ProcessLibraryConstructorList(). Via the BaseDebugLibSerialPort instance, > > those DEBUG() and ASSERT() calls result in SerialPortWrite() calls, before > > ProcessLibraryConstructorList() called either our constructor > > FdtPL011SerialPortLibInitialize(), or BaseDebugLibSerialPortConstructor(). > > > > (And even if the DXE_CORE called the latter function early enough, it > > would just invoke our SerialPortInitialize() function -- which does > > nothing.) > > > > This means that the earliest DXE_CORE debug messages are lost. > > > > Rename FdtPL011SerialPortLibInitialize() to SerialPortInitialize(), so > > that the same initialization occur through the constructor and the public > > SerialPortInitialize() library API. > > > > Turn SerialPortInitialize() calls after the first one into no-ops. > > > > Our SerialPortLib APIs already use (mSerialBaseAddress != 0) to track > > initialization. Rework those checks to actually initialize the library if > > that hasn't happened yet. > > > > The following new lines appear in the log: > > > >> CoreInitializeMemoryServices: > >> BaseAddress - 0x48000000 Length - 0xF8000000 MinimalMemorySizeNeeded - 0x38C8000 > >> InstallProtocolInterface: [EfiLoadedImageProtocol] 46EFC3E0 > >> ProtectUefiImageCommon - 0x46EFC3E0 > >> - 0x0000000046EB2000 - 0x0000000000068000 > > > > (0x46EB2000 is the load address of the DXE Core.) > > > > Cc: Ard Biesheuvel > > Cc: Gerd Hoffmann > > Cc: Leif Lindholm > > Cc: Oliver Smith-Denny > > Cc: Oliver Steffen > > Cc: Sami Mujawar > > Cc: Sean Brogan > > Reported-by: Oliver Smith-Denny > > Signed-off-by: Laszlo Ersek > > --- > > > > Notes: > > In my opinion this is the wrong approach to fix the bug, but the right > > approach would require so much work all over edk2 (and outside of it) > > that it's just not practical. > > NB I'd still like this patch to be merged; "wrong approach" is too > strongly worded. I'd now refine that as "theoretically not 100% pure". > Merging this one is a compromise, but an easy one -- I had worked > several hours on the "pure approach", and it absolutely spiralled out of > control. So this is the only one practical thing to do. > Reviewed-by: Ard Biesheuvel I'll queue this up. > > > > ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf | 2 +- > > ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 84 ++++++++++++-------- > > 2 files changed, 52 insertions(+), 34 deletions(-) > > > > diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf > > index 2b9a34aa30d1..c417514e3ed5 100644 > > --- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf > > +++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf > > @@ -15,7 +15,7 @@ [Defines] > > MODULE_TYPE = BASE > > VERSION_STRING = 1.0 > > LIBRARY_CLASS = SerialPortLib|DXE_CORE DXE_DRIVER UEFI_DRIVER DXE_RUNTIME_DRIVER UEFI_APPLICATION > > - CONSTRUCTOR = FdtPL011SerialPortLibInitialize > > + CONSTRUCTOR = SerialPortInitialize > > > > [Sources.common] > > FdtPL011SerialPortLib.c > > diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c > > index 614ea5a860b9..9d71f369570f 100644 > > --- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c > > +++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c > > @@ -23,49 +23,57 @@ > > #include > > #include > > > > -STATIC UINTN mSerialBaseAddress; > > - > > -RETURN_STATUS > > -EFIAPI > > -SerialPortInitialize ( > > - VOID > > - ) > > -{ > > - return RETURN_SUCCESS; > > -} > > +STATIC UINTN mSerialBaseAddress; > > +STATIC RETURN_STATUS mPermanentStatus = RETURN_SUCCESS; > > > > /** > > - > > Program hardware of Serial port > > > > - @return RETURN_NOT_FOUND if no PL011 base address could be found > > - Otherwise, result of PL011UartInitializePort () is returned > > + @retval RETURN_SUCCESS If the serial port was initialized successfully by > > + this call, or an earlier call, to > > + SerialPortInitialize(). > > > > + @retval RETURN_NOT_FOUND If no PL011 base address could be found. > > + > > + @return Error codes forwarded from > > + PL011UartInitializePort(). > > **/ > > RETURN_STATUS > > EFIAPI > > -FdtPL011SerialPortLibInitialize ( > > +SerialPortInitialize ( > > VOID > > ) > > { > > VOID *Hob; > > + RETURN_STATUS Status; > > CONST UINT64 *UartBase; > > + UINTN SerialBaseAddress; > > UINT64 BaudRate; > > UINT32 ReceiveFifoDepth; > > EFI_PARITY_TYPE Parity; > > UINT8 DataBits; > > EFI_STOP_BITS_TYPE StopBits; > > > > + if (mSerialBaseAddress != 0) { > > + return RETURN_SUCCESS; > > + } > > + > > + if (RETURN_ERROR (mPermanentStatus)) { > > + return mPermanentStatus; > > + } > > + > > Hob = GetFirstGuidHob (&gEarlyPL011BaseAddressGuid); > > if ((Hob == NULL) || (GET_GUID_HOB_DATA_SIZE (Hob) != sizeof *UartBase)) { > > - return RETURN_NOT_FOUND; > > + Status = RETURN_NOT_FOUND; > > + goto Failed; > > } > > > > UartBase = GET_GUID_HOB_DATA (Hob); > > > > - mSerialBaseAddress = (UINTN)*UartBase; > > - if (mSerialBaseAddress == 0) { > > - return RETURN_NOT_FOUND; > > + SerialBaseAddress = (UINTN)*UartBase; > > + if (SerialBaseAddress == 0) { > > + Status = RETURN_NOT_FOUND; > > + goto Failed; > > } > > > > BaudRate = (UINTN)PcdGet64 (PcdUartDefaultBaudRate); > > @@ -74,15 +82,25 @@ FdtPL011SerialPortLibInitialize ( > > DataBits = PcdGet8 (PcdUartDefaultDataBits); > > StopBits = (EFI_STOP_BITS_TYPE)PcdGet8 (PcdUartDefaultStopBits); > > > > - return PL011UartInitializePort ( > > - mSerialBaseAddress, > > - FixedPcdGet32 (PL011UartClkInHz), > > - &BaudRate, > > - &ReceiveFifoDepth, > > - &Parity, > > - &DataBits, > > - &StopBits > > - ); > > + Status = PL011UartInitializePort ( > > + SerialBaseAddress, > > + FixedPcdGet32 (PL011UartClkInHz), > > + &BaudRate, > > + &ReceiveFifoDepth, > > + &Parity, > > + &DataBits, > > + &StopBits > > + ); > > + if (RETURN_ERROR (Status)) { > > + goto Failed; > > + } > > + > > + mSerialBaseAddress = SerialBaseAddress; > > + return RETURN_SUCCESS; > > + > > +Failed: > > + mPermanentStatus = Status; > > + return Status; > > } > > > > /** > > @@ -102,7 +120,7 @@ SerialPortWrite ( > > IN UINTN NumberOfBytes > > ) > > { > > - if (mSerialBaseAddress != 0) { > > + if (!RETURN_ERROR (SerialPortInitialize ())) { > > return PL011UartWrite (mSerialBaseAddress, Buffer, NumberOfBytes); > > } > > > > @@ -126,7 +144,7 @@ SerialPortRead ( > > IN UINTN NumberOfBytes > > ) > > { > > - if (mSerialBaseAddress != 0) { > > + if (!RETURN_ERROR (SerialPortInitialize ())) { > > return PL011UartRead (mSerialBaseAddress, Buffer, NumberOfBytes); > > } > > > > @@ -146,7 +164,7 @@ SerialPortPoll ( > > VOID > > ) > > { > > - if (mSerialBaseAddress != 0) { > > + if (!RETURN_ERROR (SerialPortInitialize ())) { > > return PL011UartPoll (mSerialBaseAddress); > > } > > > > @@ -199,7 +217,7 @@ SerialPortSetAttributes ( > > { > > RETURN_STATUS Status; > > > > - if (mSerialBaseAddress == 0) { > > + if (RETURN_ERROR (SerialPortInitialize ())) { > > Status = RETURN_UNSUPPORTED; > > } else { > > Status = PL011UartInitializePort ( > > @@ -234,7 +252,7 @@ SerialPortSetControl ( > > { > > RETURN_STATUS Status; > > > > - if (mSerialBaseAddress == 0) { > > + if (RETURN_ERROR (SerialPortInitialize ())) { > > Status = RETURN_UNSUPPORTED; > > } else { > > Status = PL011UartSetControl (mSerialBaseAddress, Control); > > @@ -261,7 +279,7 @@ SerialPortGetControl ( > > { > > RETURN_STATUS Status; > > > > - if (mSerialBaseAddress == 0) { > > + if (RETURN_ERROR (SerialPortInitialize ())) { > > Status = RETURN_UNSUPPORTED; > > } else { > > Status = PL011UartGetControl (mSerialBaseAddress, Control); > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109366): https://edk2.groups.io/g/devel/message/109366 Mute This Topic: https://groups.io/mt/101682671/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-