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

[Laszlo Ersek <lersek@redhat.com>]

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?

Thanks,
Laszlo