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

[Laszlo Ersek <lersek@redhat.com>]

On 02/27/18 12:30, Guo Heyi wrote:
> On Tue, Feb 27, 2018 at 11:43:35AM +0100, Laszlo Ersek 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.
> 
> On our platforms, we actually use
> IntelFrameworkModulePkg/PeiDxeDebugLibReportStatusCode,
> MdeModulePkg/RuntimeDxeReportStatusCodeLib and
> IntelFrameworkModulePkg/StatusCodeRuntimeDxe as the whole DebugLib call chain
> for runtime drivers. Is StatusCodeRuntimeDxe a good place for GCD manipulation?

Sure; if DebugLib (and the library instances below it) don't directly
access hardware, but make calls to some shared code -- i.e. into another
runtime DXE driver via a protocol interface originally looked up /
stashed at boot time, and then converted to a virtual address --, then
only that runtime DXE driver needs to manage the GCD map.

> But I've no idea on how to distinguish the code between ARM and X86, for X86
> uses port IO for serial port access.

If you can ascertain that the lib instance linked into
StatusCodeRuntimeDxe (a) is not used by any other module, and (b)
performs the hardware access, then you could push the GCD management
into that lib instance. Or else, you could write a plug-in lib instance
and hook that with NULL resolution into the driver.

But, I guess, at this point it's simplest to modify PlatformPei or
PlatformDxe to add that entry to GCD in advance, regardless of the
actual number of runtime DXE drivers that will rely on the mapping.

Laszlo