From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::232; helo=mail-it0-x232.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x232.google.com (mail-it0-x232.google.com [IPv6:2607:f8b0:4001:c0b::232]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id EC0A722352289 for ; Tue, 27 Feb 2018 09:49:16 -0800 (PST) Received: by mail-it0-x232.google.com with SMTP id w63so202869ita.3 for ; Tue, 27 Feb 2018 09:55:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=PSmZJCPNt+soCPG1FNozCbyj7ubiiK+USRSQULhsz6k=; b=F3JqD/KeU3MGdW+BNrFJTMQfwdVMifHR6CPI8A8OvPC7oJd/mb+GXnAbVKI0XxHO23 1MVa0POgJBdaAEuffbED4jNe0A8XzTDYWNoVCjdLi6PsN/S2AKS39t/LCfAwpbhSbvg6 J+Syl7AyVZ3qAHHfDGFcRuRCf3F90ncf3RocE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=PSmZJCPNt+soCPG1FNozCbyj7ubiiK+USRSQULhsz6k=; b=lTLh54U3p+pjeU4YaWifeSoKqU+oaHq1JRSH7sfUhi+qkHVVU6aPiEnFdwgn7olSMo q5vT537rAHTlDZWkkULUIMjVtBBs4Fb36vvUzZIzpoXu4mcu1SK8BU4r9lHGANue8dTH IkxJggO669qSHEU28jo3CNsjfud+3gIchnovcHow9rxZ0lkbzFhUGzWr8flZ73Ef4TH3 yia8IMzpLitu8e0VY8pEA+eUWqT3fBXIjNDYjgUkerV27DRdfvzMrTCiuq8N/34yReRg uyJWgj8NtnUm4AHJLJAJX8e5YJGNYhl00zD5BqZN1Tl/G0mSGR7vxtLlGJHzevK6ZlJW lTyg== X-Gm-Message-State: APf1xPCmPxskclfv8YrsRQNlYDrq4E04F31q2MLuMrKxS6stymcDHLTz tUzolFEjx1S8o7qXGYAz4kRQXGoEy9jdb7kD6F7Qhw== X-Google-Smtp-Source: AG47ELv4vmriFtkfrXEOZARVoeD8ecT0P00YasnMFGbFwweScE3iKFikXJFAqWRdT6ahXUDIQpJm9toKGFZqWTCJJ20= X-Received: by 10.36.90.5 with SMTP id v5mr17826980ita.138.1519754122566; Tue, 27 Feb 2018 09:55:22 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.138.209 with HTTP; Tue, 27 Feb 2018 09:55:22 -0800 (PST) In-Reply-To: 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: Ard Biesheuvel Date: Tue, 27 Feb 2018 17:55:22 +0000 Message-ID: To: Laszlo Ersek Cc: Guo Heyi , "edk2-devel@lists.01.org" , Leif Lindholm 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:49:17 -0000 Content-Type: text/plain; charset="UTF-8" On 27 February 2018 at 17:47, Laszlo Ersek wrote: > 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? > Good point. I think that should be fine