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:c06::233; helo=mail-io0-x233.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x233.google.com (mail-io0-x233.google.com [IPv6:2607:f8b0:4001:c06::233]) (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 40FCF22352290 for ; Tue, 27 Feb 2018 09:27:05 -0800 (PST) Received: by mail-io0-x233.google.com with SMTP id p78so84007iod.13 for ; Tue, 27 Feb 2018 09:33:11 -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=fLlPVFbZMP1NoW3FfkTyfdHSkmy21lNNXwgjuIX4Zvs=; b=QnU78U7Tks1SJPspLQs41mY31Hf7pmIB1Lr/dD2VoIjTvmziQ7pThLf0/haIxnQlKW xIeGSENQPN9ixPkXHu/1hHCYCwk00TnIi/oX4Vt0Boh26SofhE5VJ25hGAn2xct8Txom FuWkcf1n5lDpS+WzhIQ+F9sHwr8YQPs7aoS4s= 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=fLlPVFbZMP1NoW3FfkTyfdHSkmy21lNNXwgjuIX4Zvs=; b=bpY5wZFSIPGxyhI9gmGueC/UrKYzN9tpHhHH48l7p3x1S6qrX3lO4KU3JrcKgNQLyg 8TlfMiBF+zPln41khFgjtsg1EaGJCdjzkjUowWHuIh4pzPmWaTCEwrPUc4uewiySFefO m414uCT2PnyLk4WrjEC5R+loVUGvFRMEyfyS5fvLv5+m9S4fYu21uhsNIp1u2NjJ6FwJ asYRwvaYLG049i0l8eoYdjm7Yne3zjDyewLLVXqvfr1dOVqDBRiPDL0qtyCvoKaVqDWB iAl38ybs6EB2V4epeUd/S9IbLSOOMC5wIvFcM1/zzTAuVFrHp68ZaNx9ZC5OoigIx2jL mD8A== X-Gm-Message-State: APf1xPA0gD9kbM8HSUxKeJZooZNEKqJ/+NJqRCg/B0iHb4D99JM3ZH7q vVb1qCnbbjYGL+U3R+O1zE+ma/tZA9cQQVnRpTFb+hX+R7A= X-Google-Smtp-Source: AG47ELtzVdbytoch4Ka/dAp0e2m62ziym0U7HvZv+xos4dBP1KtY8AeowpivcC+NSQHGRN20DG9KuK8mWrDUBQ2/EzA= X-Received: by 10.107.5.199 with SMTP id 190mr17719659iof.107.1519752790855; Tue, 27 Feb 2018 09:33:10 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.138.209 with HTTP; Tue, 27 Feb 2018 09:33:10 -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:33:10 +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:27:05 -0000 Content-Type: text/plain; charset="UTF-8" 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.