From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 963EC209574E7 for ; Tue, 27 Feb 2018 09:41:14 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4D57E40363BE; Tue, 27 Feb 2018 17:47:20 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-21.rdu2.redhat.com [10.10.120.21]) by smtp.corp.redhat.com (Postfix) with ESMTP id 699922024CA2; Tue, 27 Feb 2018 17:47:19 +0000 (UTC) To: Ard Biesheuvel Cc: Guo Heyi , "edk2-devel@lists.01.org" , Leif Lindholm References: <20180224142515.461-1-ard.biesheuvel@linaro.org> <20180227015036.GC2261@SZX1000114654> <27ef5753-9c6c-3b99-e732-084d9e444158@redhat.com> <2e4facde-6d3f-ef42-f8ee-6446736f3617@redhat.com> From: Laszlo Ersek Message-ID: Date: Tue, 27 Feb 2018 18:47:18 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Tue, 27 Feb 2018 17:47:20 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Tue, 27 Feb 2018 17:47:20 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Feb 2018 17:41:15 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/27/18 18:33, Ard Biesheuvel wrote: > On 27 February 2018 at 16:10, Ard Biesheuvel wrote: >> On 27 February 2018 at 16:09, Laszlo Ersek wrote: >>> On 02/27/18 15:21, Ard Biesheuvel wrote: >>>> On 27 February 2018 at 10:43, Laszlo Ersek wrote: >>>>> On 02/27/18 10:23, Ard Biesheuvel wrote: >>>>>> On 27 February 2018 at 01:50, Guo Heyi wrote: >>>>>>> Hi Ard, >>>>>>> >>>>>>> Sorry for the late of seeing this patch. I have one question: why don't we >>>>>>> implement a runtime serial port lib, which will switch UART base address in >>>>>>> virtual address map change? I think this will be useful when we want to debug >>>>>>> runtime driver in OS stage. And if we have a runtime version of SerialPortLib, >>>>>>> then we don't need a runtime version of DebugLib which just disable touching >>>>>>> serial port. >>>>>>> >>>>>> >>>>>> Well, only if the serial port is not exposed to the OS as well. The >>>>>> Linux PL011 driver is especially easy to confuse, and having both the >>>>>> firmware and the OS control it at the same time is likely to cause >>>>>> problems. >>>>>> >>>>>> However, I do agree that having the ability to assign a UART to DEBUG >>>>>> at runtime is useful, and so I do intend to create a runtime version >>>>>> of the PL011 library, in which case DxeRuntimeDebugLibSerialPort can >>>>>> be replaced with BaseDebugLibSerialPort for DXE_RUNTIME_DRIVER >>>>>> modules. >>>>>> >>>>> >>>>> Converting the PL011 base address from phys to virt can be done in the >>>>> library instance, yes (and then every runtime driver module linked >>>>> against this library instance will individually convert the address for >>>>> its own use). The messier aspect is getting the PL011 base address into >>>>> the UEFI memmap, marked as MMIO / RUNTIME, so that the OS assign a >>>>> virtual mapping to it in the first place. >>>>> >>>>> The flash drivers generally do this with AddMemorySpace / >>>>> SetMemorySpaceAttributes. >>>>> >>>>> (Side point: while I agree that those are good APIs to invoke, I think >>>>> they should also call AllocateMemorySpace right after; otherwise a >>>>> "stray" AllocateMemorySpace elsewhere could allocate a chunk out of the >>>>> middle of what the flash driver *thinks* it owns.) >>>>> >>>>> The question is where this pair (or triplet) of GCD APIs should be called: >>>>> >>>>> - In a platform DXE driver? Perhaps. >>>>> >>>>> - In the DebugLib instance constructor / destructor? That could result >>>>> in some ugly reference counting -- you might want to keep the PL011 area >>>>> registered in GCD as long as *at least one* such runtime driver is loaded. >>>>> >>>>> This is different from the flash driver because the flash driver is the >>>>> sole runtime DXE (or SMM) driver that accesses & owns the flash MMIO >>>>> range. With the PL011 register block, that's not the case; all runtime >>>>> drivers that produce debug messages own it co-operatively. >>>>> >>>> >>>> Given that this SerialPortLib will be of the DXE_RUNTIME_DRIVER type >>>> anyway, couldn't we base it on a generic RuntimeUart protocol that we >>>> can depex on in the library, and produce in a separate [singleton] >>>> runtime DXE driver, which takes care of the UART initialization as >>>> well as the GCD memory space handling? >>> >>> Yes, that should totally work. >>> >>>> In fact, we could modify DxeRuntimeDebugLibSerialPort to attach to >>>> this protocol if it exists at EndOfDxe, and either do nothing at >>>> runtime (as it does currently) or produce UART output via the protocol >>>> if it exists. >>> >>> This is a good idea IMO. What about the following: since you'd have to >>> register an event notification function for EndOfDxe, why not register a >>> protocol notify directly for the runtime UART protocol? >>> > > What I also like about this approach is that it defers the decision > whether to enable the DEBUG UART at OS runtime from compile time to > runtime [where 'runtime' is a tad overloaded here] > > IOW, you can use a variable, PCD or simply do 'load > fs0:\UartRuntimeDxe.efi' from the Shell to promote a 'runtime silent' > DEBUG build to one that produces DEBUG output while running under the > OS. > > I guess the only problem is that DxeRuntimeDebugLibSerialPort lives in > MdePkg, and so any extensions to it involving protocols require an ECR > against the PI spec first. > This is a brand new library instance, not many platform DSCs use it just yet. Can we simply move it to MdeModulePkg? Do we see any downsides to not offering the lib instance in MdePkg? Thanks Laszlo