Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 27 February 2018 at 16:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 27 February 2018 at 16:09, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 02/27/18 15:21, Ard Biesheuvel wrote:
>>> On 27 February 2018 at 10:43, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> On 02/27/18 10:23, Ard Biesheuvel wrote:
>>>>> On 27 February 2018 at 01:50, Guo Heyi <heyi.guo@linaro.org> 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.