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 0D039941D5E for ; Mon, 2 Oct 2023 14:47:14 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=WBcyD4fqx4D4QwHk5BXEtyVONFI/3nPFQBVYFDpGvDc=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:From:To:Cc:Reply-To:References:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1696258033; v=1; b=bhG4u2d0ZziX2Fzp9SM+7PmGtp1nZ2DKDKHb186kUqEe3Wecl72ATK/K1p2gWO8DJ3hs8/KU 48HfvxxQh99jsX2SEfSs5w41mJ8XP/nTg3RE4gBLCpXo3apfv8Enk97qzxtdiZ2ry6LiuJ7mIou PvoXu7Ix6c5CsO3IccKBYlLA= X-Received: by 127.0.0.2 with SMTP id xJf2YY7687511x9xDPqpK6Vm; Mon, 02 Oct 2023 07:47:13 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.83889.1696258032957493115 for ; Mon, 02 Oct 2023 07:47:13 -0700 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-264-yq19kjuJMuyoUKlkE-GATQ-1; Mon, 02 Oct 2023 10:47:03 -0400 X-MC-Unique: yq19kjuJMuyoUKlkE-GATQ-1 X-Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 7333D858F19; Mon, 2 Oct 2023 14:47:03 +0000 (UTC) X-Received: from [10.39.194.43] (unknown [10.39.194.43]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D2FF14026FB; Mon, 2 Oct 2023 14:47:01 +0000 (UTC) Message-ID: Date: Mon, 2 Oct 2023 16:47:00 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: initialize implicitly From: "Laszlo Ersek" To: devel@edk2.groups.io Cc: Ard Biesheuvel , Gerd Hoffmann , Leif Lindholm , Oliver Smith-Denny , Oliver Steffen , Sami Mujawar , Sean Brogan Reply-To: devel@edk2.groups.io,lersek@redhat.com References: <20230930212343.130015-1-lersek@redhat.com> In-Reply-To: <20230930212343.130015-1-lersek@redhat.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com 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 List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: urG4tMBfWtwnlknmrGVuhQjQx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=bhG4u2d0; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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, befor= e > ProcessLibraryConstructorList() called either our constructor > FdtPL011SerialPortLibInitialize(), or BaseDebugLibSerialPortConstructor()= . >=20 > (And even if the DXE_CORE called the latter function early enough, it > would just invoke our SerialPortInitialize() function -- which does > nothing.) >=20 > This means that the earliest DXE_CORE debug messages are lost. >=20 > Rename FdtPL011SerialPortLibInitialize() to SerialPortInitialize(), so > that the same initialization occur through the constructor and the public > SerialPortInitialize() library API. >=20 > Turn SerialPortInitialize() calls after the first one into no-ops. >=20 > Our SerialPortLib APIs already use (mSerialBaseAddress !=3D 0) to track > initialization. Rework those checks to actually initialize the library if > that hasn't happened yet. >=20 > The following new lines appear in the log: >=20 >> CoreInitializeMemoryServices: >> BaseAddress - 0x48000000 Length - 0xF8000000 MinimalMemorySizeNeeded -= 0x38C8000 >> InstallProtocolInterface: [EfiLoadedImageProtocol] 46EFC3E0 >> ProtectUefiImageCommon - 0x46EFC3E0 >> - 0x0000000046EB2000 - 0x0000000000068000 >=20 > (0x46EB2000 is the load address of the DXE Core.) >=20 > 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 > --- >=20 > Notes: > In my opinion this is the wrong approach to fix the bug, but the righ= t > 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. Thanks Laszlo >=20 > ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf | 2 = +- > ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 84 = ++++++++++++-------- > 2 files changed, 52 insertions(+), 34 deletions(-) >=20 > diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortL= ib.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 =3D BASE > VERSION_STRING =3D 1.0 > LIBRARY_CLASS =3D SerialPortLib|DXE_CORE DXE_DRIVER U= EFI_DRIVER DXE_RUNTIME_DRIVER UEFI_APPLICATION > - CONSTRUCTOR =3D FdtPL011SerialPortLibInitialize > + CONSTRUCTOR =3D SerialPortInitialize > =20 > [Sources.common] > FdtPL011SerialPortLib.c > diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortL= ib.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 > =20 > -STATIC UINTN mSerialBaseAddress; > - > -RETURN_STATUS > -EFIAPI > -SerialPortInitialize ( > - VOID > - ) > -{ > - return RETURN_SUCCESS; > -} > +STATIC UINTN mSerialBaseAddress; > +STATIC RETURN_STATUS mPermanentStatus =3D RETURN_SUCCESS; > =20 > /** > - > Program hardware of Serial port > =20 > - @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 successfu= lly by > + this call, or an earlier call, to > + SerialPortInitialize(). > =20 > + @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; > =20 > + if (mSerialBaseAddress !=3D 0) { > + return RETURN_SUCCESS; > + } > + > + if (RETURN_ERROR (mPermanentStatus)) { > + return mPermanentStatus; > + } > + > Hob =3D GetFirstGuidHob (&gEarlyPL011BaseAddressGuid); > if ((Hob =3D=3D NULL) || (GET_GUID_HOB_DATA_SIZE (Hob) !=3D sizeof *Ua= rtBase)) { > - return RETURN_NOT_FOUND; > + Status =3D RETURN_NOT_FOUND; > + goto Failed; > } > =20 > UartBase =3D GET_GUID_HOB_DATA (Hob); > =20 > - mSerialBaseAddress =3D (UINTN)*UartBase; > - if (mSerialBaseAddress =3D=3D 0) { > - return RETURN_NOT_FOUND; > + SerialBaseAddress =3D (UINTN)*UartBase; > + if (SerialBaseAddress =3D=3D 0) { > + Status =3D RETURN_NOT_FOUND; > + goto Failed; > } > =20 > BaudRate =3D (UINTN)PcdGet64 (PcdUartDefaultBaudRate); > @@ -74,15 +82,25 @@ FdtPL011SerialPortLibInitialize ( > DataBits =3D PcdGet8 (PcdUartDefaultDataBits); > StopBits =3D (EFI_STOP_BITS_TYPE)PcdGet8 (PcdUartDefaultStopBi= ts); > =20 > - return PL011UartInitializePort ( > - mSerialBaseAddress, > - FixedPcdGet32 (PL011UartClkInHz), > - &BaudRate, > - &ReceiveFifoDepth, > - &Parity, > - &DataBits, > - &StopBits > - ); > + Status =3D PL011UartInitializePort ( > + SerialBaseAddress, > + FixedPcdGet32 (PL011UartClkInHz), > + &BaudRate, > + &ReceiveFifoDepth, > + &Parity, > + &DataBits, > + &StopBits > + ); > + if (RETURN_ERROR (Status)) { > + goto Failed; > + } > + > + mSerialBaseAddress =3D SerialBaseAddress; > + return RETURN_SUCCESS; > + > +Failed: > + mPermanentStatus =3D Status; > + return Status; > } > =20 > /** > @@ -102,7 +120,7 @@ SerialPortWrite ( > IN UINTN NumberOfBytes > ) > { > - if (mSerialBaseAddress !=3D 0) { > + if (!RETURN_ERROR (SerialPortInitialize ())) { > return PL011UartWrite (mSerialBaseAddress, Buffer, NumberOfBytes); > } > =20 > @@ -126,7 +144,7 @@ SerialPortRead ( > IN UINTN NumberOfBytes > ) > { > - if (mSerialBaseAddress !=3D 0) { > + if (!RETURN_ERROR (SerialPortInitialize ())) { > return PL011UartRead (mSerialBaseAddress, Buffer, NumberOfBytes); > } > =20 > @@ -146,7 +164,7 @@ SerialPortPoll ( > VOID > ) > { > - if (mSerialBaseAddress !=3D 0) { > + if (!RETURN_ERROR (SerialPortInitialize ())) { > return PL011UartPoll (mSerialBaseAddress); > } > =20 > @@ -199,7 +217,7 @@ SerialPortSetAttributes ( > { > RETURN_STATUS Status; > =20 > - if (mSerialBaseAddress =3D=3D 0) { > + if (RETURN_ERROR (SerialPortInitialize ())) { > Status =3D RETURN_UNSUPPORTED; > } else { > Status =3D PL011UartInitializePort ( > @@ -234,7 +252,7 @@ SerialPortSetControl ( > { > RETURN_STATUS Status; > =20 > - if (mSerialBaseAddress =3D=3D 0) { > + if (RETURN_ERROR (SerialPortInitialize ())) { > Status =3D RETURN_UNSUPPORTED; > } else { > Status =3D PL011UartSetControl (mSerialBaseAddress, Control); > @@ -261,7 +279,7 @@ SerialPortGetControl ( > { > RETURN_STATUS Status; > =20 > - if (mSerialBaseAddress =3D=3D 0) { > + if (RETURN_ERROR (SerialPortInitialize ())) { > Status =3D RETURN_UNSUPPORTED; > } else { > Status =3D PL011UartGetControl (mSerialBaseAddress, Control); >=20 >=20 >=20 >=20 >=20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109255): https://edk2.groups.io/g/devel/message/109255 Mute This Topic: https://groups.io/mt/101682671/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-